Code review comment for lp://staging/~frankban/charms/precise/juju-gui/server-insecure-mode

*** Submitted:

Support serving the GUI over a non secure conn.

The GUI server can be configured to run the
GUI static files server and the WebSocket server over
an HTTP connection.
This way we support the secure=false charm option.

Tests:
   Run `make unittest` from the branch root.

R=gary.poster, teknico
CC=
https://codereview.appspot.com/12088046

https://codereview.appspot.com/12088046/diff/1/server/guiserver/manage.py
File server/guiserver/manage.py (right):

https://codereview.appspot.com/12088046/diff/1/server/guiserver/manage.py#newcode112
server/guiserver/manage.py:112: 'understand and accept the risks.')
On 2013/07/31 09:45:11, teknico wrote:
> Simpler help: "Set to True to serve the GUI over an insecure
connection (HTTP).
> Please understand and accept the inherent risks before doing so."

Yes, that's simpler, but I like the more explicit "Don't do that
unless..." in the original help.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/manage.py#newcode124
server/guiserver/manage.py:124: server().listen(80)
On 2013/07/31 09:45:11, teknico wrote:
> Would it be useful to add a redirector from 443 to 80 here?

AFAIK, this is only used in the Saucelabs CI tests for ie. In that
story, I don't think what you suggest is useful. I am not sure this is a
good idea also in general, from the user perspective: a 404 is better
than an automatic (implicit) redirection from a secure connection to an
insecure one IMHO.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py
File server/guiserver/tests/test_manage.py (right):

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py#newcode157
server/guiserver/tests/test_manage.py:157: managers = nested(
On 2013/07/31 09:45:11, teknico wrote:
> As discussed, the "nested" function is deprecated and does not add
value here,
> so it would be better not using it. If the linter gives you a hard
time with the
> multiple manager form of the "with" statement, the linter is broken
and should
> be fixed. :-)

Done.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py#newcode176
server/guiserver/tests/test_manage.py:176:
redirector_listen.assert_called_once_with(80)
On 2013/07/31 09:45:11, teknico wrote:
> Better swap these two asserts, to keep the same ordering as above.

Done.

https://codereview.appspot.com/12088046/diff/1/server/guiserver/tests/test_manage.py#newcode182
server/guiserver/tests/test_manage.py:182: self.assertEqual(0,
redirector_listen.call_count)
On 2013/07/31 09:45:11, teknico wrote:
> Better swap these two asserts too, for the same reason.

Done.

https://codereview.appspot.com/12088046/