Code review comment for lp:~allenap/launchpad/ec2-test-race-bug-422433

Revision history for this message
Abel Deuring (adeuring) wrote :

Hi Gavin,

On 02.10.2009 14:35, Gavin Panella wrote:
> Thanks for the review!
>
>> Hi Gavin,
>>
>> a very nice branch! Just one suggestion:
>>
> ...
>> The for loops in both methods are nearly identical; the only differences I see
>> are that the log messages contain "key pair" or "secuity group", and that
>> try_delete_key_pair() or try_delete_security_group() is called. Would it make
>> sense to move the code starting at "if session_name" and ending at "else:
>> self.log(..)" into a common method?
>
> Good point. I've done this, and also ditched a lot of the noisy
> logging; it now says something only if a key pair or security group is
> deleted or if the deletion fails.
>
> Incremental diff: http://pastebin.ubuntu.com/283801/

thanks for this change! Looks good

Abel

« Back to merge proposal