Code review comment for lp:~mistotebe/charms/precise/openerp-server/proper-hooks

mistotebe,

Thank you for the work on the openerp-server charm! This Merge Proposal is an improvement and the charmers appreciate the time and effort put in here.

Thanks for updating the README document! I followed the instructions and was able to deploy openerp based soley on the README. That is excellent work!

README

It would be even better if the README contained more information. Typically we like to see links to the upstream code location, links to documentation and how to file bugs. If you have charm-tools installed run the command: juju charm add readme that will generate a README.ex template that you can use to fill in the missing sections. One section I would highly suggest improving is a description of configuration options (especially the ones you added) and what values would be good to use. I understand the values are described in config.yaml, but the README is the place to describe them too.

I ran our linting tool called charm proof against the charm. There are still some warnings in the charm that we would like to see cleared up:
$ juju charm proof
W: Metadata is missing categories.
W: No icon.svg file.
I: relation openerp has no hooks

W: Metadata is missing categories.
The categories is an easy fix, we have some documenation about categories here: https://juju.ubuntu.com/docs/authors-charm-writing.html#make-some-metadata.yaml

W: No icon.svg file.
You can create an icon for this charm and it will be easier to recognize in the GUI. There are instructions here: https://juju.ubuntu.com/docs/authors-charm-icon.html

I: relation openerp has no hooks
Each relationship should have its own set of hooks. The charm provides an openerp relation but there are no hooks to handle this.

Amulet tests:

Thank you for adding amulet tests for this charm! Repeatable tests are key to make sure our charms are robust and automated tests can be run against our charms.

00test.sh
This tests does not run as root, so you need to add sudo to each of those commands. Also add a -y on apt-get install amulet so it can run automatically. The tests failed to run because of this problem.

Based on the test failure I have to put this Merge Proposal in “Work in Progress”. Please fix the tests, address the other issues. When you are ready move the status back to Needs Review and a charmer will take a look at the changes.

Your changes were very well done and greatly appreciated. Just a few minor changes and it should be ready for the charm store! Thanks again for your efforts I look forward to seeing this charm in the store soon.

If you have questions or need any further information you can find mbruzek on IRC. You can contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>