Code review comment for lp:~teknico/charms/precise/juju-gui/integrate-builtin-server
Revision history for this message
| Nicola Larosa (teknico) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
0e1f616
(Get the code!)
frankban wrote:
https:/
> config/
--guiroot=
> We could use the full path to runserver.py here.
To avoid possible interference with namesakes earlier in PATH. Done.
https:/
> config/
> 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:/
> hooks/backend.
> 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:/
> hooks/backend.
> Thank you for reordering imports.
One of those little things. :-)
https:/
> hooks/backend.
> else 'go'
> I think the definition of api_version can be moved inside the "if
> config.
Done.
> Anyway, is there a reason to prefer config.
False) over
> config[
Not really, as discussed I also removed other useless similar cases. The
only ones left are for SSL options without defaults.
https:/
> hooks/backend.
> 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:/
> hooks/backend.
> 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_
> at how that's used in utils.write_
> 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:/
> hooks/backend.
> Just curiosity: why is curl required? Is it a dependency of Tornado?
It's actually only needed by GuiMixin, moving it there.
https:/
> hooks/utils.py:458: gui_root, api_url,
api_version=
> Why do we need a DEFAULT_API_VERSION if the value is always provided?
We actually don't.
https:/
> hooks/utils.py:459: serve_tests=False, ssl_path=
> insecure=False):
> Trivial: missing docstring.
Added.
https:/
> server/
DEFAULT_
> 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:/
> server/
> 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:/