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. > 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/