PLUMgrid charm - neutron-api-plumgrid - review required

Greetings Bilal,

Thank you for your contribution to the Juju Charms ecosystem! It's great to see the fruit of the last few weeks of labor getting the charm prepped for submission. We appreciate the effort thats gone into this initial revision, and I've taken some time to review the charm and have the following notes:

The charm has some feedback when issuing `charm proof` - if you'd like to follow along at home, you can execute `charm proof` in the root of your charms code and receive instant feedback on common gotchyas when reviewing charms. Anything above an I: message is considered needs revision. The items I found were pretty minor and should be a quick fix:

W: no copyright file
I: missing recommended hook config-changed
W: config.yaml: option install_keys does not have the keys: description
I: config.yaml: option install_keys has no default value
W: config.yaml: option install_sources does not have the keys: description

For reference, I'm including a link to the Charm Store Policy document that I will be citing as we move forward with the review:

https://jujucharms.com/docs/stable/authors-charm-policy

Charm store policy dictates that all charms (not the service being delivered, just the charm code itself) must be licensed under an OSI approved license: http://opensource.org/osd, once a license has been chosen - ensure you include it in your charm in the `copyright` file.

It looks as if you were trying to make a 'null' default value in the install_keys configuration option. By default, when a value is not provided, the value defaults to a true null character, whereas the string 'null' will evaluate to the string literal 'null'.

Additionally, descriptions are rendered for each of the config.yaml options, and displayed in the store. This helps new users quickly grokk what is required to configure the charm and how it will respond/react to changes when updated.

A great example resource for a config.yaml can be found in the MySQL charm here: https://api.jujucharms.com/charmstore/v4/trusty/mysql-25/archive/config.yaml

Looking through the metadata.yaml, the long description appears to be missing which is rendered out in the charm store. Having a nice and descriptive multi-line paragraph here about the service will go a long way for users searching the charm store for the perfect networking solution. Use this as your first opportunity to grab their attention :)

The hook code is missing a crucial hook as you have options listed in config.yaml - the config-changed hook is primarily reponsible for acting on configuration changes in this file when set by the user. If i were to deploy the charm in its current shape, and misconfigure the service - there is no way for me to recover the service aside from completely tearing it down and standing it back up again. This breaks the users expectation of how juju works, and unfortunately gets a -1 from me.

I see a lot of symlinked hooks that are not implemented in the neutron_plumgrid_hooks.py file - while this isnt necessarily a game-breaking pattern, juju by default will exit 0 when entering a hook context that is not implemented. I feel its worth suggesting to remove the hook symlinks that a...

As a follow up, I didn't make the immediate connection that the yaml fragements were specific to configuring the charmhelpers.fetch method - and the null value is fine, as its implicitly called out and checked for in the charmhelper. Feel free to ignore that blurb!

Thank alot Charles. Will be pushing in a patch very soon.

On Mon, Jun 1, 2015 at 11:22 AM, Charles Butler <
<email address hidden>> wrote:

> As a follow up, I didn't make the immediate connection that the yaml
> fragements were specific to configuring the charmhelpers.fetch method -
> and the null value is fine, as its implicitly called out and checked for
> in the charmhelper. Feel free to ignore that blurb!
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1459559
>
> Title:
> PLUMgrid charm - neutron-plumgrid-plugin - review required
>
> Status in Juju Charms:
> In Progress
>
> Bug description:
> Review is required for neutron-plumgrid-plugin charm
>
> https://code.launchpad.net/~plumgrid-team/charms/trusty/neutron-plumgrid-plugin/trunk
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1459559/+subscriptions
>

--
Bilal Baqar
MTS - PLUMgrid Inc.
<http://bit.ly/1I0EhyF>

Sorry for the delayed response. We are adding support for KILO openstack in the charms therefore were going to push a patch after that is done. That patch will also address each of the points raised in the reviews above. ETA is this week.

Hey guys just to get you on the same page as us, let me share our progress.
The reviews pointed out the following problems

1. Charles pointed out that our charms were not in accordance with the juju charms policy. They are basic things like passing charms through charm proof. I have already made those changes and will be pushing them in the next patch.

2. Cory pointed out that our charms were failing on AWS. We had not been able to test our charms on AWS at the time therefore they failing on AWS. We have made the changes required and will the add them in the next patch.

3. James wants us to change the architecture of the charms. Currently our charms are dependent on each other but aren't subordinates therefore we have to ensure that they get deployed on the same node. We have finalized the architecture after discussions with James. Our discussion can be followed on the neutron-iovisor charm bug and also these drawings (https://docs.google.com/drawings/d/1_QVGe-uG-iioqeAtaCfQ6H4E5DAVQYAgD7rLAEkUTnU , https://docs.google.com/drawings/d/1rnsMMt4BaiL1qB2nZHKKYuzrhlhmBw6Iba5CH4OYn5s ). We are currently implementing the proposed changes which will take some time as the changes are significant.

Hopefully we will be able to update the charms in the next week.

Greetings Bilal,

since the last review there have been significant changes to the neutron-api-plumgrid 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:

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.

when running the linter, i get feedback that the line length is an issue, with this make target being available it should exit cleanly:

hooks/neutron_plumgrid_context.py:3:80: E501 line too long (82 > 79 characters)
hooks/neutron_plumgrid_context.py:99:80: E501 line too long (80 > 79 characters)
hooks/neutron_plumgrid_context.py:113:80: E501 line too long (100 > 79 characters)

The make unit_test target seems to expect some dependencies to be available. This directly affects the road to oil, in that all dependencies should be self-managed in the charms makefile, either through a dependency management tool like tox, or through venv managed dependencies as makefile dependency chains.

I highly recommend the tox or venv approach. Here is a sample of the venv:

http://bazaar.launchpad.net/~charmers/charms/trusty/mysql/trunk/view/head:/Makefile

virtualenv:
 virtualenv .venv
 .venv/bin/pip install flake8 nose coverage mock six pyyaml \
        netifaces netaddr

Then simply mark your makefile targets with the virtualenv dependency such as:

lint: virtualenv
 .venv/bin/flake8 --exclude hooks/charmhelpers,tests/charmhelpers \
        hooks unit_tests tests
 @charm proof

test: virtualenv
 @echo Starting tests...
 @.venv/bin/nosetests --nologcapture --with-coverage unit_tests

the tests/00-setup script is also installing python-requests which appears to be un-used and can safely be removed.

With these minor fixup's 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.

Bilal,

+1 LGTM. Thank you for the work you've put into this new submission for the charm store. Congrats on your achievement! I've promulgated your charm (placed it in the ~charmer recommended namespace) and you can find the bug tracker here: https://bugs.launchpad.net/charms/+source/neutron-api-plumgrid and the project for your charm here: https://code.launchpad.net/~charmers/charms/trusty/neutron-api-plumgrid/trunk

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.