frankban wrote:

https://codereview.appspot.com/12086044/diff/4001/config/guiserver.conf.template#newcode7
> config/guiserver.conf.template:7: exec runserver.py
--guiroot="{{gui_root}}" \
> We could use the full path to runserver.py here.

To avoid possible interference with namesakes earlier in PATH. Done.



https://codereview.appspot.com/12086044/diff/4001/config/guiserver.conf.template#newcode9
> config/guiserver.conf.template:9: {{if api_version}}
> What do you think about always providing --apiversion and --sslpath?
> We know those values in the hooks, and specifying them here allows for
> removing these if blocks and also generates a more explicit config,
> i.e. looking at the rendered upstart config we always know where are
> the ssl certs and what juju implementation we are currently using.

Done.



https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode24
> hooks/backend.py:24:
> On 2013/07/31 21:18:18, gary.poster wrote:
>> Composition is supposed to be simpler than inheritance, but I'm not
>> sure this entirely succeeded.  My feeling is that it is because its
>> composition is still so similar in feel to inheritance.  I think I
>> might be happier with this if, rather that Backend's

>> [smip]

>> and so on.

> +1!

Agreed, I'll tackle this in a future branch, as mentioned in the reply
to gary's review.



https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode35
> hooks/backend.py:35: import charmhelpers
> Thank you for reordering imports.

One of those little things. :-)



https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode140
> hooks/backend.py:140: api_version = 'python' if utils.legacy_juju()
> else 'go'
> I think the definition of api_version can be moved inside the "if
> config.get('builtin-server', False)" block below.

Done.


> Anyway, is there a reason to prefer config.get('builtin-server',
False) over
> config['builtin-server']?

Not really, as discussed I also removed other useless similar cases. The
only ones left are for SSL options without defaults.



https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode141
> hooks/backend.py:141: if config.get('builtin-server', False):
> On 2013/07/31 21:18:18, gary.poster wrote:
>> For the change story, do you think we ought to remove the old
>> haproxy/apache startup files?  I'm inclined to say yes, but open to
>> alternate opinions. :-)

>> I guess that would happen in "stop"?

> I agree. So, IIUC:
> - the start method generates the upstart files;
> - the stop method (called by the stop and by the config-changed hooks)
>    removes the upstart file(s) in use for that configuration.

Agreed, as mentioned in the reply to gary's review.



https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode143
> hooks/backend.py:143: utils.JUJU_GUI_DIR, utils.get_api_address(),
> utils.JUJU_GUI_DIR is passed here and used in the function as
> gui_root. This does not seem to be correct. I expected "build_dir" to
> be the right value to pass here instead: am I missing something?

True, that was a thinko on my part, thank you.


> Moreover, utils.get_api_address() seems wrong too. Please take a look
> at how that's used in utils.write_haproxy_config(). I am +1, if you
> want, on changing that helper function to handle both the pyJuju and
> juju-core cases.

That's also true, addressed this too.


> Also note that the returned address does not include the schema
> (wss://) and IIRC, the builtin server requires it to be passed.

And this..



https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode200
> hooks/backend.py:200: debs = ('curl', 'openssl', 'python-pip')
> Just curiosity: why is curl required? Is it a dependency of Tornado?

It's actually only needed by GuiMixin, moving it there.



https://codereview.appspot.com/12086044/diff/4001/hooks/utils.py#newcode458
> hooks/utils.py:458: gui_root, api_url,
api_version=DEFAULT_API_VERSION,
> Why do we need a DEFAULT_API_VERSION if the value is always provided?

We actually don't.



https://codereview.appspot.com/12086044/diff/4001/hooks/utils.py#newcode459
> hooks/utils.py:459: serve_tests=False, ssl_path=DEFAULT_SSL_PATH,
> insecure=False):
> Trivial: missing docstring.

Added.



https://codereview.appspot.com/12086044/diff/4001/server/guiserver/manage.py#newcode37
> server/guiserver/manage.py:37: from hooks_utils import
DEFAULT_API_VERSION,
> DEFAULT_SSL_PATH
> Hum... here is why DEFAULT_API_VERSION is a constant.
> Is it worth connecting (coupling?) the server to the hooks, creating a
> symlink etc. just for those two values (which, incidentally, are
> always provided in the upstart script)? I am not sure, but feel free
> to ignore my opinion here.

You're right, it's not worth it. It irked me too and it's not even
needed, those default values are unnecessary, as you point out.



https://codereview.appspot.com/12086044/diff/4001/server/runserver.py#newcode27
> server/runserver.py:27: (Specifying the last two options together does
> not actually make sense.)
> Perhaps, rather than highlighting the meaningless of it all, it's
> worth saying that --sslpath is ignored if --insecure is set.

Your suggestion make sense, done.

However, highlighting the meaningless of it all, in the general sense,
is something I find very appealing. ;-)


https://codereview.appspot.com/12086044/