Code review comment for lp:~bac/charms/precise/juju-gui/increment-deployments

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

https://codereview.appspot.com/26740043/diff/1/server/guiserver/bundles/__init__.py#newcode99
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/__init__.py#newcode105
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.py#newcode99
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.py#newcode125
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.py#newcode159
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.py#newcode190
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.py#newcode192
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.py#newcode25
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.py#newcode229
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.py#newcode237
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.py#newcode238
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.py#newcode240
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.py#newcode82
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.py#newcode136
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/test_base.py
File server/guiserver/tests/bundles/test_base.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles/test_base.py#newcode111
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/test_utils.py
File server/guiserver/tests/bundles/test_utils.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles/test_utils.py#newcode450
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/test_utils.py#newcode451
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/test_views.py
File server/guiserver/tests/bundles/test_views.py (right):

https://codereview.appspot.com/26740043/diff/1/server/guiserver/tests/bundles/test_views.py#newcode212
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.

https://codereview.appspot.com/26740043/