Buildbot charms

[Gary Poster]

We have a buildbot master charm and a buildbot slave charm that we would like reviewed.

The slave wants to share code with the master. We have done that right now with symlinks, which we know are not accepted. We intend to change this to whatever you suggest, as well as respond to other issues you raise.

It might also be worth noting that Graham Binn's email to the <email address hidden> titled "Juju feedback from the Launchpad Yellow Squad" list raises issues we discovered when writing this charm: https://lists.ubuntu.com/archives/juju/2012-February/001271.html .

If writing a charm were our top priority for the current project, we'd rewrite it with what we know now. As it is, we hope to revise it per your review, and learn from this for next charms.

Thank you

[Mark Mims (mark-mims)]

Thanks for the charm!
A static review follows:

---
# things to fix:

- `charm proof buildbot-master/` shows 'E: revision file in root of charm is required'

- juju-wrapper is a great idea... it shouldn't live in the charm itself though (pls take a look at lp:juju-jitsu).

- cross-charm symlinks've just gotta go. I understand the need, but this should be either in a charm-tools helper package or, in this case, a submodule. As a submodule, the hooks would just appear replicated in each charm. Perhaps a structure where hooks/a-hook is just a symlink to lib/my-special-hooks/a-hook, where lib/my-special-hooks is a submodule within each charm. Until the charm store supports submodules or the equivalent, please just copy them and maintain in parallel. (please see the recommendation below to create a single buildbot charm).

- please move HACKING.txt into README.txt as that's automatically displayed in various places (charm browser).

---
# discussion / recommendations / opinions (no need to fix and in no particular order):

- Please consider combining buildbot-master and buildbot-slave into a single charm. It can then be deployed into different roles based on the service names. See lp:charms/mysql for an example, but maybe something like:

    juju deploy --repository . local:buildbot bbmaster
    juju deploy --repository . --num-units 20 local:buildbot bbslave
    juju add-relation bbmaster bbslave:master ( which I'd probably change to bbmaster:slave bbslave)

Note that this would remove the need for cross-charm symlinks discussed above!

- whoohoo! I love the work you did in helpers... please please please extract this out to charm-helpers-python in lp:charm-tools so others can use this too!

- Prefer to keep only hook executables (and links to hook executables) in $CHARM_DIR/hooks. Perhaps helpers.py, local.py, etc could go in $CHARM_DIR/lib? master.cfg could go in $CHARM_DIR/etc? etc... totally just a style thing tho.

- Wow, thanks for tests! We'll probably have some updates to this soon as automated execution of charm tests found in $CHARM_DIR/tests are in spec and in progress.

- I like $CHARM_DIR/examples... that's totally going in best practices.

- love the config serializer... please add to charm-helpers-python in lp:charm-tools!

- please implement an upgrade-charm hook... it's often just a symlink to the install hook, but you'd have to watch that the install hook is (and remains) idempotent and preserves working data.

---

I have some questions about the buildbot master/slave relation... I'm trying to understand why you chose to specify script_xxx params in config -vs- in relation data. I'll talk to gary in #juju tomorrow and then do a dynamic review based on test runs.

Thanks... great work! We'll be adding several things you did to helpers and charming best-practices.

[Mark Mims (mark-mims)]

Ok, I'm good with spin-up.

I'm setting the bug status back to "In Progress"... please hit the "things to fix" section of the previous comment, then set this back to "Fix Committed", and I'll promulgate.

Note that if there will still be lots of active development we might want to delay a bit on promulgation as that changes ownership to lp:~charmers and'll need merge proposals for changes.

Thanks gang... awesome work!

[Gary Poster (gary)]

Thank you very much, Mark.

We addressed all of your "must fix" items.

- We added revision files.

- We removed juju-wrapper. We were tempted to replace it with instructions on what to do instead, but juju-jitsu's test tools seem destined to be replaced with Clint's new test approach and the environmental variable options, so we didn't pursue it at this time.

- We have removed the symlinks. Currently we have copied the files, and created a python-shell-toolbox package that we depend on (for this and another need). https://launchpad.net/python-shell-toolbox . We also have made a change to charm tools to add some juju-specific helpers. This MP is ready: https://code.launchpad.net/~gmb/charm-tools/add-charm-helpers/+merge/96204

- We moved HACKING into README.

We acted on some of your suggestions, and discussed them all.

- We talked a lot about combining slave and master into a single charm, and most of us investigated the mysql charm a bit. We decided that it did not fit our needs. We had two points in particular.
   * There was not a lot of overlap between the master and slave beyond what we had already abstracted.
   * As announced on the juju list earlier, we had decided that non-install-time configuration was often an unnecessary expense and complication, requiring much more testing and care. Flexibly deciding to go one way or the other went against that shared opinion.

- We did not move non-hook files out of hooks. We might have done so, but we had a few questions on how we might do it with a few files, and just have not gotten around to it, as a low priority.

- We implemented an upgrade-charm hook.

- As noted, we have abstracted our helpers, one into a standalone python shell package and one into the charm helpers MP.

Thanks again! I'll move this to fix committed as soon as we land two more branches waiting in the wings....

[Gary Poster (gary)]

Actually, no more branches are needed. Thanks!

[Gary Poster (gary)]

Also, we are fine with promulgation: we do MPs anyway, and we can deploy from our own branches if we need to do something in a hurry.

[Mark Mims (mark-mims)]

Promulgate away please.

Thanks for the great work!

[Mark Mims (mark-mims)]

or rather... I'll promulgate right away.