Code review comment for lp:~ankatare/juju-ci-tools/juju-aws-add-credential

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

Some overall comments:
  - run "make lint" often, it will catch any of the smaller errors that might creep in (not there is a couple issues with this branch it will catch).

  - Please don't delete your branch and re-propose unless it's really needed (in this case we lose the previous comments from the original PR).

  - Tests are needed for the methods added. In most cases you'll have more joy if you write the tests first, among other things it helps you layout the structure of the code before you start implementing things.

I've had to repeat a couple of comments here, please if you disagree with a comment or have a question about it please comment why that is so we can have a discussion etc. (another reason not to delete the merge proposal).

Please see inline comments regarding code specifics.

review: Needs Fixing

« Back to merge proposal