Comment 5 for bug 944246

Revision history for this message
Clint Byrum (clint-fewbar) wrote :

Bi Ben! The charm is looking great. Very few things left to change:

charm proof

I'd recommend running 'charm proof' in the charm root after most changes. It shows this right now:

I: relation subway has no hooks
E: revision file in root of charm is required

The hooks problem is actually quite important.. as the 'subway' relation needs a joined hook that pushes its hostname and port into the relationship. So you need a file

hooks/subway-relation-joined

Which at its minimum is an executable that looks like this:

#!/bin/sh
relation-set hostname=`unit-get private-address` port=80

That way users can relate your charm to a web proxy of any kind and it will inform their proxy of the port and hostname to send traffic to.

charm-helpers

# Install the totally awesome charm-helper-sh package
add-apt-repository ppa:charmers/charm-helpers
apt-get update && apt-get install -y charm-helper-sh

You don't actually use charm-helpers, so this block can be deleted.

tmp/meta

Its not clear how these directories are used. Is that used by subway itself after startup? If so, beware that the whole charm dir is cleaned up, so on 'destroy-service', the data will be wiped out (normally its left alone in case you want to backup or transfer it to another machine between destroy-service and terminate-machine).

adding nohup

This needs to go in the 'start' hook, not install. Also the corresponding way to stop the service should be in stop. Does node provide a pidfile? Even better, you could use an upstart job to be the cool kid on the block. :)

running as root

Best not to run things as root like this. I'd suggest creating a user called 'subway' with

adduser --system --group subway

-- More review later, reached EOD, but please respond to these soon. :)