Comment 3 for bug 1030953

Revision history for this message
Antonio Rosales (arosales) wrote :

# I am providing a initial +1 review. A ~charmer will give a final review and merge this charm for inclusion in the Charm Store if they don't have any additional feedback.

## Review points:
- Charm passes charm proof
- Description in metadata.yaml
- Maintainer field set in metadata.yaml
-Copyright file included with GPL-3 license
- Successfully deploys in EC2 with default config.
- apt-get deb from upstream, pagekite.net (with apt-key)
- No tests. I talked with Jose about this in IRC and he is looking into Amulet. While our policy doesn't state tests must be included there is a current proposal out to include test[0]

### Knit pics
- Readme could have an explict "cookbook" example of how a user could execute each command and get a working deployment. For example, in addition to having the [charmname] example you could specifically mention a charm so a user could programmatically follow each step.
- The metadata.yaml could also mention what the charm specifically does in addition to the service description.

## Summary
- Initial review looks good and is ready for an in-depth review review by the ~charmers team.
- A plus would be to have tests and a "cook book" example in the Readme.
- Good first charm submission.

[0] https://lists.ubuntu.com/archives/juju/2013-December/003330.html

-thanks,
Antonio