Gitolite charm

[Chris Hardee]

lp:~shazzner/charm/oneiric/gitolite/trunk

Initial working charm for Gitolite, has limited functionality at the moment but can create repositories and administer users.

Provides a server interface: gitolite

[Nick Barcet (nijaba)]

Thanks a lot for this new charm, it's a great idea!

I am not an "official" reviewer, but here are a few remarks from my point of view:

[1] shared fs backend:
due to the ephemeral nature of instances, it might make sense to propose to use a shared file systems where the git repository are stored. I would suggest, to use a ceph or nfs relation to share the files (charms already exist for both). This would also allow to use "add-unit" to increase the number of unit while publishing the same repositority, which is the whole point of juju, IMHO. It also would prevent from loosing all the repo if the instance this is running on was to die.

[2] readme:
It is a good practice (not a requirement) to place a readme file in the charm's directory to explain how to use your charm, give caveats, examples etc...

[3] chmod 777 "$KEY_FILE"
are you sure you want to make your key read/write/execute to the world? 660 would seem more appropriate.

[4] config-changed hook
it would be nice to implement config-changed event to allow for a change of the keys

[5] start/stop hook
need to be implemented. A simple 'service <service-name> [restart|stop] is generally enough.

Thanks again for your work. This is only my 2 cents review as a community charmer.

[Chris Hardee (shazzner)]

Hey Nick, thanks for your feedback that's exactly what I was looking for. The shared fs backend was something I hadn't thought about and makes a lot of sense. I'll continue to work on this, thanks! :)

[Mark Mims (mark-mims)]

please update this bug status when you want another round of review

[Jorge Castro (jorge)]

http://gitlabhq.com/

This might be interesting as an option (not sure if it should be a separate charm or not though)

[Chris Hardee (shazzner)]

Thanks Jorge, I need to get back and finish this.

[Jorge Castro (jorge)]

Nice! When you need a review just tack 'new-charm' as a tag on this bug, thanks!

[Jorge Castro (jorge)]

Oh whoops, I see you did that already. Do you need a review on this now?

[Chris Hardee (shazzner)]

Not yet! Need to finish some items, it should be ready to review before Friday though. Thanks!

[Chris Hardee (shazzner)]

Finished most items, Gitlab will be moved to a separate charm since gitolite doesn't really exist independently from it. I'd like to add theme configuration and some additional Gitweb options in the future.

[Jorge Castro (jorge)]

Marking committed so we can get this in review, thanks shazzner!

[Clint Byrum (clint-fewbar)]

Hi shazzner. This is almost ready for promulgation. The only blocker is that the 'provides' listed in the metadata.yaml do not actually provide anything. At the very least, the website/http relation needs to have a 'website-relation-joined' hook that does

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

The server side is less important since I don't see any 'requires' in the charm store for gitolite.

Fix those relations so they perform some function, or remove them from metadata.yaml.

Another blocker, you have a predictable file creation in /tmp, which is a security problem since a user could create a symlink to a file which they can then cause you to overwrite. Its super minor, but a well known problem and could cause issues later:

KEY_NAME=`config-get pub-key-name`
KEY_FILE="/tmp/$KEY_NAME.pub"

juju-log "Created: $KEY_FILE"
PUB_KEY=`config-get pub-key`
echo "$PUB_KEY" >> "$KEY_FILE"

This might be ok since its only during install, but it would be MUCH better and simple to use mktemp to create a random filename:

KEY_FILE=`mktemp /tmp/$KEY_NAME.pub.XXXXXXXX`

Some other minors (fix these at your leisure, they are not blockers for the charm store):

* indent the gitweb sed's at the end of install

* consider moving almost all of the install bits that use config-get to a config-changed hook. This will allow the service to be changed after initial deploy, and shouldn't require any logic changes.

* Perhaps also consider locking SSH down by disabling password logins. Since you'll be exposing port 22.. its important to make sure the brute forcers never succeed.

[Chris Hardee (shazzner)]

Updated the Gitolite charm with Clints suggestions, thanks for you feedback :)

[Clint Byrum (clint-fewbar)]

Hi shazzner. Thanks for the updates.

Unfortunately, this fails to deploy on precise, which is the new target for charms (I did not test on oneiric):

2012-04-19 06:05:58,554: hook.output@ERROR: /tmp/gitolite.pub.7RUWP4i17I must end in .pub

I think thats just a matter of changing the mktemp to be gitolite.XXXXXXXX.pub.

There are a few more minors:

* 'hostname -f' is not reliable, and should not be used. I'll go ahead and fix since its such a minor thing. In the future, use 'unit-get private-address'.

* You've duplicated the config-changed logic in install. config-changed is always run directly after install. So there should never be a reason to do that.

* install is not idempotent. It will try to add the gitolite user and group again. This is important if there is a transient error and you need to use 'juju resolved gitolite/0 --retry' You can fix that by making gitolite a system user, and just using adduser:

adduser --system --group gitolite

However I'm not sure if you want gitolite to be a system user because of the ssh integration.

Which will not throw errors if the group or user exists.

* config-changed is not idempotent. Here:

  if [ "$installgitweb" = "true" ] ; then
        usermod -a -G gitolite www-data
        mkdir /var/www/gitweb

        ln -s /usr/share/gitweb/* /var/www/gitweb/.

        service apache2 restart
fi

The mkdir should have '-p' so that it won't fail if the directory exists. The ln should also have a '-f' so it will not error out because the links exist already. This is important because it will be run every time any config options are changed.

* PasswordAuthentication is turned off in the config file, but you need to reload the ssh service to make that take effect.

Anyway, thanks for sticking with this. I am excited for the charm to be in the charm store!

[Chris Hardee (shazzner)]

Thanks Clint, let me get this working. :)

[Juan L. Negron (negronjl)]

Reviewing this now.

-Juan

[Juan L. Negron (negronjl)]

The changes suggested by Clint above have not been applied yet so, at this point, the charm still needs to be revised.

-Juan

[Marco Ceppi (marcoceppi)]

Juan, did you review the shazzner or the marcoceppi branch?

[Jorge Castro (jorge)]

Is anyone working on this?