Code review comment for lp:~frankban/charms/precise/juju-gui/support-firewall

Revision history for this message
Francesco Banconi (frankban) wrote :

Reviewers: mp+189645_code.launchpad.net,

Message:
Please take a look.

Description:
Avoid installing from PPA if not required.

No longer add the juju-gui PPA by default:
the external repository is added only if required,
i.e. if the legacy server is used or if a branch
is passed to juju-gui-source.

The only missing bit to make the charm work well
from behind a firewall AFAICT is avoiding the release
to be downloaded from Launchpad.

Also included the shelltoolbox file in the charm:
unfortunately the python-shelltoolbox package is
not available on precise. On the other hand, this
allows for getting rid of the bootstrap_utils.py
file, and the install hook now feels cleaner.

Refactoring + some magic removal on the backend
framework. Now it should be less surprising, and
also allows for more customizations, e.g. what
I did in the install method.

Also added missing tests for the backend framework:
those were required in order to increase our control
over what's really happening in the backend "hooks".

Switched to the builtin Tornado server by default.

This diff is very big, I am sorry, but:
- you can ignore the bootstrap_utils removal;
- you can ignore the shelltoolbox.py file: it is
   just a copy of the one present in the raring
   python-shelltoolbox package;
- a lot of code is tests, the rest of the code
   should be quite easy to follow.

QA:
     `make deploy` and watch the logs:
     - no PPA should be installed by default;
     - the deployment succeeds and the GUI works well;
     switch to builtin-server=false and watch the logs:
     - the PPA is installed (and then haproxy, apache...);
     - the config-change hook exits without errors and
       the GUI works well.

Tests:
     `make unittest`
     (I ran the functional tests myself).

Thank you!

https://code.launchpad.net/~frankban/charms/precise/juju-gui/support-firewall/+merge/189645

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14433049/

Affected files (+1261, -365 lines):
   A [revision details]
   M config.yaml
   M hooks/backend.py
   D hooks/bootstrap_utils.py
   M hooks/install
   A hooks/shelltoolbox.py
   M hooks/utils.py
   M revision
   M tests/requirements.pip
   M tests/test_backends.py
   M tests/test_utils.py

« Back to merge proposal