Comment 6 for bug 1195736

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