Comment 1 for bug 1012942

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

Hi Patrick, its great to review the other side of your django/wsgi charm. :)

* Cool use of SWIFT!

Issues
--------

* Error checking. You do a lot of operations throughout your hooks, but no error checking is done. I'd like to see 'set -e' at the very least in your bash scripts. 'set -ue' would be even better. If you cannot do that, please assert why its ok to do all the operations even if some of the earlier ones failed.

* website-relation-joined: I've not seen 'domain' used in the http relation. Adding optional fields is ok, but this probably deserves some explanation.
* website-relation-joined: hostname -f should be changed to unit-get private-address
* hooks/start and hooks/stop are useless in this context with the wsgi subordinate doing all the resident daemon running, please remove them.

Fix these, and change the status to Fix Committed, and we should be able to get your charm promulgated.

Minors
---------

* website-relation-joined: this does no relation-get, so it seems like it should be a joined only.
* a lot of the config parameters are ignored after install, this should be documented in their config.yaml description, as users should be able to expect that config parameters will be respected any time 'juju set' is used. Ideally all of that would be moved into config-changed anyway, so the service can be changed at will, but that can also make config-changed more complicated.
* pip does not cryptographically verify its downloads. While this is optional, since one doesn't have to use the pip requirements bit, it should include a warning in config.yaml that these are not as safe as using packaged software.
* config.yaml - you only support hg, git, bzr, and svn, please document this in the config.yaml description