Integrate the built-in server into the charm.

Integrate the built-in server into the charm.

Add a "builtin-server" option to config.yaml, defaulting to false, to enable
a new Tornado-based built-in web server, in place of haproxy and Apache (still
the default).

Add a "guiserver.conf" Upstart config file, generated via a template.

Add a BuiltinServerMixin to hooks/backend.py .

Add tests for the new functions in hooks/utils.py .

Clean up all paths and constants in hooks/utils.py .

Miscellaneuos cleanup.

QA1 (quick):

$ make unittest

QA2 (long, 40 minutes):

$ make ftest

QA3 (long but shorter than the former):

$ juju bootstrap
$ make deploy
  (check web access)
$ juju set juju-gui builtin-server=true
  (check web access)
$ juju set juju-gui builtin-server=false
  (check web access)
$ juju destroy-environment

https://code.launchpad.net/~teknico/charms/precise/juju-gui/integrate-builtin-server-2/+merge/179138

(do not edit description out of merge proposal)

[teknico, 2013-08-08 09:48:01]

Please take a look.

[frankban, 2013-08-08 11:12:58]

This branch is nice Nicola, we are almost there, thank you.
Please fix the problem I described below, I will QA again later.

https://codereview.appspot.com/12650044/diff/1/config/guiserver.conf.template
File config/guiserver.conf.template (right):

https://codereview.appspot.com/12650044/diff/1/config/guiserver.conf.template...
config/guiserver.conf.template:7: exec /usr/bin/python
/usr/local/bin/runserver.py \
Cool.

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

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode110
hooks/utils.py:110: SERVER_DIR = os.path.join(CURRENT_DIR, 'server')
I am not sure I like all those constants. I agree they are used in more than one
place (but usually only two times AFAIK, and e.g. UNIT_NAME is used only once),
so this prevents typos, but I also liked the possibility to read the functions
without having to jump at the beginning of the file. As I said, I see the value
in this change, and I can live with this. 
The real request here is to fix all those "os.path.join('', ...": see below.

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode132
hooks/utils.py:132: config_json = Serializer(os.path.join('', 'tmp',
'config.json'))
I don't think this is the same as before: this results in "tmp/config.json".
You might want to use "os.path.join(os.path.sep, 'tmp', 'config.json')", but I
am also ok with the original version.
Note that this generates an install error, that prevented me to QA this branch.
I'll retry it later when this is fixed.

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode549
hooks/utils.py:549: api_url = '{}://127.0.0.1:{}'.format(
The "x if y else z" is not so easy to read when it is not a one-liner IMHO.
I had to read this carefully, perhaps the more usual "if: else:" can be better?

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode564
hooks/utils.py:564: def start_builtin_server(build_dir, serve_tests,
ssl_cert_path, insecure):
What do you think about inverting serve_tests and ssl_cert_path? This way we can
have a similar signature for start_builtin_server and
write_builtin_server_startup.

https://codereview.appspot.com/12650044/diff/1/tests/20-functional.test
File tests/20-functional.test (right):

https://codereview.appspot.com/12650044/diff/1/tests/20-functional.test#newco...
tests/20-functional.test:189: self.assertIn('TornadoServer', server_header)
The header check is great. I'd also add an environment check, i.e. we must 
ensure that the WebSocket is connected (basically the same thing we do in the
other functional tests).

[teknico, 2013-08-08 11:29:40]

frankban wrote:
> https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode110
> hooks/utils.py:110: SERVER_DIR = os.path.join(CURRENT_DIR, 'server')
> I also liked the possibility to read the functions without having
> to jump at the beginning of the file.

Hopefully the names of the constants are meaningful enough that you don't need
to see the associated values to be able to understand the code.


> As I said, I see the value in this change, and I can live with this. 

Good.


> https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode132
> hooks/utils.py:132: config_json = Serializer(os.path.join('', 'tmp',
> 'config.json'))
> I don't think this is the same as before: this results in
> "tmp/config.json". You might want to use "os.path.join(os.path.sep,
> 'tmp', 'config.json')", but I am also ok with the original version.

Oh, sorry, I did not check it, will fix.


> Note that this generates an install error, that prevented me to
> QA this branch. I'll retry it later when this is fixed.

Weird, this did not happen to me in QA.


> https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode549
> hooks/utils.py:549: api_url = '{}://127.0.0.1:{}'.format(
> The "x if y else z" is not so easy to read when it is not a
> one-liner IMHO. I had to read this carefully, perhaps the more
> usual "if: else:" can be better?

Hrmpf. You know I like it, and I know you don't like it. Is it really that ugly?
:-)


> https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode564
> hooks/utils.py:564: def start_builtin_server(build_dir, serve_tests,
> ssl_cert_path, insecure):
> What do you think about inverting serve_tests and ssl_cert_path?
> This way we can have a similar signature for start_builtin_server
> and write_builtin_server_startup.

Uhm, I don't like it either but there was a reason for it, I'll check.


>
https://codereview.appspot.com/12650044/diff/1/tests/20-functional.test#newco...
> tests/20-functional.test:189: self.assertIn('TornadoServer', server_header)
> The header check is great. I'd also add an environment check, i.e. we
> must ensure that the WebSocket is connected (basically the same thing
> we do in the other functional tests).

Will do.

[teknico, 2013-08-08 15:41:23]

Please take a look.

[matthew.scott, 2013-08-08 16:28:23]

Code looks good with Francesco's comments, QAing now.

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

https://codereview.appspot.com/12650044/diff/1/hooks/utils.py#newcode549
hooks/utils.py:549: api_url = '{}://127.0.0.1:{}'.format(
On 2013/08/08 11:12:58, frankban wrote:
> The "x if y else z" is not so easy to read when it is not a one-liner IMHO.
> I had to read this carefully, perhaps the more usual "if: else:" can be
better?

I agree in this instance. The x if y else z construct works much better in a
single-line format than across multiple lines; in this instance, it's hard to
tell that this is conditional until halfway through, then you have to parse it
inside out across multiple lines.

[matthew.scott, 2013-08-08 17:08:02]

QA3 worked for me, LGTM

[frankban, 2013-08-09 09:00:16]

LGTM with the changes below.

I run the functional tests, they were passing, the last one was stopped by the
timeout: 40 mins are not enough, please increase that value (60 mins?).

QA:
switching from the legacy to the new server (and back) worked well. The builtin
server stopped working after setting secure=false on the charm. I think upstart
is not happy with the generated configuration file when --insecure is passed. 

I believe this branch can land with the timeout fix, but please add a card to
fix the "insecure" bug. Thank you!

[teknico, 2013-08-09 11:15:26]

frankban wrote:
> I run the functional tests, they were passing, the last one was stopped
> by the timeout: 40 mins are not enough, please increase that value
> (60 mins?).

Timeout increased to 60 minutes.

> QA:
> switching from the legacy to the new server (and back) worked well. The
> I think upstart builtin server stopped working after setting
> secure=false on the charm. is not happy with the generated
> configuration file when --insecure is passed.

Indeed, there was a mistake in config/guiserver.conf.template. Fixed, thank you.

[teknico, 2013-08-09 14:08:14]

*** Submitted:

Integrate the built-in server into the charm.

Add a "builtin-server" option to config.yaml, defaulting to false, to enable
a new Tornado-based built-in web server, in place of haproxy and Apache (still
the default).

Add a "guiserver.conf" Upstart config file, generated via a template.

Add a BuiltinServerMixin to hooks/backend.py .

Add tests for the new functions in hooks/utils.py .

Clean up all paths and constants in hooks/utils.py .

Miscellaneuos cleanup.

QA1 (quick):

$ make unittest

QA2 (long, 40 minutes):

$ make ftest

QA3 (long but shorter than the former):

$ juju bootstrap
$ make deploy
  (check web access)
$ juju set juju-gui builtin-server=true
  (check web access)
$ juju set juju-gui builtin-server=false
  (check web access)
$ juju destroy-environment

R=frankban, matthew.scott
CC=
https://codereview.appspot.com/12650044