Code review comment for lp:~teknico/charms/precise/juju-gui/integrate-builtin-server

gary.poster wrote:
> Hi Nicola. This is very nice, thank you. I have some trivial changes
> that you can take or leave, including a visit by the Grammar Police
> and a few visits by the Idiosyncratic Text Rewriter.

Hi fellows, you're always welcome, make yourselves at home. Fancy some
tea? ;-)

> I think that the new upstart script should follow the haproxy upstart
> pattern. Namely, it should be created during install, and copied over
> in start(). We should also remove the copy, and the haproxy copy when
> appropriate, in stop(), I think!

> Otherwise, I think we will have problems when we switch from one
> server to another. It will also be the right way forward when we
> think about upgrading, I think.

Yes, that's a better strategy. See also frankban's clarification.

https://codereview.appspot.com/12086044/diff/4001/Operation.md#newcode40
> Operation.md:40: removed, only the built-in server will remain.
> removed. Only

> or

> removed, and only

> or

> removed; only

Done; third option, I love semicolons.

> https://codereview.appspot.com/12086044/diff/4001/config.yaml
> File config.yaml (right):

https://codereview.appspot.com/12086044/diff/4001/config.yaml#newcode171
> config.yaml:171: Enable the built-in server, disabling both haproxy
> and Apache. suggestion:

> "This is a temporary option. The built-in server will be the only
> server in the future."

Added.

> (unless we want it to serve the sandbox?)

Not sure what you mean here.

> https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py
> File hooks/backend.py (right):

https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode24
> hooks/backend.py:24:
> Maybe add the following?

> """Mixins are not actually mixed in to the backend class using Python
> inheritance machinery. Instead, each mixin is instantiated and
> collected in the Backend __init__, as needed. Then the install(),
> start(), and stop() methods have a "self" that is the simple
> instantiated mixin, and a "backend" argument that is the backend
> instance. Python inheritance machinery is somewhat mimicked in that
> certain properties and methods are explicitly aggregated on the
> backend instance: see the chain_methods function, the merge_properties
> method, and their usages."""

Added.

> <Gary procrastinates from writing doc he is supposed to be doing>

That's ok. ;-)

> [rambling snipped]
> Anyway, just rambling. Nothing to do with this branch.

This actually points out something that's been nagging me for a while.
As discussed, the code structure has proved itself hard to read and
understand. I'll try your suggestions in a future branch and see how
much they improve matters.

https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode139
> hooks/backend.py:139: # TODO: eventually the option, haproxy and
> Apache will go away
> a bit confusing.

> eventually this option will go away, as well as haproxy and Apache

> or similar?

Done.

https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode141
> hooks/backend.py:141: if config.get('builtin-server', False):
> 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"?

That's right, we want no trace of the alternate implementation around.

https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode147
> hooks/backend.py:147: else:
> Similarly, I think we ought to remove the old guiserver file in this
> case.

> Alternatively, we always generate both files, and only add/remove them
> from the upstart directory in ServerInstallMixinBase._post_install?

It's cleaner to only generate them when needed.

https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode204
> hooks/backend.py:204: # Install Tornado from a local tarball.
> Nice idea. We should make the GUI and the Python branch do this too,
> and only look for new ones when some flag is turned on, like "check-
> for-new-versions".

Possibly, yes.

https://codereview.appspot.com/12086044/diff/4001/hooks/backend.py#newcode291
> hooks/backend.py:291: # TODO: eventually the option, haproxy and
> Apache will go away
> same comment about slight confusion here. trivial, ignorable, maybe
> idiosyncratic on my part.

Done too. :-)

https://codereview.appspot.com/12086044/diff/4001/hooks/utils.py#newcode469
> hooks/utils.py:469: render_to_file('guiserver.conf.template', context,
> BUILTIN_SERVER_STARTUP)
> Ah! you don't copy it over but create it in place, I see.

> hm. I think it is in fact technically more correct to do it (within
> the context of the framework) the way haproxy does it. We don't want
> the upstart script installed unless we have started. If we have
> stopped, then the upstart script should not be there.

> Also, if we copy over the file in start and remove the copy in stop,
> there's a clear point at which to uninstall the upgrade script, which
> is the right thing to do and will be important for supporting the
> ability to switch between apache/haproxy and gui server. That also
> might be important for our upgrade story.

> I'd like us to follow that model.

Agreed, see above.

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