Asynchronous watcher for bundle deployments.

Asynchronous watcher for bundle deployments.

This is the first of several branches implementing
the bundle deployments support in the GUI server.

This branch includes an AsyncWatcher implementation:
it will be used to observe the progress of bundle
deployments.

To give you a wider overview of what will be added,
I created the bundle module and added a comprehensive
documentation of the pieces that will be used to 
implement the functionality, and a description of the
request/response process.

Tests: `make unittest` from the root of the branch.

https://code.launchpad.net/~frankban/charms/precise/juju-gui/guiserver-async-watcher/+merge/180162

(do not edit description out of merge proposal)

[frankban, 2013-08-14 14:55:29]

Please take a look.

[benji, 2013-08-14 15:30:32]

LGTM.  I had a few questions or suggestions, but nothing major.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
File server/guiserver/bundles/__init__.py (right):

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:36: The watch and next interface methods
are used to retrieve information
Some newlines between these paragraphs would make it a little easier to read.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:77: - response handling: views -> response
I read the arrows as "calls"; is that right?

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:96: deploy a bundle from a URL.
It is not important for this branch, but Aaron and I have been thinking that
deploying a bundle from a URL may be something we want to avoid.  The reason
being that if the environment is behind an egress-filtering firewall, the GUI
server won't be able to fetch the URL.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:186: The Status can be one of the
following: 'scheduled', 'started' and 'completed'.
newline?

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:217: the second one is a successful
response:
The second response includes reference to an error; I think I see how that can
be a "successful" response (the request was processed correctly, but it contains
details about an error that occurred while deploying a bundle).  It might be
nice to have a clarifying comment that the "Error" key means that the request
generated an error, not that the response includes information about an error
that otherwise occurred.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/watchers.py
File server/guiserver/watchers.py (right):

https://codereview.appspot.com/12903044/diff/1/server/guiserver/watchers.py#n...
server/guiserver/watchers.py:115: return self._changes[-1]
I suppose that all this code is single-threaded so we don't have to worry about
races and locking.  Is my supposition correct?

[frankban, 2013-08-14 16:09:48]

Please take a look.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
File server/guiserver/bundles/__init__.py (right):

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:36: The watch and next interface methods
are used to retrieve information
On 2013/08/14 15:30:33, benji wrote:
> Some newlines between these paragraphs would make it a little easier to read.

Done.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:77: - response handling: views -> response
On 2013/08/14 15:30:33, benji wrote:
> I read the arrows as "calls"; is that right? 

Yes, added a comment.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:96: deploy a bundle from a URL.
On 2013/08/14 15:30:33, benji wrote:
> It is not important for this branch, but Aaron and I have been thinking that
> deploying a bundle from a URL may be something we want to avoid.  The reason
> being that if the environment is behind an egress-filtering firewall, the GUI
> server won't be able to fetch the URL.

Good to know, thank you.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:186: The Status can be one of the
following: 'scheduled', 'started' and 'completed'.
On 2013/08/14 15:30:33, benji wrote:
> newline?

Done.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:217: the second one is a successful
response:
On 2013/08/14 15:30:33, benji wrote:
> The second response includes reference to an error; I think I see how that can
> be a "successful" response (the request was processed correctly, but it
contains
> details about an error that occurred while deploying a bundle).  It might be
> nice to have a clarifying comment that the "Error" key means that the request
> generated an error, not that the response includes information about an error
> that otherwise occurred.

Done, also fixed a typo in many places of this document: 
s/DeployerId/DeploymentId.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/watchers.py
File server/guiserver/watchers.py (right):

https://codereview.appspot.com/12903044/diff/1/server/guiserver/watchers.py#n...
server/guiserver/watchers.py:115: return self._changes[-1]
On 2013/08/14 15:30:33, benji wrote:
> I suppose that all this code is single-threaded so we don't have to worry
about
> races and locking.  Is my supposition correct?

Yes it is. That's one of the greatest benefits in using Tornado (i.e. an
asynchronous framework).

[bac, 2013-08-14 18:16:53]

LGTM Francesco.  Very cool work.  Thanks.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
File server/guiserver/bundles/__init__.py (right):

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:19: Note that the following is a work in
progress. Some of the objects described
Maybe add an XXX as a reminder to delete this disclaimer.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:70: library belong here. Specifically this
module defines two function:
typo: functions

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:97: 
Are these "bundles" really what charmworld calls "baskets", i.e. a deployer
config file that contains multiple bundles?

If so it may make sense to adopt charmworld's nomenclature.

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

https://codereview.appspot.com/12903044/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/__init__.py:17: """Tests package for the Juju GUI
server bundles support."""
This GPL'd file reminds me of John Cage's work 4'33".

https://codereview.appspot.com/12903044/diff/1/server/guiserver/tests/test_wa...
File server/guiserver/tests/test_watchers.py (right):

https://codereview.appspot.com/12903044/diff/1/server/guiserver/tests/test_wa...
server/guiserver/tests/test_watchers.py:32: """Ensure the future is done and it
contains the expected results."""
Francesco you may have unwittingly written the most profound docstring ever. 
Bravo!

https://codereview.appspot.com/12903044/diff/1/server/guiserver/watchers.py
File server/guiserver/watchers.py (right):

https://codereview.appspot.com/12903044/diff/1/server/guiserver/watchers.py#n...
server/guiserver/watchers.py:115: return self._changes[-1]
On 2013/08/14 16:09:49, frankban wrote:
> On 2013/08/14 15:30:33, benji wrote:
> > I suppose that all this code is single-threaded so we don't have to worry
> about
> > races and locking.  Is my supposition correct?
> 
> Yes it is. That's one of the greatest benefits in using Tornado (i.e. an
> asynchronous framework).

It looks really clean and powerful.

[frankban, 2013-08-19 08:47:38]

*** Submitted:

Asynchronous watcher for bundle deployments.

This is the first of several branches implementing
the bundle deployments support in the GUI server.

This branch includes an AsyncWatcher implementation:
it will be used to observe the progress of bundle
deployments.

To give you a wider overview of what will be added,
I created the bundle module and added a comprehensive
documentation of the pieces that will be used to 
implement the functionality, and a description of the
request/response process.

Tests: `make unittest` from the root of the branch.

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

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
File server/guiserver/bundles/__init__.py (right):

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:19: Note that the following is a work in
progress. Some of the objects described
On 2013/08/14 18:16:53, bac wrote:
> Maybe add an XXX as a reminder to delete this disclaimer.

Done.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:70: library belong here. Specifically this
module defines two function:
On 2013/08/14 18:16:53, bac wrote:
> typo: functions

Done.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:97: 
On 2013/08/14 18:16:53, bac wrote:
> Are these "bundles" really what charmworld calls "baskets", i.e. a deployer
> config file that contains multiple bundles?
> 
> If so it may make sense to adopt charmworld's nomenclature.

+1 on using the same nomenclature. However, I am not sure this is the same
concept: the GUI server does not receive files. Is there a place where to find
these definitions? I'd like to align the nomenclature in a follow up branch.

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

https://codereview.appspot.com/12903044/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/__init__.py:17: """Tests package for the Juju GUI
server bundles support."""
On 2013/08/14 18:16:53, bac wrote:
> This GPL'd file reminds me of John Cage's work 4'33".

Heh, the silence is full of noise.

https://codereview.appspot.com/12903044/diff/1/server/guiserver/tests/test_wa...
File server/guiserver/tests/test_watchers.py (right):

https://codereview.appspot.com/12903044/diff/1/server/guiserver/tests/test_wa...
server/guiserver/tests/test_watchers.py:32: """Ensure the future is done and it
contains the expected results."""
On 2013/08/14 18:16:53, bac wrote:
> Francesco you may have unwittingly written the most profound docstring ever. 
> Bravo!

:-) I don't like the expectations based kind of thinking, but we must admit that
it works well in the unit tests world.

[frankban, 2013-08-19 08:52:07]

Hi Benji and Brad, thanks for your reviews!