Hello Maarten!

Thanks again for your submission of the Storm charm to the Charm Store. I think it will definitely be a great addition! I have done a review of the charm and here is what I found.

# charm proof

The charm passes `charm proof` without any errors. That is awesome! Let's move on.

# Deployment

I was able to complete the deployment without any errors by following the instructions found on the README file. I did get to the web interface after a little while after it said the unit was started, and I am assuming this is because the service took a bit starting.

Storm seems to be running under the 'storm' user and not root. Awesome!

# Configuration Options

I see that currently the port can not be set to a number fewer than 1024. This is because the service is not running as root, correct? In this case the `setcap` command may be able to help you in order to accept ports lower than 1024. On the other hand, I do not see any verification to check if the port is 1024 or lower. If the user sets port 80, for example, then the service will just continue to display as 'started' with port 80 opened, but the user will not be able to connect. Probably a good idea to add a verification or use setcap to be able to set ports lower than 80.

# Relations

As I mentioned before, I was able to fully deploy the service and access a working UI. But when I did `juju destroy-relation stormmaster:master stormworker:worker` I was not able to access the UI even though `juju status` listed the port opened. It may be a good idea to close the port once the relation is broken if it is expected to have no UI. Also, when I re-added the relation, the UI went back up, but "Supervisor Summary" is now empty, which had one unit listed in there before I destroyed the relation.

I destroyed the relation between stormmaster and zookeeper and the UI went down. When re-added, it came back up, as expected. On the other hand, when I destroyed the relation between stormworker and zookeeper nothing seemed to happen. Is that expected?

# Knitpicks

There are some thingies that I'd like to see for the charm to be even more awesome.

First of all, it would be nice to have consistency in terms of line-wrapping. I suggest having 80-line wrapping in all the README, config.yaml and metadata.yaml files. And on the README files, you should give 4-line intending to get it displayed as code. There is some code that has some extra spaces. Also, the README file is a bit slim. I would suggest doing 'charm add readme' and filling in all the info. There are lots of parts of the template that can just be filled in by copying the info you already have, so you're halfway through that one :)  The README file should be renamed to README.md in order for it to be correctly displayed on the GUI - otherwise, even if it's on Markdown, it will not display correctly. The revision file can be deleted - it's not something that is used now. Finally, on the metadata.yaml file, you set the category as 'misc'. Is there any reason for not having it listed as an application?

Again, thanks a lot for your submission of the charm to the Charm Store. I am really eager to see the charm being promulgated. For now, I am marking this bug as Incomplete, please mark it as New or Fix Committed once you have fixed the issues above. If you need any help make sure to send us an email to the Juju mailing list (<email address hidden>) or find us on #juju on irc.freenode.net - I'm jose there.