On 2015/03/10 10:20:07, frankban wrote:
> Please take a look.


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):
> On 2015/03/10 01:18:32, rharding wrote:
> > can this just use the netutils NotFoundError?

> It can. But I didn't implement it lije that on purpose.
> My idea is that having the charmstore raise a charmstore.NotFoundError
is part
> of the contract. Instead, exposing its internal dependency on
netutils.urlread
> is not. This is why we basically mask the underlying exception.
> With this implementation, we discourage people to write something
like:

> try:
>     charmstore.get(...)
> except netutils.NotFoundError:  # This currently does not work.
>     ...

> The code above would establish a connection between the charmstore and
the
> netutils code, which is only an implementation detail. If, for
instance in the
> future we decide to replace the simplistic code in netutils with
something like
> requests or similar, the current API doesn't have to change, i.e. the
charmstore
> would still raise a charmstore.NotFoundError.
> In essence, clients using the charmstore API doesn't have to know (or
import)
> netutils, and vice versa.

> I considered the module namespace to be good enough to distinguish
between
> netutils.NotFoundError and charmstore.NotFoundError.

> That is the reasoning behind this implementation, and I agree it can
seem a
> repetition, but it's separation of concerns in my idea. Of course feel
free to
> disagree with the above.


https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode34
> quickstart/charmstore.py:34: def get(path):
> On 2015/03/10 01:18:32, rharding wrote:
> > are these internal? Should they be _get prefixed or anything as
internal use
> > methods?

> The idea is that this is public API. A client can use this function
when no
> higher level calls are available.


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

> Overriding the charmstore API URL is not supported currently. Do we
need to
> support that? If so, I'll create a card.


https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode51
> quickstart/charmstore.py:51: raise NotFoundError(msg)
> On 2015/03/10 01:18:32, rharding wrote:
> > 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?

> Not sure if I understand the question about using the traceback. See
above for
> the goal of having two separate exception types.


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:
> On 2015/03/10 01:18:32, rharding wrote:
> > here you mention a charm reference but this is particular to
bundles. Should
> > this be bundle reference?

> Why is this specific to bundles? The code here works with charms as
well. Am I
> missing something?


https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode68
> quickstart/charmstore.py:68: Raise a NotFoundError the an entity with
the given
> reference cannot be
> On 2015/03/10 01:44:34, bac wrote:
> > typo: the an entity

> Done.


https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode78
> quickstart/charmstore.py:78: def resolve(name, series=None):
> On 2015/03/10 01:18:32, rharding wrote:
> > 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.

> The goal of resolve is to retrieve the last revision of an entity. If
we already
> have it, I am not sure about why a client would call this function.
Could you
> expand on the failure handling part of your question? Right now, if
the entity
> does not exist, a NotFoundError is raised.


https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode116
> quickstart/charmstore.py:116: def get_legacy_bundle_data(reference):
> On 2015/03/10 01:18:32, rharding wrote:
> > 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?

> We need to, and I'll try to explain why.
> Quickstart bundles support works like this:
> - if a new-style bundle is provided (mediawiki-single or u/foo/bar)
then we
> never retrieve the legacy bundle data, and when sending the bundle to
the GUI
> server we wrap the decoded YAML with an arbitrary top level name.
> - if an old-style syntax is used (bundle:mediawiki/single) then we
have two
> possible cases:
>    1) the new bundle is named mediawiki-single (in the case multiple
bundle names
> were included in the basket)
>    or 2) the new bundle is named just mediawiki.
>    - quickstart tries case 1) first: if it succeeds, there is no need
to get the
> legacy data, and when sending the bundle to the GUI server we wrap the
decoded
> YAML with an arbitrary top level name.
>    - if case 1) returns a 404, we also try case 2), and this is when
this
> function is used, because we need to check that a "single" name is
included in
> the legacy YAML, so that bundle:mediawiki/single works but
bundle:mediawiki/xyz
> doesn't. We then flatten the retrieved legacy bundle and we wrap it
again as
> usual when sending it to the GUI server.

> So, the answer is yes, quickstart works only with flat bundle and only
nest it
> inside another level when sending it, and this is done in all cases.
But in the
> specific case above the legacy data is still required for validation.
The good
> thing is that the code handling legacy data and the flattening logic
is very
> confined in the bundles.py module, and the resulting bundle object is
unaware of
> multiple bundles or the basket/bundle name concepts.


https://codereview.appspot.com/215070043/diff/1/quickstart/charmstore.py#newcode134
> quickstart/charmstore.py:134: def _retrieve_bundle_data(reference,
path):
> On 2015/03/10 01:18:32, rharding wrote:
> > 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?

> Done.


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)
> On 2015/03/10 01:18:32, rharding wrote:
> > 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?

> As mentioned, this is done because we need to validate the provided
name. Using
> just get_bundle_data would work, but paradoxically would work in too
many cases:
> since "bundle:foo/bar" is translated to just "foo", we would have
bundle:foo/*
> as a valid input, e.g. "juju-quickstart bundle:foo/exterminate".


https://codereview.appspot.com/215070043/diff/1/quickstart/models/bundles.py#newcode400
> quickstart/models/bundles.py:400: def _open_yaml(content):
> On 2015/03/10 01:18:32, rharding wrote:
> > should this be shared with the yaml parsing code in the charmstore
methods?
> e.g.
> > a utils.py or the like around yaml management?

> Done.


https://codereview.appspot.com/215070043/diff/1/quickstart/models/bundles.py#newcode412
> quickstart/models/bundles.py:412: def _ensure_is_dict(data):
> On 2015/03/10 01:18:32, rharding wrote:
> > same here.

> This is only used here actually.


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
> On 2015/03/10 01:18:32, rharding wrote:
> > 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.

> Yes indeed.

LGTM with the one update we talked about this morning. Thanks for this
solid branch.

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