Disable console

Disable console

Adds a configuration variable for whether or not the javascript console object should be enabled or disabled.  Defaults to disabled.

https://code.launchpad.net/~makyo/charms/precise/juju-gui/juju-gui/+merge/136985

(do not edit description out of merge proposal)

[matthew.scott, 2012-11-29 16:44:19]

Please take a look.

[benji, 2012-11-29 17:00:13]

This branch looks good.  I just had one small comment on the config schema.

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

https://codereview.appspot.com/6856112/diff/1/config.yaml#newcode15
config.yaml:15: type: string
I think there is a boolean type that we could use here.

[matthew.scott, 2012-11-29 18:55:44]

Thanks for the tip on the boolean.

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

https://codereview.appspot.com/6856112/diff/1/config.yaml#newcode15
config.yaml:15: type: string
On 2012/11/29 17:00:13, benji wrote:
> I think there is a boolean type that we could use here.

Done.

[matthew.scott, 2012-11-29 18:56:34]

Please take a look.

[frankban, 2012-11-30 10:24:22]

Looks good to me.
I have just one minor below, but I think this can be landed.

https://codereview.appspot.com/6856112/diff/5001/hooks/start
File hooks/start (right):

https://codereview.appspot.com/6856112/diff/5001/hooks/start#newcode23
hooks/start:23: import json
I usually separate imports in 3 groups: stdlib (see os and sys above), other
installed libraries (e.g. charmhelpers) and application ones (utils). Inside
each group, libraries are imported in alphabetical order. json should be placed
in the first group. Anyway, this is just a suggestion, do with it what you want,
I am not sure we have a code style for that.

https://codereview.appspot.com/6856112/diff/5001/hooks/start#newcode51
hooks/start:51: 'console_enabled':
json.dumps(config['juju-gui-console-enabled'])
Really nice.

[gary.poster, 2012-11-30 13:10:59]

Fly-by review.  :-)  Looks good to me, but I didn't do a full review because you
had the ones you needed.  Thanks!

Gary

https://codereview.appspot.com/6856112/diff/5001/hooks/start
File hooks/start (right):

https://codereview.appspot.com/6856112/diff/5001/hooks/start#newcode23
hooks/start:23: import json
On 2012/11/30 10:24:22, frankban wrote:
> I usually separate imports in 3 groups: stdlib (see os and sys above), other
> installed libraries (e.g. charmhelpers) and application ones (utils). Inside
> each group, libraries are imported in alphabetical order. json should be
placed
> in the first group. Anyway, this is just a suggestion, do with it what you
want,
> I am not sure we have a code style for that. 

It does appear to be the current style of the file, which we like to honor. 
Also, what Francesco suggests is the recommended PEP 8 style
(http://www.python.org/dev/peps/pep-0008/#imports) with the addition of
alphabetical sorting.

[matthew.scott, 2012-11-30 15:05:28]

*** Submitted:

Disable console

Adds a configuration variable for whether or not the javascript console object
should be enabled or disabled.  Defaults to disabled.

R=benji, frankban, gary.poster
CC=
https://codereview.appspot.com/6856112

[matthew.scott, 2012-12-06 16:07:38]

Please take a look.

[benji, 2012-12-06 16:44:28]

This looks good.  I think we need to figure out the expires headers before
landing, doing that wrong could cause large amounts of irritation to our users.

https://codereview.appspot.com/6856112/diff/5001/hooks/start
File hooks/start (right):

https://codereview.appspot.com/6856112/diff/5001/hooks/start#newcode23
hooks/start:23: import json
On 2012/11/30 10:24:22, frankban wrote:
> I usually separate imports in 3 groups:
[snip]
> I am not sure we have a code style for that. 

That is the style guideline for Launch^WCloud Engineering code.

I used to do the same thing on my projects until I worked at a company that
practised the smash-all-the-imports-together approach.  I worked there for more
than five years and never once missed the carefully arranged imports and enjoyed
the freedom of being able to just add an import and then lexicographically sort
the whole block and call it done.

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template
File config/nginx.conf.template (right):

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template#...
config/nginx.conf.template:10: expires max;
If we use "expires max" here we will also need to version the assets directory
so upgrading the app will work correctly.  We can leave the "expires" out and
let nginx's normal if-modified-since logic do the work.

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template#...
config/nginx.conf.template:14: expires max;
We can probably live with the favicon lagging behind the most recent version,
but "max" (10 years) is a bit much.  How about "expires 10d" to give us caching
of the favicon for 10 days.

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template#...
config/nginx.conf.template:17: location = /index.html {
Given the rule of last resort below, do we need this rule?

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template#...
config/nginx.conf.template:18: expires max;
I do not think we want to specify and "expires" here.

If we use "expires max" and the user installs a new version of juju-gui they
will not see it until they clear their browser's cache.

[matthew.scott, 2012-12-06 16:53:51]

Thanks for the comments, changes are made!  Sorry about the broken
proposal/review, changes will show up in https://codereview.appspot.com/6898043/

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template
File config/nginx.conf.template (right):

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template#...
config/nginx.conf.template:10: expires max;
On 2012/12/06 16:44:28, benji wrote:
> If we use "expires max" here we will also need to version the assets directory
> so upgrading the app will work correctly.  We can leave the "expires" out and
> let nginx's normal if-modified-since logic do the work.

Done.

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template#...
config/nginx.conf.template:14: expires max;
On 2012/12/06 16:44:28, benji wrote:
> We can probably live with the favicon lagging behind the most recent version,
> but "max" (10 years) is a bit much.  How about "expires 10d" to give us
caching
> of the favicon for 10 days.

Done.

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template#...
config/nginx.conf.template:17: location = /index.html {
On 2012/12/06 16:44:28, benji wrote:
> Given the rule of last resort below, do we need this rule?

The last rule, when trying to access index.html, will cause a rewrite loop
without this rule.

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template#...
config/nginx.conf.template:18: expires max;
On 2012/12/06 16:44:28, benji wrote:
> I do not think we want to specify and "expires" here.
> 
> If we use "expires max" and the user installs a new version of juju-gui they
> will not see it until they clear their browser's cache.

Done.

[benji, 2012-12-06 16:57:52]

Looks great.  Thanks for the changes and answers.

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template
File config/nginx.conf.template (right):

https://codereview.appspot.com/6856112/diff/10001/config/nginx.conf.template#...
config/nginx.conf.template:17: location = /index.html {
On 2012/12/06 16:53:51, matthew.scott wrote:
> On 2012/12/06 16:44:28, benji wrote:
> > Given the rule of last resort below, do we need this rule?
> 
> The last rule, when trying to access index.html, will cause a rewrite loop
> without this rule.

Ah!  That makes sense.  I guess we could change ".*" to be "not index.html" but
that's probably more complicated than the current way.