Juju Charms Collection

Bug #1209026
Comment #21

Comment 21 for bug 1209026

Revision history for this message

Charles Butler (lazypower) wrote on 2014-06-26:

#21

David,

thank you for the updated submission! I'm happy to see the quick turn around on this and see how close it is to being accepted to the charm store. There has been a lot of nice progress here and MapR looks like a brilliant service. With the proper steps outlined I was able to run a terrasort in about 5 minutes flat on AWS.

I ran a thorough audit on the charm, and came up with the following notes:

in the disk mounting script - hooks/lib/findMapRDisks - it makes use of the AWS metadata url. This is an infraction of the recommended quality documentation - as it binds the charm very tightly to AWS - not all cloud providers support this metadata url (see: Azure, HPCloud, Joyent)

After looking through the code, it appears the charm is only fetching the SSH Key and the TimeZone of the AZ from this metadata. Would it be out of the question to normalize the TZ's to UTC, and offer an ssh key to be provided via configuration? This would eliminate the dependency on AWS Metadata.

# Knitpicks
Some recurring behavior I noticed about the hook code was several calls to relation-set without a given relationship id, this was found in the install, config-changed, and start hooks. While its not necessarily a blocker - these should be moved to only be called from within the context of a relationship hook, such as the peer relationship hook.

And the last knitpick I have, is to rename it from maprcharm to mapr - as that reflects the service we are deploying. It will appear nice and concise to users deploying the charm if they can just type in juju deploy mapr - its already a known idiom that this is the mapr-charm.

Aside from the AWS Metadata URL I find no further issues with the charm. This is excellent work for a first revision of having MapR in the Juju charm store. In the interrim - if you'd like to see the MapR charm listed, you can push a branch of your charm to a personal namespace on launchpad

bzr push lp:~username/charms/precise/mapr/trunk

This will provide MapR being listed prior to it becoming a recommended Juju Charm.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com.

David,

I ran a thorough audit on the charm, and came up with the following notes:

# Knitpicks 
Some recurring behavior I noticed about the hook code was several calls to relation-set without a given relationship id, this was found in the install, config-changed, and start hooks. While its not necessarily a blocker - these should be moved to only be called from within the context of a relationship hook, such as the peer relationship hook.

And the last knitpick I have, is to rename it from maprcharm to mapr - as that reflects the service we are deploying. It will appear nice and concise to users deploying the charm if they can just type in juju deploy mapr  - its already a known idiom that this is the mapr-charm.

Aside from the AWS Metadata URL  I find no further issues with the charm. This is excellent work for a first revision of having MapR in the Juju charm store. In the interrim - if you'd like to see the MapR charm listed, you can push a branch of your charm to a personal namespace on launchpad

bzr push lp:~username/charms/precise/mapr/trunk

This will provide MapR being listed prior to it becoming a recommended Juju Charm.

If you have any questions/comments/concerns about the review contact us in #juju on irc.freenode.net or email the mailing list juju@lists.ubuntu.com, or ask a question tagged with "juju" on http://askubuntu.com.