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

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

A couple of times in discussion I'm stated that we'll concentrate only on AWS for this MP and then do the other providers in follow up MPs.
This MP alters more than just the AWSAccount class, a couple of issues with this:

  - This makes the MP even longer. The diff is 548 lines long, this is too long to comfortably review. As mentioned multiple times before we want to have short MPs (less than 300 lines) this makes reviews quicker, easier and less prone to errors.

  - There are no tests included for the altered *Account classes. There is no assurance that this branch hasn't broken anything.

  - Due to the first points, adding the needed tests would blow the diff count way out.

make lint fails.

Please notice the comments about methods that could be made functions, having functions makes testing a lot easier (no need to create or mock state).

review: Needs Fixing

« Back to merge proposal