After deploying a bundle increment counter.

After deploying a bundle increment counter.

The guiserver will make a GET request to the deployment counter incrementer
URL for the bundle.  This required accepting the bundle ID in the
Deployer/Import path.

Note neither the GUI nor quickstart have been updated to pass this value yet.

The charmworldurl is also now passed to the server via the command line.

QA: just deploy your favorite bundle and see that it works as it should.  You
can observe the bundle page in charmworld and see the counts remain at 0.

https://code.launchpad.net/~bac/charms/precise/juju-gui/increment-deployments/+merge/195317

(do not edit description out of merge proposal)

[bac, 2013-11-14 21:52:11]

Please take a look.

[frankban, 2013-11-15 08:48:20]

This branch looks very nice Brad.
LGTM with changes. I added a lot of comments below, but they are mostly
minors/nice to have, except for some documentation requests, code removal
requests, and a possible string encoding problem. I will QA this code when the
changes are pushed later. Thank you!

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/apps.py#newco...
server/guiserver/apps.py:43: options.charmworldurl)
It would be nice to expose the charmworld URL also in the /gui-server-info
InfoHandler. That's just a nice to have for another card I guess.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:99: },
In the other parts of this docstring, we aligned the closing brace of the value
with the key. <shrug>

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:105: parameter is optional in the case YAML
includes only one bundle.
Please document that the BundleID is completely optional, what that value means,
what it is used for and how it looks like (e.g. I guess it must not start with
"bundle:" because that's a bundle URL, but better not to guess).

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:99: self.bundle_ids = {}
Is bundle_ids really required? ISTM it's not really used.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:125: def import_bundle(self, user, name,
bundle, bundle_id, test_callback=None):
It would be nice to make bundle_id explicitly optional (i.e. bundle_id=None
here).

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:159: self.bundle_ids[deployment_id] = bundle_id
AFAICT these two lines can be removed as well.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:190: # Increment the Charmworld deployment
count upon successful
The count is increased also if the deployment failed. It seems ok to me, but we
should fix the comment.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:192: if bundle_id is not None:
Nice and simple.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:25: from tornado.httpclient import
AsyncHTTPClient
Very very minor: could you please move this import after the "from tornado
import..." one? Or just add "httpclient" to the list below?
<shrug>

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:229: url =
'{}api/3/bundle/{}/{}'.format(charmworld_url, bundle_id, path)
Please check that bundle_id is a string. I suspect it's a unicode string, so we
should encode it to utf-8 here (so that it is not implicitly ascii encoded) or
make the whole url a unicode string. The bundle id comes from the user and can
be anything.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:237: logging.error('URL: {}'.format(url))
If you decided to use a unicode url, then:
format(url.encode('utf-8')). Otherwise 'URL: {!r}'.format(url).

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:238: logging.error('Exception:
{}'.format(exc.message))
IIRC you could also use logging.exception(exc): this will print the full
traceback to the log file <shrug>.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:240: success = bool(resp.code == 200)
Isn't resp.code == 200 already a bool? If you just wanted to be more explicit,
sounds good.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/views...
server/guiserver/bundles/views.py:82: will be None if not given.
Very nice.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/views...
server/guiserver/bundles/views.py:136: request.user, name, bundle, bundle_id)
As mentioned, this could be bundle_id=bundle_id.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_base.py:111: self.user, 'bundle',
self.bundle, None,
If we make the import_bundle bundle_id parameter optional, all those None values
can be omitted, or we could pass them as kwargs, so that it's more clear what
the function requires.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_utils.py:450: with mock.patch(mock_path):
as mock_fetch...

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_utils.py:451: ok = yield
utils.increment_deployment_counter(bundle_id, None)
self.assertFalse(mock_fetch.called)?

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_views.py:212: # The following tests exercise
views._validate_import_params directly.
Thank you! ISTM that those three tests don't require @gen_test.

[bac, 2013-11-15 12:45:10]

Please take a look.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/apps.py#newco...
server/guiserver/apps.py:43: options.charmworldurl)
Good idea but I'd rather not do it in this branch.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:99: },
On 2013/11/15 08:48:20, frankban wrote:
> In the other parts of this docstring, we aligned the closing brace of the
value
> with the key. <shrug>

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__ini...
server/guiserver/bundles/__init__.py:105: parameter is optional in the case YAML
includes only one bundle.
On 2013/11/15 08:48:20, frankban wrote:
> Please document that the BundleID is completely optional, what that value
means,
> what it is used for and how it looks like (e.g. I guess it must not start with
> "bundle:" because that's a bundle URL, but better not to guess).

Done.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:99: self.bundle_ids = {}
Good catch.  I stopped using it when I began passing it directly to add_future.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:125: def import_bundle(self, user, name,
bundle, bundle_id, test_callback=None):
I don't see the reason.  It is optional in the params payload and will get
passed as None if there.  import_bundle is not called through any other path.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:159: self.bundle_ids[deployment_id] = bundle_id
On 2013/11/15 08:48:20, frankban wrote:
> AFAICT these two lines can be removed as well.

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/base....
server/guiserver/bundles/base.py:190: # Increment the Charmworld deployment
count upon successful
I'd really like to only increment on success.  I'll add a check for error is
None.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:25: from tornado.httpclient import
AsyncHTTPClient
On 2013/11/15 08:48:20, frankban wrote:
> Very very minor: could you please move this import after the "from tornado
> import..." one? Or just add "httpclient" to the list below?
> <shrug>

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:229: url =
'{}api/3/bundle/{}/{}'.format(charmworld_url, bundle_id, path)
On 2013/11/15 08:48:20, frankban wrote:
> Please check that bundle_id is a string. I suspect it's a unicode string, so
we
> should encode it to utf-8 here (so that it is not implicitly ascii encoded) or
> make the whole url a unicode string. The bundle id comes from the user and can
> be anything.

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:238: logging.error('Exception:
{}'.format(exc.message))
On 2013/11/15 08:48:20, frankban wrote:
> IIRC you could also use logging.exception(exc): this will print the full
> traceback to the log file <shrug>.

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/utils...
server/guiserver/bundles/utils.py:240: success = bool(resp.code == 200)
Yeah, I was just going for (overly) explicit.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/views...
server/guiserver/bundles/views.py:136: request.user, name, bundle, bundle_id)
Could.  :)

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_base.py:111: self.user, 'bundle',
self.bundle, None,
I've made them kwargs.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_utils.py:450: with mock.patch(mock_path):
On 2013/11/15 08:48:20, frankban wrote:
> as mock_fetch...

Done.

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_utils.py:451: ok = yield
utils.increment_deployment_counter(bundle_id, None)
Nice addition.

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles...
server/guiserver/tests/bundles/test_views.py:212: # The following tests exercise
views._validate_import_params directly.
Good eye.

[frankban, 2013-11-15 14:42:03]

Just one more comment, I'll QA asap.

https://codereview.appspot.com/26740043/diff/20001/server/guiserver/bundles/b...
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/26740043/diff/20001/server/guiserver/bundles/b...
server/guiserver/bundles/base.py:188: if bundle_id is not None and error is
None:
I the future has been cancelled then error is not defined here.

[bac, 2013-11-15 18:51:47]

*** Submitted:

After deploying a bundle increment counter.

The guiserver will make a GET request to the deployment counter incrementer
URL for the bundle.  This required accepting the bundle ID in the
Deployer/Import path.

Note neither the GUI nor quickstart have been updated to pass this value yet.

The charmworldurl is also now passed to the server via the command line.

QA: just deploy your favorite bundle and see that it works as it should.  You
can observe the bundle page in charmworld and see the counts remain at 0.

R=frankban
CC=
https://codereview.appspot.com/26740043

https://codereview.appspot.com/26740043/diff/20001/server/guiserver/bundles/b...
File server/guiserver/bundles/base.py (right):

https://codereview.appspot.com/26740043/diff/20001/server/guiserver/bundles/b...
server/guiserver/bundles/base.py:188: if bundle_id is not None and error is
None:
On 2013/11/15 14:42:03, frankban wrote:
> I the future has been cancelled then error is not defined here.

Done.