yellow team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Land with changes.

This branch looks good Nicola: it's nice to see everything's working
with TLS enabled. Tests pass.
Please take a look at my comments below: especially the config-changed
issue needs to be addressed IMHO.



https://codereview.appspot.com/7007045/diff/6002/HACKING.md
File HACKING.md (right):

https://codereview.appspot.com/7007045/diff/6002/HACKING.md#newcode128
HACKING.md:128: /var/lib/juju/units/[NAME OF UNIT].  There is a
charm.log file to investigate,
Good catch, thank you.

https://codereview.appspot.com/7007045/diff/6002/hooks/config-changed
File hooks/config-changed (right):

https://codereview.appspot.com/7007045/diff/6002/hooks/config-changed#newcode88
hooks/config-changed:88: start_improv(
The improv and the api agent services now depend on the certificates
path, and need to be restarted if the ssl-cert-path is changed. AFAICT,
currently this is not handled here. E.g., if the user run ``juju set
juju-gui ssl-cert-path=/tmp/juju-gui``, the services are not restarted
and the GUI ends up in a broken state.

https://codereview.appspot.com/7007045/diff/6002/hooks/config-changed#newcode106
hooks/config-changed:106: start_agent(
See above.

https://codereview.appspot.com/7007045/diff/6002/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/7007045/diff/6002/hooks/utils.py#newcode59
hooks/utils.py:59: SSL_CERT_PATH = '/etc/ssl/juju-gui'
Here the config default is redefined, and this value is used as default
for the functions below needing the tls keys path.
Is this really required? I believe that the default value is never used,
i.e. each time we call start_improv, start_agent etc., we always pass
the value taken from config. Moreover, it seems to me that if, in the
future, we want to change the default path, we will need to change the
same value in two different places.

https://codereview.appspot.com/7007045/diff/6002/hooks/utils.py#newcode339
hooks/utils.py:339: crt_path = os.path.join(ssl_cert_path, 'juju.crt')
I like how you renamed the keys.

https://codereview.appspot.com/7007045/

-- 
https://code.launchpad.net/~teknico/charms/precise/juju-gui/encrypt-api-env-connection/+merge/141107
Your team Juju GUI Hackers is requested to review the proposed merge of lp:~teknico/charms/precise/juju-gui/encrypt-api-env-connection into lp:~juju-gui/charms/precise/juju-gui/trunk.