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#newcode43
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/__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: },
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/__init__.py#newcode105
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.py#newcode99
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.py#newcode125
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.py#newcode159
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.py#newcode190
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.py#newcode25
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.py#newcode229
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.py#newcode238
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.py#newcode240
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.py#newcode136
server/guiserver/bundles/views.py:136: request.user, name, bundle,
bundle_id)
Could.  :)

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,
I've made them kwargs.

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):
On 2013/11/15 08:48:20, frankban wrote:
> as mock_fetch...

Done.

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)
Nice addition.

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.
Good eye.

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