PLUMgrid charm - neutron-api-plumgrid - review required
Bug #1459559 reported by
Bilal Baqar
This bug affects 1 person
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
Juju Charms Collection |
Fix Released
|
Undecided
|
Unassigned |
Bug Description
Review is required for neutron-
https:/
Related branches
lp://staging/~plumgrid-team/charms/trusty/neutron-api-plumgrid/trunk
- Review Queue (community): Needs Fixing (automated testing)
- Charles Butler: Pending requested
-
Diff: 10336 lines (+4626/-3509)58 files modifiedMakefile (+1/-1)
bin/charm_helpers_sync.py (+253/-0)
charm-helpers-sync.yaml (+6/-1)
config.yaml (+30/-2)
hooks/charmhelpers/contrib/amulet/deployment.py (+4/-2)
hooks/charmhelpers/contrib/amulet/utils.py (+382/-86)
hooks/charmhelpers/contrib/ansible/__init__.py (+0/-254)
hooks/charmhelpers/contrib/benchmark/__init__.py (+0/-126)
hooks/charmhelpers/contrib/charmhelpers/__init__.py (+0/-208)
hooks/charmhelpers/contrib/charmsupport/__init__.py (+0/-15)
hooks/charmhelpers/contrib/charmsupport/nrpe.py (+0/-360)
hooks/charmhelpers/contrib/charmsupport/volumes.py (+0/-175)
hooks/charmhelpers/contrib/database/mysql.py (+0/-412)
hooks/charmhelpers/contrib/network/ip.py (+55/-23)
hooks/charmhelpers/contrib/network/ovs/__init__.py (+6/-2)
hooks/charmhelpers/contrib/network/ufw.py (+5/-6)
hooks/charmhelpers/contrib/openstack/amulet/deployment.py (+135/-14)
hooks/charmhelpers/contrib/openstack/amulet/utils.py (+421/-13)
hooks/charmhelpers/contrib/openstack/context.py (+318/-79)
hooks/charmhelpers/contrib/openstack/ip.py (+35/-7)
hooks/charmhelpers/contrib/openstack/neutron.py (+64/-23)
hooks/charmhelpers/contrib/openstack/templating.py (+30/-2)
hooks/charmhelpers/contrib/openstack/utils.py (+939/-70)
hooks/charmhelpers/contrib/peerstorage/__init__.py (+0/-268)
hooks/charmhelpers/contrib/python/packages.py (+35/-11)
hooks/charmhelpers/contrib/saltstack/__init__.py (+0/-118)
hooks/charmhelpers/contrib/ssl/__init__.py (+0/-94)
hooks/charmhelpers/contrib/ssl/service.py (+0/-279)
hooks/charmhelpers/contrib/storage/linux/ceph.py (+823/-61)
hooks/charmhelpers/contrib/storage/linux/loopback.py (+10/-0)
hooks/charmhelpers/contrib/storage/linux/utils.py (+8/-7)
hooks/charmhelpers/contrib/templating/__init__.py (+0/-15)
hooks/charmhelpers/contrib/templating/contexts.py (+0/-139)
hooks/charmhelpers/contrib/templating/jinja.py (+0/-39)
hooks/charmhelpers/contrib/templating/pyformat.py (+0/-29)
hooks/charmhelpers/contrib/unison/__init__.py (+0/-313)
hooks/charmhelpers/core/hookenv.py (+220/-13)
hooks/charmhelpers/core/host.py (+298/-75)
hooks/charmhelpers/core/hugepage.py (+71/-0)
hooks/charmhelpers/core/kernel.py (+68/-0)
hooks/charmhelpers/core/services/helpers.py (+30/-5)
hooks/charmhelpers/core/strutils.py (+30/-0)
hooks/charmhelpers/core/templating.py (+21/-8)
hooks/charmhelpers/core/unitdata.py (+61/-17)
hooks/charmhelpers/fetch/__init__.py (+18/-2)
hooks/charmhelpers/fetch/archiveurl.py (+1/-1)
hooks/charmhelpers/fetch/bzrurl.py (+22/-32)
hooks/charmhelpers/fetch/giturl.py (+20/-23)
hooks/neutron_plumgrid_context.py (+59/-46)
hooks/neutron_plumgrid_hooks.py (+20/-1)
hooks/neutron_plumgrid_utils.py (+61/-11)
metadata.yaml (+7/-0)
templates/kilo/pgrc (+8/-0)
templates/kilo/plumgrid.ini (+22/-0)
templates/kilo/plumlib.ini (+5/-2)
unit_tests/test_neutron_plumgrid_plugin_context.py (+20/-18)
unit_tests/test_neutron_plumgrid_plugin_hooks.py (+1/-0)
unit_tests/test_neutron_plumgrid_plugin_utils.py (+3/-1)
description: | updated |
description: | updated |
summary: |
- PLUMgrid charm - neutron-plumgrid-plugin - review required + PLUMgrid charm - neutron-api-plumgrid - review required |
Changed in charms: | |
status: | In Progress → Fix Committed |
Changed in charms: | |
status: | In Progress → Fix Committed |
To post a comment you must log in.
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...