Comment 2 for bug 1373081

Revision history for this message
Jorge Niedbalski (niedbalski) wrote :

@lazypower,

Overall looks good. I just ran a deploy of 3 units in front of apache, everything worked as expected. Please note that i didn't
tried any specific configuration option, just the vanilla installation.

Proofing charm.. OK
Tests runs .. OK
LInt ... OK

The test coverage , looks pretty good ;

hooks 553 80 86%

Only a few comments:

1) By default my system uses python 3.4 instead of 2.7, and this causes the 'make test' command to fail, since
the synchronized charmhelpers and the hooks code are not compatible with python 3.4, adding the following
lines will force the usage of 2.7.

=== modified file 'Makefile'
--- Makefile 2014-09-11 18:50:46 +0000
+++ Makefile 2014-10-01 22:29:02 +0000
@@ -19,7 +19,7 @@

 .venv:
  sudo apt-get install -y python-apt python-virtualenv
- virtualenv .venv --system-site-packages
+ virtualenv --python=python2.7 .venv --system-site-packages
  .venv/bin/pip install -I nose testtools mock pyyaml

 test: .venv

2) The following lines could be factorized using the @hooks.hook("hook_name") decorator:

    if hook_name == "install":
        install_hook()
    elif hook_name == "upgrade-charm":
        install_hook()
        config_changed()
        update_nrpe_config()
    elif hook_name == "config-changed":
        config_changed()
        update_nrpe_config()
    elif hook_name == "start":
        start_hook()
    elif hook_name == "stop":
        stop_hook()
    elif hook_name == "reverseproxy-relation-broken":
        config_changed()
    elif hook_name == "reverseproxy-relation-changed":
        reverseproxy_interface("changed")
    elif hook_name == "reverseproxy-relation-departed":
        reverseproxy_interface("departed")
    elif hook_name == "website-relation-joined":
        website_interface("joined")
    elif hook_name == "website-relation-changed":
        website_interface("changed")
    elif hook_name == "peer-relation-joined":
        website_interface("joined")
    elif hook_name == "peer-relation-changed":
        reverseproxy_interface("changed")
    elif hook_name in ("nrpe-external-master-relation-joined",
                       "local-monitors-relation-joined"):
        update_nrpe_config()
    else:
        print "Unknown hook

3) Method's documentation is not complaining with python docstrings ( http://legacy.python.org/dev/peps/pep-0257/ )