Charm Needed: Review Board

[Jorge Castro]

We should have RB, it's sexy:

http://www.reviewboard.org/

[Adam Collard (adam-collard)]

lp:~mars/charms/precise/reviewboard has something for this

[Marco Ceppi (marcoceppi)]

Hi David! Thank you for another submission to the charm store! I've started reviewing your charm now and will reply back after I've done a first pass.

[Marco Ceppi (marcoceppi)]

Hi David, thanks again for this submission! My first review is below:

# Proof

I ran `juju charm proof` from the charm-tools package and got the following results:

I: relation website has no hooks

Now, this is informational, because not all relations need a hook. However, in the case of the http interface the minimum needed is a `website-relation-joined` with the following contents:

```
#!/bin/bash

relation-set hostname=`unit-get private-address` port=80
```

That way services which ingest the http interface can know where to map the port and hostname.

# Blockers

You currently have a default value for password. Per the charm store policy we don't like having default passwords. At the end of the day it opens an attack vector for all deployed services using Juju. Please set the default to an empty string "", have the config-changed hook not expose/setup the service until the user sets the password (unless it's not needed for setup), then make sure to document setting the password in the README as part of the standard service setup.

You're assigning copyright to Canonical, when I think you meant to assign copyright (and ownship of the charm's code) to yourself. If that's the case, please make sure to update the copyright file header :)

You're README does a good job outlining the usage of the charm, deploying, exposing, adding relations as well as a "preferred" recommended setup. We'd like if you expanded this to include the configuration option descriptions, examples on how to scale out the service, etc.

Your upgrade-charm won't actually work. hooks are executed from the $CHARM_DIR, so they need to be prefixed with hooks/ (ie: hooks/install, hooks/config-changed).

# Knitpicks

I can't actually find any at this time. Your db-relation-broken and cache-relation-broken are a little worrisome being symlinked to the changed relation event respectfully. However, the code looks sane in handling those cases.

Just want to thank you again for another awesome submission to the charm store! Like with the other requests I've moved this to incomplete pending resolution of the above blockers. Once you're ready for another review change the bug status to either New or Fix Committed to have it queued for another look.

As always, if you have any questions please feel free to ask them here, find us in #juju on freenode.net, mail the list <email address hidden>, or ask a question tagged with "juju" on http://askubuntu.com

[Adam Collard (adam-collard)]

Hi Charmers!

I opted to start from scratch on charming Review Board.

Could you please review lp:~adam-collard/charms/precise/reviewboard/trunk for inclusion in the Charm Store?

Thanks!

[Cory Johns (johnsca)]

Adam,

Thank you for your submission for the ReviewBoard charm. It looks really good, and I'm excited about having ReviewBoard in the store, since it's a great code review tool.

I was able to deploy your charm without issue by following the README, which is complete and informative. Charm proof also passes without warnings or errors. The hooks code is nice and clean and handles idempotency issues well.

The one issue that I see with the charm is that, although documented, having config options that don't respect post-deployment changes is a big issue, since it prevents the user from being able to admin the software via the standard charm interfaces. I understand the difficulty with ReviewBoard not exposing great interfaces for managing the admin password and host name, but I think it would be worth finding a way to support the changes. For example, you could possibly use `rb-site manage /var/www/reviewboard changepassword admin` to update the admin password. Setting the host name, though, may require issuing a SQL command to postgresql, so it might be easier to implement both in that fashion.

Thanks again for the submission. I'm going to change status of this bug to "incomplete" and when you're ready for another review please change the status of the bug to "fix submitted" or "new" and someone will be along shortly to review your changes.

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>

[Cory Johns (johnsca)]

Adam,

Thanks for the quick turn-around. Your changes address the config issue and work as expected, and everything else on the charm looks great.

So this is my +1 and a charmer should be along shortly to make it official.

[Charles Butler (lazypower)]

Adam,

Great work on this submission! This is a really good first cut of the reviewboard charm. I'm looking forward to the future iterations of the service, as I see a really good road map outlined in the README

I see no blocking issues with the charm itself that would prevent promulgation. I'm going to promote this charm to the store and it should show up within the next 15 to 30 minutes.

You can find the issue tracker here: https://bugs.launchpad.net/charms/+source/reviewboard

Welcome to the charm store! Thanks again for the excellent work.

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