Merge lp://staging/~frankban/charms/precise/juju-gui/guiserver-bundles-base-deployer into lp://staging/~juju-gui/charms/precise/juju-gui/trunk

Reviewers: mp+181824_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/12802045/

Affected files:
   A [revision details]
   M revision
   A server/guiserver/bundles/base.py
   M server/guiserver/bundles/blocking.py
   A server/guiserver/tests/bundles/test_base.py
   M server/guiserver/tests/bundles/test_blocking.py
   M server/guiserver/tests/helpers.py

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.py#newcode41
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/test_blocking.py
File server/guiserver/tests/bundles/test_blocking.py (right):

https://codereview.appspot.com/12802045/diff/1/server/guiserver/tests/bundles/test_blocking.py#newcode175
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/test_blocking.py#newcode181
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.py#newcode190
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.py#newcode306
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.

https://codereview.appspot.com/12802045/

LGTM with small doc cleanup

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

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/base.py#newcode21
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/base.py#newcode98
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/base.py#newcode105
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'?

https://codereview.appspot.com/12802045/

*** 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/base.py
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/12802045/diff/8001/server/guiserver/bundles/base.py#newcode21
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/base.py#newcode98
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/base.py#newcode105
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.

https://codereview.appspot.com/12802045/

Thank you both for the reviews.

https://codereview.appspot.com/12802045/