Comment 2 for bug 1399228

Revision history for this message
Tim Van Steenburgh (tvansteenburgh) wrote :

Hi saurabh, thanks for submitting your charm for review! I've added a Related Branch to this bug, in which I've included some suggested changes, which I'll explain below:

1. Added amulet tests (see tests/ dir). Tests are required for admission to the trusty charm store. I added a basic test -- you are welcome to add more (for example, a test that logs in to the admin console). You can find more info on amulet at https://juju.ubuntu.com/docs/tools-amulet.html

2. Added apt-get update before apt-get install in hooks/install. Ensures package lists are up-to-date before attempting installation.

3. Cleaned up README (formatting/typos).

4. -relation-changed hooks:
   - Use $CHARM_DIR to get path to templates
   - Replace sleep with a conditional that exits until data is available. Read more about this technique here: https://juju.ubuntu.com/docs/authors-relations-in-depth.html

I also notice that the charm seems to support a website relation, but there is no info in the README on how to use it -- that would be good to add.

I did not look closely at the slave charm, but you may wish to see which of these improvements would also apply there.

Great start on this charm! If you have any questions, please feel free to ping me (tvansteenburgh) in #juju on irc.freenode.net, or send an email to the mailing list at <email address hidden>.