Code review comment for lp:~rharding/charmworld/move-bundle-url

Revision history for this message
Richard Harding (rharding) wrote :

Reviewer comments added.

https://codereview.appspot.com/21370044/diff/20001/charmworld/routes.py
File charmworld/routes.py (right):

https://codereview.appspot.com/21370044/diff/20001/charmworld/routes.py#newcode57
charmworld/routes.py:57:
"/bundle/~{owner}/{basket}/{rev}/{bundle}/json")
this could probably go into some sort of /bundle/*bits but leaving as
simple case of moving the url for the moment.

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/api/__init__.py
File charmworld/views/api/__init__.py (right):

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/api/__init__.py#newcode159
charmworld/views/api/__init__.py:159: def _bundle_result(cls, bundle,
db, route_url):
route_url is needed to build the url to the person bundle json route
we're providing as an endpoint for the deployer/quickstart to use.

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_api.py
File charmworld/views/tests/test_api.py (right):

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_api.py#newcode768
charmworld/views/tests/test_api.py:768: self.enable_routes()
because of the need to generate the route for the deployer_file_url we
need to enable routes on more tests.

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_api.py#newcode841
charmworld/views/tests/test_api.py:841:
u'http://example.com/bundle/%7Ebac/bat/4/byobu/json',
I'm so sorry, but the linter...he hates me.

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_api.py#newcode1585
charmworld/views/tests/test_api.py:1585: class TestAPI3(TestAPI,
API3Mixin):
remove the stupid Z. Just skipping the bad test for now. See later
files.

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_misc.py
File charmworld/views/tests/test_misc.py (right):

https://codereview.appspot.com/21370044/diff/20001/charmworld/views/tests/test_misc.py#newcode54
charmworld/views/tests/test_misc.py:54: self.skipTest("See bug
#1237025
")
Skipping this as the new enable_routes() caused it to fail over here for
no good reason. Will look at as a follow up after bundles are released.

https://codereview.appspot.com/21370044/

« Back to merge proposal