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 that's 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:

I: missing recommended hook start
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


By updating the config.yaml options with a default description, this will clear up those W's.

Additionally it appears you wanted a Null terminated value for the option install_keys' - the null statement left there will render a stringified 'null' when being read by Juju. Simply omitting the value will give you a true null if this was the intention.

The README is pretty sparse with details, and I would like to see it updated with all the keys available from 'charm-tools' command 'charm add readme' - which will render a README.ex file, with all the headers and pointers to expected information.

I see a lot of symlinked hooks that are not implemented in the neutron_iovisor_hooks.py file - while this isn't 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 are not in use so its very clear to any maintainers that follow you which hooks are implemented.

I like the separation of concerns you have made with the neutron_iovisor_utils.py, which is handling the lower level system operations, and the hooks remain high level execution statements. +1 on this pattern and workflow.

I also see the beginnings of unit tests, and functional tests being implemented in this release. This is an excellent start in giving us the insight as to the status of the charm as we move it forward towards the charm store. +1 on this effort, and thank you for the due dilligence in this effort! I know getting started testing was a bit of a pain point, and I encourage you to submit any feedback you have about the experience to the mailing list <email address hidden>

It appears that the primary directive of this charm is to install and load the iovisor kernel module. MTU data is being transmitted on the joined hook - but if the relationship is ever broken, nothing happens to signify that the plugin should reconfigure itself into an 'unconfigured' state - is this intentional behavior?

Thanks again for the submission. I'm going to change status of this bug to "In Progress" and when you're ready for another review, simply change the 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.