Code review comment for lp:~frankban/juju-quickstart/old-style-bundles-regression
Revision history for this message
| Richard Harding (rharding) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
On 2015/03/10 10:20:07, frankban wrote:
> Please take a look.
https:/
> File quickstart/
https:/
> quickstart/
> 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.
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.
> ...
> 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.
> 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.
> 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:/
> quickstart/
> 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:/
> quickstart/
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:/
> quickstart/
> 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:/
> quickstart/
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:/
> quickstart/
the given
> reference cannot be
> On 2015/03/10 01:44:34, bac wrote:
> > typo: the an entity
> Done.
https:/
> quickstart/
> 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:/
> quickstart/
> 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:
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:
bundle:
> 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:/
> quickstart/
path):
> On 2015/03/10 01:18:32, rharding wrote:
> > get_bundle_data vs _retrieve_
Maybe we can
> > get the word 'parse' into the function name to help clarify that one
fetches
> > while the other actually parses?
> Done.
https:/
> File quickstart/
https:/
> quickstart/
> charmstore.
> 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_
> 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:
https:/
> quickstart/
> 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:/
> quickstart/
> On 2015/03/10 01:18:32, rharding wrote:
> > same here.
> This is only used here actually.
https:/
> File quickstart/
https:/
> quickstart/
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:/