GUI server: Deployer and DeployMiddleware.

GUI server: Deployer and DeployMiddleware.

This branch includes the bundles support
base classes. They are already described
in the bundles package docstring.

The diff is a bit long, sorry about that.
Most of the new code are tests.
The next branch will integrate and enable
the bundle support in the GUI server.

Tests: `make unittest` from the branch root.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/guiserver-bundles-base-deployer/+merge/181824

(do not edit description out of merge proposal)

[frankban, 2013-08-23 13:22:38]

Please take a look.

[benji, 2013-08-23 15:48:16]

LGTM.  The docstrings and inline comments are well done.

I just had a couple of small comments.

https://codereview.appspot.com/12802045/diff/1/server/guiserver/bundles/base.py
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/12802045/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:41: SUPPORTED_API_VERSIONS = ['go']
Because the first element of this list is used as the default API by
make_deployer, it would be nice to add a comment to that effect here.

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/bundles...
File server/guiserver/tests/bundles/test_blocking.py (right):

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_blocking.py:175: self.apiurl, self.password,
self.name, self.bundle)
Since I don't see any assertions, I don't understand how this shows that the
import fails.

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_blocking.py:181: # Ensure JUJU_HOME is
included in the context when the Importer
I would be inclined to assert that the test's pre-condition is true.  E.g.,

    assert not os.path.isdir(juju_home), 'directory should not exist'

(I used a non-test assert to communicate to the reader that if this fails it is
a test error, as opposed to a test failure.)

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/helpers.py
File server/guiserver/tests/helpers.py (right):

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/helpers...
server/guiserver/tests/helpers.py:190: def make_deployer(self, apiversion=None):
Given that base.SUPPORTED_API_VERSIONS doesn't change during program execution
you could be slightly sneaky here and set it as the default, like so: 

    apiversion=base.SUPPORTED_API_VERSIONS[0]

Oh, and shouldn't it be "api_version" instead of all together?

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/helpers...
server/guiserver/tests/helpers.py:306: return self._call_args
Maybe this is overly paranoid for test code, but I would return
list(self._call_args) to avoid the chance that the caller would mutate the list
after the fact.  Or maybe just a note in the docstring would be sufficient.

[frankban, 2013-08-23 16:06:59]

Please take a look.

https://codereview.appspot.com/12802045/diff/1/server/guiserver/bundles/base.py
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/12802045/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:41: SUPPORTED_API_VERSIONS = ['go']
On 2013/08/23 15:48:16, benji wrote:
> Because the first element of this list is used as the default API by
> make_deployer, it would be nice to add a comment to that effect here.

Done.

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/bundles...
File server/guiserver/tests/bundles/test_blocking.py (right):

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_blocking.py:175: self.apiurl, self.password,
self.name, self.bundle)
On 2013/08/23 15:48:16, benji wrote:
> Since I don't see any assertions, I don't understand how this shows that the
> import fails.

The assertions are done by the self.overlapping_services
context manager. But you are right, that wasn't clear
enough. Renamed the cm to "assert_overlapping_services".

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_blocking.py:181: # Ensure JUJU_HOME is
included in the context when the Importer
On 2013/08/23 15:48:16, benji wrote:
> I would be inclined to assert that the test's pre-condition is true.  E.g.,
> 
>     assert not os.path.isdir(juju_home), 'directory should not exist'
> 
> (I used a non-test assert to communicate to the reader that if this fails it
is
> a test error, as opposed to a test failure.)

Done, good suggestion and explanation, thank you.

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/helpers.py
File server/guiserver/tests/helpers.py (right):

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/helpers...
server/guiserver/tests/helpers.py:190: def make_deployer(self, apiversion=None):
On 2013/08/23 15:48:16, benji wrote:
> Given that base.SUPPORTED_API_VERSIONS doesn't change during program execution
> you could be slightly sneaky here and set it as the default, like so: 
> 
>     apiversion=base.SUPPORTED_API_VERSIONS[0]
> 
> Oh, and shouldn't it be "api_version" instead of all together?

Done. I use apiversion here because that's how the positional
argument is called in the Deployer initializer.

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/helpers...
server/guiserver/tests/helpers.py:306: return self._call_args
On 2013/08/23 15:48:16, benji wrote:
> Maybe this is overly paranoid for test code, but I would return
> list(self._call_args) to avoid the chance that the caller would mutate the
list
> after the fact.  Or maybe just a note in the docstring would be sufficient.

Done.

[bac, 2013-08-23 16:56:43]

LGTM with small doc cleanup

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/ba...
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/ba...
server/guiserver/bundles/base.py:21: and the DeployMiddleware, a glue code that
connects the WebSocket handler, the
s/a glue/glue

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/ba...
server/guiserver/bundles/base.py:98: raise gen.Return('unsupported API version')
Please include the bad version in the message.

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/ba...
server/guiserver/bundles/base.py:105: def import_bundle(self, user, name,
bundle, callback=None):
Perhaps rename the callback parameter to make it obvious it is only for testing.
 'test_callback'?

[frankban, 2013-08-26 08:00:32]

*** Submitted:

GUI server: Deployer and DeployMiddleware.

This branch includes the bundles support
base classes. They are already described
in the bundles package docstring.

The diff is a bit long, sorry about that.
Most of the new code are tests.
The next branch will integrate and enable
the bundle support in the GUI server.

Tests: `make unittest` from the branch root.

R=benji, bac
CC=
https://codereview.appspot.com/12802045

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/ba...
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/ba...
server/guiserver/bundles/base.py:21: and the DeployMiddleware, a glue code that
connects the WebSocket handler, the
On 2013/08/23 16:56:43, bac wrote:
> s/a glue/glue

Done.

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/ba...
server/guiserver/bundles/base.py:98: raise gen.Return('unsupported API version')
On 2013/08/23 16:56:43, bac wrote:
> Please include the bad version in the message.

Done.

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/ba...
server/guiserver/bundles/base.py:105: def import_bundle(self, user, name,
bundle, callback=None):
On 2013/08/23 16:56:43, bac wrote:
> Perhaps rename the callback parameter to make it obvious it is only for
testing.
>  'test_callback'?

Done.

[frankban, 2013-08-26 08:08:26]

Thank you both for the reviews.