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

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

Hi Vishwa,

In the future it would be helpful to mention why you made a change different to what was suggested, it removes any need for guessing and/or assuming that there was a misunderstanding.
Also, please state the actual reason for the decision (i.e. x is better because of y, not someone else said to do it this way).

Regarding your question re: issubset, please make sure you read all the comments as I have already answered this. On March 17th I state:
"""
You're missing that possibly_known_ids is a list object and doesn't have the method issubset. You need a set object for that.
The use of issubset was also mentioned in a previous comment.

Aarons suggestion here is also to get rid of the function and just make the call where it's used. With reducing the call down to use issubset this makes sense.
Originally I suggested to have a separate function to ease testing and readability but this has evolved to the point where that's not needed.
"""

I have answered your question in line (sorry for the wall of text).
Please see here: http://paste.ubuntu.com/24226385/ to see an example of changing the test to follow the suggestions I made. Not this test now uncovers an error in the code.
Hint: it's related to the comment about what get_security_groups actually returns.

review: Needs Fixing

« Back to merge proposal