PLUMgrid charm - plumgrid-director - review required

[Bilal Baqar]

Review is required for plumgrid-director charm
https://code.launchpad.net/~plumgrid-team/charms/trusty/plumgrid-director/trunk

[Cory Johns (johnsca)]

Greetings Bilal,

As Charles mentioned on your neutron-iovisor review, it's great to get your contribution to the ecosystem! Overall, this charm looks like a great contribution to Open Stack, though I did run into a couple of issues, as noted below:

* `charm proof` passes with no warnings or errors. Excellent!

* The README is pretty light on details. As Charles suggested in his review, using `charm add readme` will give you a good template to start filling in more detail here.

* `make lint` fails with a quite a few errors. They're mostly "line too long" errors, but there are a few others in there as well.

* Running the tests using bundletester fails with a path error: http://pastebin.ubuntu.com/11567699/ It appears this is due to running the tests from the root charm folder, and should probably reference the file using: os.path.join(os.path.dirname(__file__), 'files/plumgrid-director.yaml')

* Also, as Charles mentioned in his review, symlinking hooks that are not actually handled can be a bit confusing, so it might be nice to remove the unimplemented ones

* When deploying the charm on AWS, following the instructions in the README, the director charm deploys fine, but I got an install error on neutron-iovisor. I've noted the error on the review for that charm, but to continue testing of this charm, I `juju resolved` it.

* The plumgrid-plugin-relation-joined hook failed, and I found the following two sets of lines in the log file that seem like they might be related: http://pastebin.ubuntu.com/11569256/ and http://pastebin.ubuntu.com/11569253/ Of course, it's possible that it might be due to the error in the neutron-iovisor charm and me forcing it with `juju resolved`

[Bilal Baqar (bbaqar)]

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.

[amir sanjar (asanjar)]

hi Biala,
Has KILO openstack support patch been added to the charm yet? Let us know when your charm is ready for review again, thanks

[Bilal Baqar (bbaqar)]

Haven't added it yet. Will change bug status once I do.
On Jul 9, 2015 7:35 PM, "amir sanjar" <email address hidden> wrote:

> hi Biala,
> Has KILO openstack support patch been added to the charm yet? Let us know
> when your charm is ready for review again, thanks
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1459568
>
> Title:
> PLUMgrid charm - plumgrid-director - review required
>
> Status in Juju Charms:
> New
>
> Bug description:
> Review is required for plumgrid-director charm
>
> https://code.launchpad.net/~plumgrid-team/charms/trusty/plumgrid-director/trunk
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1459568/+subscriptions
>

[Marco Ceppi (marcoceppi)]

I've placed this as "Incomplete", when ready please move this either back to New or "Fix Committed" to have it placed back in the review queue. If you're unable to change the bug status, make sure you join https://launchpad.net/~charm-contributors

[Bilal Baqar (bbaqar)]

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.

[Charles Butler (lazypower)]

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.

[Charles Butler (lazypower)]

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/plumgrid-director and the project for your charm here: https://code.launchpad.net/~charmers/charms/trusty/plumgrid-director/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.