Hi Marco,

I made the updates. Storm is unfortunately not ready for prime-time yet,
since the supervisors/workers seem to stop working when something happens
with Nimbus and I was not able to validate DRPC works. However there is
good news. I found a storm expert, whose book I am currently reviewing to
take ownership and bring the storm charm up to standard and use it for his
book/blog. I will write him an email with you in copy. Not sure if you need
to do something more before he can start working on it. As soon as he says
he is ready, we can do the transfer and he becomes the owner. Ok?

thanks,
Maarten

Thanks,
Maarten Ectors
Senior Cloud Strategist
Changing the Future of Cloud
Ubuntu <http://ubuntu.com> / Canonical <http://canonical.com> UK LTD
<email address hidden>
Fixed: +44 (0) 207 630 2435
Mobile: +44 (0) 791 860 8145


On Mon, Jul 8, 2013 at 4:09 PM, Marco Ceppi <email address hidden> wrote:

> Hi Maarten, sorry for the delay in getting this reviewed. Great work on
> the charm so far, I didn't find much in my review. You can find my notes
> below:
>
> # Blockers
>
> These are items that are actively preventing the charm from making it's
> way in to the store.
>
> ## hooks/install
>
> - L62: You download zeromq from source, but don't perform any
> cryptographic verification of the payload[1]. It looks as though they
> provide SHA1SUMS for their downloads so you'll just need to add the
> extra step of verifying the tarbal matches the SHA1 hash
>
> - L86-L88: These lines breaks idempotency. If the install hook were to
> be run again (say it failed half way though and someone ran `juju
> resolved --retry`) there would be no $CHARM_DIR/hooks/storm.yaml file
> anymore. Consider instead using the install command or just copying it
> to it's destination. Also, you'll need to check to insure the group and
> user doesn't already exist. If it does groupadd will exit > 0 and so
> will useradd.
>
> - There are inconsistencies with restart_storm, start_storm, and
> stop_storm. There's a condition where coworker-relation could fire
> before master-relation was met, meaning restart_storm would be called
> before service_ready nullifying the effects of start_storm.
> restart_storm could probably just call stop_storm and start_storm
> respectively instead of directly calling service commands. I may be
> reading the hook wrong though, so if that's not the cause carry on.
>
> # Minor
>
> Not blockers, just things to consider. Not required for charm store.
>
> - I'd consider not having the "install" hook being the central symlinked
> file. It gives the wrong impression when looking at the directory tree.
> Typically people place a "hooks" (hooks.py, hooks.sh) file in the hooks
> directory and symlink all hooks to that file. I'd go one step further
> and say create a $CHARM_DIR/lib/hooks.sh (or common.sh) outside the
> hooks directory. Then instead of symlinks stub the calls in each hook
> file instead. So instead of a symlink of hooks/install -> lib/hooks.sh
> have the install hook include the common file and stub the commands
> there (remove the case call from common file):
>
> ```
> #!/bin/bash
>
> . lib/hooks.sh
>
> configure_hosts
> install_base_packages
> configure_storm_base
> ```
>
> This is, again just a suggestion, but it's been considered to help
> improve charm readability.
>
> - Typically you'll want to avoid placing anything other than hooks in
> the hooks directory. The files storm.sh and storm.yaml are just files
> that are copied to the units, you may want to instead place them under a
> files directory in the $CHARM_DIR or somewhere other than hooks.
>
> - The description for the charm is quite large and replicates a lot of
> what's in the README. I'd consider removing the code examples and
> instead making sure that information is in the README. The description
> is used to give an overview of what the service is and what the charm
> does. The REAME should be where you drill down in to use cases, etc
>
> # Nitpicks
>
> Seriously, these are just me splitting hairs but I feel compelled to
> mention anyways. You're by no means required or suggested to implement
> any of these fixes.
>
> There are some odd formatting issues, indents not being intented to
> match braces (L43-L89), lots of extra white space, etc. Nothing
> actionable, just feel the need to mention it.
>
> --
>
> >From what I can tell the rest of the charm looks great! This will be a
> fantastic addition to the charm store, thanks again for your work on the
> charm. When you've addressed the above major blockers move the bug's
> status to New and it'll be entered in the review queue again for another
> round of reviews. If you have any questions feel free to reply to this
> mail, ask on the juju mailing (<email address hidden>), or in freenode at
> #juju
>
> ** Changed in: charms
>        Status: New => Incomplete
>
> --
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1195736
>
> Title:
>   Storm Charm was added.
>
> Status in Juju Charms:
>   Incomplete
>
> Bug description:
>   The Storm, i.e. the distributed real-time communication system that
>   originated at Twitter, was submitted.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/charms/+bug/1195736/+subscriptions
>