Code review comment for lp:~viswesn/juju-ci-tools/ensure_provider_cleanup

Revision history for this message
Christopher Lee (veebers) wrote :

I don't think that adding unclean=[] to terminate_instances is the best way to do this. Instead you should make the call to terminate_instances and catch any exception that might occur and log that.
Changing terminate_instances like you have drastically changes how the method responds to error (it use to raise an exception in some places, now it errors silently) which might have a major effect on other parts of existing code that rely on the original behaviour.

Also of note, I don't think having an empty list as a default arg like that is what you want, you might not be aware but it's behaviour might be surprising.
This link can describe it better than I can: http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments

Please also see inline comments too.
I plan to get some other eyes on this in case I miss something.

review: Needs Fixing

« Back to merge proposal