Comment 7 for bug 1459568

Revision history for this message
Charles Butler (lazypower) wrote :

Greetings Bilal,

since the last review there have been significant changes to the plumgrid-director charm, and I'm pleased to review this charm again to see the improvements.

I do have some minor nitpicks that surfaced during the review and should be final wrap up fixes for this charm before acceptance:

It appears that when updating the README, `charm add readme` was used, and the .ex file was left. This triggers a warning in `charm proof`

W: Includes template README.ex file
W: README.ex includes line 6 of boilerplate README.ex

The README.ex file should be renamed to README.md to resolve when running charm proof. Additionally there is a single line in the README that is getting tripped up on `charm proof` - Step by step instructions on using the charm: I have proposed a merge against charm-tools to address this output making it much easier for a developer to know what has failed the linting routine here: https://code.launchpad.net/~lazypower/charm-tools/refactor-readme-lint/+merge/266784 so future issues coming from charm proof wrt the readme should be more helpful.

The lint target fails with the following output:

ubuntu@7bd884bf30f0:~/trusty/plumgrid-director$ make lint
hooks/pg_dir_context.py:3:80: E501 line too long (82 > 79 characters)
hooks/pg_dir_context.py:24:80: E501 line too long (86 > 79 characters)
hooks/pg_dir_context.py:61:80: E501 line too long (80 > 79 characters)
hooks/pg_dir_context.py:69:80: E501 line too long (119 > 79 characters)
hooks/pg_dir_utils.py:140:80: E501 line too long (88 > 79 characters)
hooks/pg_dir_utils.py:235:80: E501 line too long (105 > 79 characters)
hooks/pg_dir_utils.py:242:80: E501 line too long (84 > 79 characters)
make: *** [lint] Error 1

When executing the plumgrid director tests, the test runner failed execution due to the test not being able to locate 'files/director.yaml' which appears to be related to how the test is assuming it is run. as Cory had suggested you should probably reference the file using: os.path.join(os.path.dirname(__file__), 'files/plumgrid-director.yaml')

With these minor fixes I would have no problem approving the charm. Thanks so much for the refactoring and vigilance during the review process. I'm going to change status of this bug to "in progress" and when you're ready switch merge status to 'fix committed' and someone will be along shortly to review your work.

 If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.