User, password and read-only options.

User, password and read-only options.

This branch adds three more options to
the charm. If staging is true, user and
password options are ignored, and the
authentication is automatically handled
by the charm, providing staging pre-defined
values.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/bug-1100194-add-charm-options/+merge/143560

(do not edit description out of merge proposal)

[frankban, 2013-01-16 17:34:17]

Please take a look.

[teknico, 2013-01-16 18:02:12]

Land with changes.

Nice code, thanks. See the optional change below.

https://codereview.appspot.com/7127048/diff/1/hooks/utils.py
File hooks/utils.py (right):

https://codereview.appspot.com/7127048/diff/1/hooks/utils.py#newcode129
hooks/utils.py:129: return 'admin', 'admin'
Maybe move the default staging credentials to top level module constants?

https://codereview.appspot.com/7127048/diff/1/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/7127048/diff/1/tests/test_utils.py#newcode255
tests/test_utils.py:255: expected = ('admin', 'admin')
Module constants too.

https://codereview.appspot.com/7127048/diff/1/tests/test_utils.py#newcode455
tests/test_utils.py:455: self.assertIn('password: "admin"', js_conf)
Module constants too.

[gary.poster, 2013-01-16 18:56:46]

Land with changes.

Thank you, this is a nice change.

The only one I think is somewhat important is the additional information in the
config.yaml field description.

I'm OK with ADMIN='admin', as I think Nicola is suggesting, but wouldn't do it
myself.  I don't really understand which module constant he is advocating in
test_utils.py#newcode455--maybe 'password: "%s"' % ADMIN?  Not clear to me, and
I waver between -0 and -(fractional value) depending on what I imagine it to
mean.

Gary

https://codereview.appspot.com/7127048/diff/1/config.yaml
File config.yaml (right):

https://codereview.appspot.com/7127048/diff/1/config.yaml#newcode89
config.yaml:89: Whether or not the GUI is in read-only mode.
Perhaps add the following:

Note that read-only mode is advisory, and enforced only in the client.  If
someone can access the websocket and has the password, they can send commands
over the websocket to mutate the environment.

https://codereview.appspot.com/7127048/diff/1/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/7127048/diff/1/tests/test_utils.py#newcode248
tests/test_utils.py:248: # Provided user and password are ignored if staging is
True.
Niggle, but I'd delete "Provided."  It makes it hard to read.

https://codereview.appspot.com/7127048/diff/1/tests/test_utils.py#newcode255
tests/test_utils.py:255: expected = ('admin', 'admin')
On 2013/01/16 18:02:12, teknico wrote:
> Module constants too.

you mean ADMIN='admin'?  -0 <shrug>

https://codereview.appspot.com/7127048/diff/1/tests/test_utils.py#newcode455
tests/test_utils.py:455: self.assertIn('password: "admin"', js_conf)
On 2013/01/16 18:02:12, teknico wrote:
> Module constants too.

Which?

[frankban, 2013-01-17 10:59:23]

Thanks for the reviews Nicola and Gary.

https://codereview.appspot.com/7127048/diff/1/config.yaml
File config.yaml (right):

https://codereview.appspot.com/7127048/diff/1/config.yaml#newcode89
config.yaml:89: Whether or not the GUI is in read-only mode.
On 2013/01/16 18:56:46, gary.poster wrote:
> Perhaps add the following:
> 
> Note that read-only mode is advisory, and enforced only in the client.  If
> someone can access the websocket and has the password, they can send commands
> over the websocket to mutate the environment.

Done.

https://codereview.appspot.com/7127048/diff/1/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/7127048/diff/1/tests/test_utils.py#newcode248
tests/test_utils.py:248: # Provided user and password are ignored if staging is
True.
On 2013/01/16 18:56:46, gary.poster wrote:
> Niggle, but I'd delete "Provided."  It makes it hard to read.

Done.

[frankban, 2013-01-17 11:00:44]

*** Submitted:

User, password and read-only options.

This branch adds three more options to
the charm. If staging is true, user and
password options are ignored, and the
authentication is automatically handled
by the charm, providing staging pre-defined
values.

R=
CC=
https://codereview.appspot.com/7127048