Juju Charms Collection

Bug #1290636
Comment #3

Comment 3 for bug 1290636

Revision history for this message

Matt Bruzek (mbruzek) wrote on 2014-04-24:

Hi Darryl,

Thanks for your submission of this new charm! I am excited to have a Drupal charm in Juju.

I ran charm proof and saw an Error in the report. Here is the error I saw:

E: config.yaml: type of option password is specified as string, but the type of the default value is NoneType

It looks like you were responding to Chuck's previous review by not providing a default password. This looks like a YAML syntax error, the proper way to default to an empty string is default: "". It looks like your config-changed hook will properly handle the password values of an empty string.

I was able to deploy the drupal charm without any errors in Juju.

Not knowing much about Drupal before this review I was relying on the readme to give me information about the charm. The readme was in markdown format (that is great!) but looked a little sparse. I would highly suggest adding more information to that file. We have a tool to help you with that, run: juju charm create readme and a README.ex file will be created in that directory. That gives a template of what full readme should look like. Specifically include links for Drupal support, mailing lists (https://drupal.org/mailing-lists), bug tracker, and a link to Drupal documentation. Not all charms are available on port 80 of the IP address, so it would be a good idea to include some words about how to access Drupal after the deploy is complete. Being new to Drupal I didn't know the proper URL or the right username to log in with and they were not in the readme file.

Other than those things to add to the readme I believe this charm is outstanding! This drupal charm is an excellent example of writing clean hooks! The hooks are very easy to read and very few lines of code. I am very impressed.

Based on the charm proof error I am going to place the status of this bug as incomplete. When you are ready for an additional review, please set the bug status to "Fix Comitted" or "new" and it will get added back to the review queue.

Thanks again for your submission!

Hi Darryl,

Thanks for your submission of this new charm!  I am excited to have a Drupal charm in Juju.

I ran charm proof and saw an Error in the report. Here is the error I saw:

E: config.yaml: type of option password is specified as string, but the type of the default value is NoneType

It looks like you were responding to Chuck's previous review by not providing a default password.  This looks like a YAML syntax error, the proper way to default to an empty string is default: "".  It looks like your config-changed hook will properly handle the password values of an empty string.

I was able to deploy the drupal charm without any errors in Juju.

Not knowing much about Drupal before this review I was relying on the readme to give me information about the charm. The readme was in markdown format (that is great!) but looked a little sparse.  I would highly suggest adding more information to that file.  We have a tool to help you with that, run:  juju charm create readme and a README.ex file will be created in that directory.  That gives a template of what full readme should look like.  Specifically include links for Drupal support, mailing lists (https://drupal.org/mailing-lists),  bug tracker, and a link to Drupal documentation.  Not all charms are available on port 80 of the IP address, so it would be a good idea to include some words about how to access Drupal after the deploy is complete.  Being new to Drupal I didn't know the proper URL or the right username to log in with and they were not in the readme file.

Other than those things to add to the readme I believe this charm is outstanding!  This drupal charm is an excellent example of writing clean hooks!  The hooks are very easy to read and very few lines of code.  I am very impressed.

Thanks again for your submission!