charm needed: distcc

Distcc charm linked for approval.

(lp:~negronjl/charms/oneiric/distcc/trunk)

-Juan

Can we "promulgate" this ?

-Juan

Reviewing

Hey Juan. Nice idea and looks pretty close to being ready despite the long review below. I consider bugs which prevent the charm from working the same on local/ec2/orchestra as blockers, but the other things can be fixed post-promulgate if you are pressed for time as I really like this charm and want to see it in the collection soon!

-- blockers --

[1] install - hostname -f is very unreliable anywhere except EC2. Use 'unit-get private-address'. There are some charm helpers for turning this into an IP if you feel strongly that it should always be turned into an IP, but I'd rather see it just used as it comes from the provider unless distcc can't use hostnames. Also really I think that whole step belongs in config-changed, as the file basically needs to be created and rebuilt from scratch each time configs are changed (see below about idempotency in config-changed, moving this to config-changed would solve it). The peer relation further complicates this, so I think you may need to build that file in two pieces (config/install, and the peer relation) and concatenate it together.

[2] distcc-cluster-relation-joined - Also uses hostname -f and dig. dig, btw, is not included by default, you need dnsutils (its on ec2 images, but not the minibuntu that LXC uses). In this case, you don't need distcc_host anyway, because public-address and private-address are always guaranteed to be part of relations. I'd suggest removing distcc_host entirely from the peer interface.

-- non blocking bugs --

[3] config-changed - Not idempotent. It will append ${EXISTING_DISTCC} over and over to /etc/distcc/hosts every time a config is changed.

[4] distcc-cluster-relation-changed - looks like it has a lot copied/pasted from config-changed. Perhaps the two can be consolidated into one script?

[5] d-c-r-changed - The debug-log will already print out the relation settings whenever they are changed, so you can get rid of the extra relation-get / juju-log at the bottom.

[6] d-c-r-changed/joined - MY_PORT is static, should be looked up with config-get

[7] config-changed - Line 11 is

DISTCC_OPTION=...

but line 19 uses DISTCC_OPTIONS, so this will most likely fail because of the set -u if EXISTING_DISTCC != auto.

[8] - README - this just looks super generic. I'd like to see an example of how to use it with juju. For instance, its not clear why the peer relation builds up /etc/distcc/hosts. Is the intention that any of the boxes can be used for a massively scalable distcc build (I assume yes, but README would be the place to assert that)

-- opinions --

[9] install/config-changed - I've gone back and forth on sed vs. building the file. I think you'll be better off building /etc/default/distcc completely using heredocs/appending, rather than trying to sed it into submission. Having a .d style directory which you then cat x.d/* into the file makes the most sense, IMO, for doing this in stages.

[10] - README would be nice to have this text wrapped or something. Its unreadable when you cat it in a terminal. I use 'fmt' to format most text files.

Thanks for the review Clint. I'll address those points.

-Juan

I committed all of the fixes/suggestions/etc. that Clint suggested.

I need another review :)

-Juan

Nicely done Juan!! +1, please promulgate!

Promulgated: lp:/charms/distcc
Thanks Clint
-Juan

This bug is in the review queue because the author was the last to comment. A non author comment removes this Fix Released bug from the review queue.