Code review comment for lp:~frankban/juju-quickstart/old-style-bundles-regression
Revision history for this message
| Francesco Banconi (frankban) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
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.
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:
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:
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/
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.
https:/