Juju Charms Collection

Bug #1431045
Comment #15

Comment 15 for bug 1431045

Revision history for this message

Matt Bruzek (mbruzek) wrote on 2015-05-29: Re: New charm: gpfs

#15

# gpfs charm review

Hello Michael and thank you for the submission of the GPFS charm!

When doing a review, the [Charm Store Policy](https://jujucharms.com/docs/devel/authors-charm-policy) and the [Best Practice for Charm Authors](https://jujucharms.com/docs/devel/authors-charm-best-practice) documents outline what we are looking for. Please refer to these documents if you have questions about the review.

# charm proof
The gpfs charm passes charm proof (our lint tool) with no messages! Sadly, not every charm can claim this. This shows me you have put the time in to clear up the warning and informational messages. Well done!

# icon.svg
Normally we like to avoid letters in the icon, but I recognize that IBM is your brand, and the GPFS product may not have a recognizable pictogram to use for the icon.

# metadata.yaml
This all looks great. I would suggest adding a name for the maintainer before the email. It should be in the format “Name <emailaddress>”. It is my understanding that there is a team of charmers, so allow me to suggest “IBM Juju Support <email address hidden>” or something along those lines. If anyone wanted to email the maintainer they would know it is a team and not someone named “jujusupp”.

# config.yaml
The configuration descriptions are very brief but are OK. The descriptions will show up in the Juju GUI so they often are more verbose, including valid options or more description but these look descriptive enough.

# README.md
Other reviewers have commented on the readme, some of their issues have not been fully addressed. Please revisit comment #11 there are some good points that are not in the configuration section.

The latest readme looks to be correct markdown and converts to HTML correctly. The section on configuration of apache2 is a little vague, perhaps a link to the apache docs or more description here would be nice. The deploy steps look clear and show a relation from manager to client.

The configuration section is very short. Perhaps adding a few sentences about each configuration option, what they do or why someone would want to change them would be nice to have. One could add common configuration examples and how to pass the configuration files on the deploy command.

The contact information section is also brief. This is your area to give links to the software and where to buy the product, where to get more information (docs), and where to go when they are having problems (support). I realize that some of these links are already in the text, but having the links together in the contact section would be a welcome addition.

# hooks
The hooks are written in python and look to be well written. The code is clear and easy to follow, and looks like some significant work went into this charm!

Without the binary I was unable to test the full charm functionality. Please make sure the hooks are idempotent, meaning they can be run multiple times with no errors or different output. Make sure that all configuration options are handled in config-changed (which it appears they are) so they change when and how the user expects. These are generally the things we look for when reviewing.

After contacting Michal some progress has been made on getting a binary for Canonical to use when reviewing this charm but we do not yet have the file. We need the binary to fully evaluate the charm. I am going to put this bug back to “in progress” until we have something for us to test. Please consider the suggestions I have made here and put the bug back in “Fix Committed” when you are ready for another review.

# gpfs charm review

Hello Michael and thank you for the submission of the GPFS charm!

When doing a review, the [Charm Store Policy](https://jujucharms.com/docs/devel/authors-charm-policy) and the [Best Practice for Charm Authors](https://jujucharms.com/docs/devel/authors-charm-best-practice) documents outline what we are looking for.  Please refer to these documents if you have questions about the review.

# charm proof
The gpfs charm passes charm proof (our lint tool) with no messages!  Sadly, not every charm can claim this.  This shows me you have put the time in to clear up the warning and informational messages.  Well done!

# icon.svg
Normally we like to avoid letters in the icon, but I recognize that IBM is your brand, and the GPFS product may not have a recognizable pictogram to use for the icon.

# metadata.yaml
This all looks great.  I would suggest adding a name for the maintainer before the email.  It should be in the format “Name <emailaddress>”.  It is my understanding that there is a team of charmers, so allow me to suggest “IBM Juju Support <jujusupp@us.ibm.com>” or something along those lines.  If anyone wanted to email the maintainer they would know it is a team and not someone named “jujusupp”.

# config.yaml
The configuration descriptions are very brief but are OK.  The descriptions will show up in the Juju GUI so they often are more verbose, including valid options or more description but these look descriptive enough.

The latest readme looks to be correct markdown and converts to HTML correctly.   The section on configuration of apache2 is a little vague, perhaps a link to the apache docs or more description here would be nice.  The deploy steps look clear and show a relation from manager to client.

The configuration section is very short.  Perhaps adding a few sentences about each configuration option, what they do or why someone would want to change them would be nice to have.  One could add common configuration examples and how to pass the configuration files on the deploy command.

The contact information section is also brief.  This is your area to give links to the software and where to buy the product, where to get more information (docs), and where to go when they are having problems (support).  I realize that some of these links are already in the text, but having the links together in the contact section would be a welcome addition.

# hooks
The hooks are written in python and look to be well written.  The code is clear and easy to follow, and looks like some significant work went into this charm!

Without the binary I was unable to test the full charm functionality. Please make sure the hooks are idempotent, meaning they can be run multiple times with no errors or different output. Make sure that all configuration options are handled in config-changed (which it appears they are) so they change when and how the user expects.  These are generally the things we look for when reviewing.

After contacting Michal some progress has been made on getting a binary for Canonical to use when reviewing this charm but we do not yet have the file.  We need the binary to fully evaluate the charm.  I am going to put this bug back to “in progress” until we have something for us to test.  Please consider the suggestions I have made here and put the bug back in “Fix Committed” when you are ready for another review.