Merge lp://staging/~frankban/juju-quickstart/old-style-bundles-regression into lp://staging/juju-quickstart

Reviewers: mp+252353_code.launchpad.net,

Message:
Please take a look.

Description:
Fix the old-style bundle regression.

This branch fixes
https://bugs.launchpad.net/juju-quickstart/+bug/1429129
In essence, legacy bundles are converted by the
ingestion process like the following:
- if a basked includes multiple bundles, the resulting
   v4 bundle name is {basket-name}-{bundle-name} for
   each {bundle-name};
- if a basket only includes one bundle, the v4 bundle
   is just {basket-name}.
Previously quickstart always assumed the former: this
branch adds a check for the latter before exiting with
an error.

This branch also introduces a charmstore module in
quickstart. All the interactions between quickstart
and the charm store are now collected in this module.

As part of this refactoring, quickstart is now able
to distinguish HTTP 404 errors and all the other
generic IOErrors that can be raised when connecting
to the network.

Also simplified the logging format and bootstrap logging
earlier in the application execution.

Tests: `make check`.

QA:
install bundles with quickstart:
`devenv/bin/juju-quickstart {bundle}`
Try the following bundles:
- devenv/bin/juju-quickstart mediawiki-single
- devenv/bin/juju-quickstart u/bigdata-dev/apache-analytics-sql
- devenv/bin/juju-quickstart bundle:mediawiki/scalable
- devenv/bin/juju-quickstart
bundle:~landscape/landscape-dense-maas/landscape-dense-maas
- devenv/bin/juju-quickstart bundle:django/example-single

Those instead should return errors:
- devenv/bin/juju-quickstart mediawiki/trusty
- devenv/bin/juju-quickstart mediawiki-nosuch
- devenv/bin/juju-quickstart no-such
- devenv/bin/juju-quickstart bundle:no/such
- devenv/bin/juju-quickstart bundle:invalid
- devenv/bin/juju-quickstart
bundle:~landscape/landscape-dense-maas/landscape

https://code.launchpad.net/~frankban/juju-quickstart/old-style-bundles-regression/+merge/252353

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/215070043/

Affected files (+856, -266 lines):
   A [revision details]
   M quickstart/__init__.py
   M quickstart/app.py
   A quickstart/charmstore.py
   M quickstart/manage.py
   M quickstart/models/bundles.py
   M quickstart/models/references.py
   M quickstart/netutils.py
   M quickstart/settings.py
   M quickstart/tests/helpers.py
   M quickstart/tests/models/test_bundles.py
   M quickstart/tests/models/test_references.py
   M quickstart/tests/test_app.py
   A quickstart/tests/test_charmstore.py
   M quickstart/tests/test_netutils.py

LTGM QA OK

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

Thanks a lot for this fix. really cleans up a lot of the code!

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

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/mo...

LGTM. Thanks for the branch. Did not QA.

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#newcode68
quickstart/charmstore.py:68: Raise a NotFoundError the an entity with
the given reference cannot be
typo: the an entity

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#newcode187
quickstart/models/bundles.py:187:
Thank you for the super-clear regex construction.

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

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 particula...

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 has...

*** Submitted:

Fix the old-style bundle regression.

This branch fixes
https://bugs.launchpad.net/juju-quickstart/+bug/1429129
In essence, legacy bundles are converted by the
ingestion process like the following:
- if a basked includes multiple bundles, the resulting
   v4 bundle name is {basket-name}-{bundle-name} for
   each {bundle-name};
- if a basket only includes one bundle, the v4 bundle
   is just {basket-name}.
Previously quickstart always assumed the former: this
branch adds a check for the latter before exiting with
an error.

This branch also introduces a charmstore module in
quickstart. All the interactions between quickstart
and the charm store are now collected in this module.

As part of this refactoring, quickstart is now able
to distinguish HTTP 404 errors and all the other
generic IOErrors that can be raised when connecting
to the network.

Also simplified the logging format and bootstrap logging
earlier in the application execution.

Tests: `make check`.

QA:
install bundles with quickstart:
`devenv/bin/juju-quickstart {bundle}`
Try the following bundles:
- devenv/bin/juju-quickstart mediawiki-single
- devenv/bin/juju-quickstart u/bigdata-dev/apache-analytics-sql
- devenv/bin/juju-quickstart bundle:mediawiki/scalable
- devenv/bin/juju-quickstart
bundle:~landscape/landscape-dense-maas/landscape-dense-maas
- devenv/bin/juju-quickstart bundle:django/example-single

Those instead should return errors:
- devenv/bin/juju-quickstart mediawiki/trusty
- devenv/bin/juju-quickstart mediawiki-nosuch
- devenv/bin/juju-quickstart no-such
- devenv/bin/juju-quickstart bundle:no/such
- devenv/bin/juju-quickstart bundle:invalid
- devenv/bin/juju-quickstart
bundle:~landscape/landscape-dense-maas/landscape

R=jeff.pihach, rharding, bac
CC=
https://codereview.appspot.com/215070043

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

Thanks for the reviews!

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