Code review comment for lp:~frankban/juju-quickstart/old-style-bundles-regression

Thanks for this branch Francesco, I've mainly got one big question on
the legacy version of the call and some minor questions on just code
bits.

Will make sure to look asap in the morning. Let me know if anything here
is unclear.

https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py
File quickstart/charmstore.py (right):

https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode30
quickstart/charmstore.py:30: class NotFoundError(Exception):
can this just use the netutils NotFoundError?

https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode34
quickstart/charmstore.py:34: def get(path):
are these internal? Should they be _get prefixed or anything as internal
use methods?

https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode45
quickstart/charmstore.py:45: url = settings.CHARMSTORE_API +
path.lstrip('/')
can you confirm that the settings will allow overriding the api endpoint
via env var? Is that documented, maybe in the hacking doc?

https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode51
quickstart/charmstore.py:51: raise NotFoundError(msg)
Ok, so this is intentional to tell which kind of "NotFoundError" you've
got by if it's a netutils vs a charmstore.py one? Should we name them
different or just use the traceback?

https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode64
quickstart/charmstore.py:64: For instance, to retrieve the hash of a
charm reference, use the following:
here you mention a charm reference but this is particular to bundles.
Should this be bundle reference?

https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode78
quickstart/charmstore.py:78: def resolve(name, series=None):
what about specifying the version? The main concern is that if it
doesn't exist that the failure is consistent or clear to the user.

https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode116
quickstart/charmstore.py:116: def get_legacy_bundle_data(reference):
do we ever get the legacy bundle though now? I thought we only got the
updated bundle and then turned it into legacy format by nesting it
inside another level?

https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode134
quickstart/charmstore.py:134: def _retrieve_bundle_data(reference,
path):
get_bundle_data vs _retrieve_bundle_data was a bit unclear to me. Maybe
we can get the word 'parse' into the function name to help clarify that
one fetches while the other actually parses?

https://codereview.appspot.com/215070043/diff/1/quickstart/models/bundles.py
File quickstart/models/bundles.py (right):

https://codereview.appspot.com/215070043/diff/1/quickstart/models/bundles.py#newcode228
quickstart/models/bundles.py:228: data =
charmstore.get_legacy_bundle_data(reference)
I was expecting this to use the same 'get_bundle_data but with the
different reference using only the basket vs getting the orig.yaml file
using the get_legacy_bundle_data. Can you clarify this for me please?

https://codereview.appspot.com/215070043/diff/1/quickstart/models/bundles.py#newcode400
quickstart/models/bundles.py:400: def _open_yaml(content):
should this be shared with the yaml parsing code in the charmstore
methods? e.g. a utils.py or the like around yaml management?

https://codereview.appspot.com/215070043/diff/1/quickstart/models/bundles.py#newcode412
quickstart/models/bundles.py:412: def _ensure_is_dict(data):
same here.

https://codereview.appspot.com/215070043/diff/1/quickstart/settings.py
File quickstart/settings.py (right):

https://codereview.appspot.com/215070043/diff/1/quickstart/settings.py#newcode41
quickstart/settings.py:41: ('trusty', 'cs:trusty/juju-gui-21'),
<3 ty for the drive by here.

https://codereview.appspot.com/215070043/diff/1/quickstart/tests/models/test_bundles.py
File quickstart/tests/models/test_bundles.py (right):

https://codereview.appspot.com/215070043/diff/1/quickstart/tests/models/test_bundles.py#newcode110
quickstart/tests/models/test_bundles.py:110: # XXX frankban 2015-03-09:
remove this check once we get rid of the
maybe we should tag these in a way? I guess the comment is consistent
with the world 'charmworld' in it to serve as a way to grep out all of
this that needs cleanup down the road.

https://codereview.appspot.com/215070043/