Refactor for easier builtin server integration.

Refactor for easier builtin server integration.

Refactor the charm code to ease integration of the builtin server.
Detail of the changes, per file:

hooks/backend.py:

- order the mixins code as they are called, to ease readability;
- merge InstallMixin into GuiMixin;
- rename UpstartMixin to HaproxyApacheMixin;
- reimplement GuiMixin.install using the new functions from the split of
  hooks/utils.py#fetch_gui (see below);
- reimplement GuiMixin.start using the new functions from the split of
  hooks/utils.py#start_gui (see below);
- move here the chain and merge functions from hooks/utils.py, renaming
  them to be less generic;
- further simplify the backend constructor code.

hooks/utils.py:

- rename JUJU_GUI_SITE and JUJU_GUI_PORTS to APACHE_SITE and
  APACHE_PORTS respectively;
- split the start_gui function into compute_build_dir, write_gui_config
  and write_haproxy_config;
- split the fetch_gui function into fetch_gui_from_branch and
  fetch_gui_release;
- move the chain and merge functions to hooks/backend.py.

tests/test_backends.py:

- rename, add mocks and reorder to make tests pass again.

tests/test_utils.py:

- rename TestStartStop to TestStartImprovAgentGui;
- split the start_gui function tests in TestStartImprovAgentGui into
  tests for compute_build_dir, write_gui_config, write_haproxy_config
  and write_apache_config;

Sorry for the big diff. It was even bigger, but I moved some of the
changes to a future branch.

https://code.launchpad.net/~teknico/charms/precise/juju-gui/refactor-gui-startup/+merge/177364

(do not edit description out of merge proposal)

[teknico, 2013-07-29 11:12:06]

Please take a look.

[bac, 2013-07-29 13:53:58]

LGTM Nicola.  Thanks for the nice reorganization.

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py#newcode67
hooks/backend.py:67: def install(self, backend):
This is the same as for ImprovMixin.  You want to extract it or do you think
that's be too much abstraction?

https://codereview.appspot.com/12022044/diff/1/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/12022044/diff/1/tests/test_utils.py#newcode639
tests/test_utils.py:639: def test_compute_build_dir(self):
This is a nice, compact test but it is pretty hard to decipher.  Perhaps an
brief comment would help.

[teknico, 2013-07-29 14:09:10]

Thanks for the review, see replies below.

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py#newcode67
hooks/backend.py:67: def install(self, backend):
bac wrote:
> This is the same as for ImprovMixin.  You want to extract it
> or do you think that's be too much abstraction?

Uhm, not sure. A superclass for the mixins, you say?

https://codereview.appspot.com/12022044/diff/1/tests/test_utils.py
File tests/test_utils.py (right):

https://codereview.appspot.com/12022044/diff/1/tests/test_utils.py#newcode639
tests/test_utils.py:639: def test_compute_build_dir(self):
bac wrote:
> This is a nice, compact test but it is pretty hard to decipher.
> Perhaps a brief comment would help.

Good idea.

[gary.poster, 2013-07-29 14:26:51]

LGTM. Very nice changes, Nicola.  I have a few small comments, one of which
might address a conversation you and Brad are having.

In your cover letter, you mentioned "reorder to make tests pass again." 
Reordering tests to make them pass made me nervous, but looking at the tests I
don't think that's what you did.  What did you mean?

Thanks!

Gary

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py
File hooks/backend.py (right):

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py#newcode40
hooks/backend.py:40: class ImprovMixin(object):
You stated in your cover letter that you wanted these to be arranged "as they
are called, to ease readability."  I suggest you add a comment to that effect,
so that there is a chance that the intended order is understood, appreciated,
and maintained in the future.

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py#newcode71
hooks/backend.py:71: utils.fetch_api(config['juju-api-branch'])
So I see now that we will do this twice (once in the Python Mixin and once in
the ImprovMixin) if staging is turned on.  Not horrible, but not ideal.

I'd recommend removing the install method from ImprovMixin.  The ImprovMixin can
only work with the PythonMixin, and the PythonMixin will do everything the
ImprovMixin needs in this regard.

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py#newcode93
hooks/backend.py:93: class GuiMixin(object):
This merge makes sense to me, thanks.

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py#newcode108
hooks/backend.py:108: if backend.different('juju-gui-source'):
This is nicer too, I agree, thanks.

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py#newcode128
hooks/backend.py:128: utils.write_gui_config(
It's nice to have a clearer idea what is going on when you read this backend.

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py#newcode175
hooks/backend.py:175: def chain_methods(name):
I like the chain_methods and merge_properties move and rename also.  Makes it
less mysterious.  OTOH, I think Ben and others (Matt Wedgwood?) are interested
in making this some shared, simple infrastructure, which would push it back out
again, but we can worry about that when and if it comes up.

https://codereview.appspot.com/12022044/diff/1/hooks/backend.py#newcode241
hooks/backend.py:241: self.mixins.append(GuiMixin())
I take it that you verified that it doesn't matter that the InstallMixin happens
last now?  Makes me a bit nervous...however, I reviewed the install methods and
it looks ok to me.  If you have done the same, and we've done reasonable qa,
cool.

[teknico, 2013-07-29 14:55:41]

gary.poster wrote:
> In your cover letter, you mentioned "reorder to make tests pass again." 
> Reordering tests to make them pass made me nervous, but looking at the
> tests I don't think that's what you did.  What did you mean?

Sorry, I was too concise. :-) I made tests pass by renaming and adding mocks:
the reordering referred to mocks and other names, but was not pertinent to tests
passing, as you correctly noted.


> hooks/backend.py:40: class ImprovMixin(object):
> You stated in your cover letter that you wanted these to be arranged "as they
> are called, to ease readability."  I suggest you add a comment to that effect,
> so that there is a chance that the intended order is understood, appreciated,
> and maintained in the future.

Good idea, done.


> hooks/backend.py:71: utils.fetch_api(config['juju-api-branch'])
> So I see now that we will do this twice (once in the Python Mixin and once in
> the ImprovMixin) if staging is turned on.  Not horrible, but not ideal.

It does not look that way to me. The relevant code is:

            if staging:
                self.mixins.append(ImprovMixin())
            elif sandbox:
                self.mixins.append(SandboxMixin())
            else:
                self.mixins.append(PythonMixin())

We either instantiate ImprovMixin or PythonMixin, never both of them.


> hooks/backend.py:175: def chain_methods(name):
> I like the chain_methods and merge_properties move and rename also.  Makes it
> less mysterious.  OTOH, I think Ben and others (Matt Wedgwood?) are interested
> in making this some shared, simple infrastructure, which would push it back
out
> again, but we can worry about that when and if it comes up.

Indeed. YAGNI is paramount. :-)


> hooks/backend.py:241: self.mixins.append(GuiMixin())
> I take it that you verified that it doesn't matter that the InstallMixin
happens
> last now?  Makes me a bit nervous...however, I reviewed the install methods
and
> it looks ok to me.  If you have done the same, and we've done reasonable qa,
> cool.

Yes, ordering does not seem an important requirement for the install phase.

[teknico, 2013-07-29 15:03:36]

Please take a look.

[teknico, 2013-07-30 10:15:06]

*** Submitted:

Refactor for easier builtin server integration.

Refactor the charm code to ease integration of the builtin server.
Detail of the changes, per file:

hooks/backend.py:

- order the mixins code as they are called, to ease readability;
- merge InstallMixin into GuiMixin;
- rename UpstartMixin to HaproxyApacheMixin;
- reimplement GuiMixin.install using the new functions from the split of
  hooks/utils.py#fetch_gui (see below);
- reimplement GuiMixin.start using the new functions from the split of
  hooks/utils.py#start_gui (see below);
- move here the chain and merge functions from hooks/utils.py, renaming
  them to be less generic;
- further simplify the backend constructor code.

hooks/utils.py:

- rename JUJU_GUI_SITE and JUJU_GUI_PORTS to APACHE_SITE and
  APACHE_PORTS respectively;
- split the start_gui function into compute_build_dir, write_gui_config
  and write_haproxy_config;
- split the fetch_gui function into fetch_gui_from_branch and
  fetch_gui_release;
- move the chain and merge functions to hooks/backend.py.

tests/test_backends.py:

- rename, add mocks and reorder to make tests pass again.

tests/test_utils.py:

- rename TestStartStop to TestStartImprovAgentGui;
- split the start_gui function tests in TestStartImprovAgentGui into
  tests for compute_build_dir, write_gui_config, write_haproxy_config
  and write_apache_config;

Sorry for the big diff. It was even bigger, but I moved some of the
changes to a future branch.

R=bac, gary.poster
CC=
https://codereview.appspot.com/12022044