Merge lp://qastaging/~frankban/juju-quickstart/jujucharms-bundles into lp://qastaging/juju-quickstart

Proposed by Francesco Banconi
Status: Merged
Merged at revision: 122
Proposed branch: lp://qastaging/~frankban/juju-quickstart/jujucharms-bundles
Merge into: lp://qastaging/juju-quickstart
Diff against target: 3303 lines (+1519/-940)
19 files modified
HACKING.rst (+2/-2)
quickstart/__init__.py (+1/-1)
quickstart/app.py (+19/-10)
quickstart/juju.py (+3/-3)
quickstart/jujutools.py (+22/-21)
quickstart/manage.py (+63/-90)
quickstart/models/bundles.py (+300/-88)
quickstart/models/references.py (+191/-70)
quickstart/netutils.py (+2/-2)
quickstart/settings.py (+4/-7)
quickstart/tests/functional/test_functional.py (+1/-1)
quickstart/tests/helpers.py (+8/-21)
quickstart/tests/models/test_bundles.py (+384/-196)
quickstart/tests/models/test_references.py (+386/-184)
quickstart/tests/test_app.py (+50/-32)
quickstart/tests/test_juju.py (+14/-11)
quickstart/tests/test_jujutools.py (+38/-28)
quickstart/tests/test_manage.py (+30/-172)
tox.ini (+1/-1)
To merge this branch: bzr merge lp://qastaging/~frankban/juju-quickstart/jujucharms-bundles
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+251156@code.qastaging.launchpad.net

Description of the change

Support retrieving bundles from charm store v4.

This branch implements the ability to deploy
bundles from the new charm store, retrieving
them with the v4 API.

Also introduce the new preferred bundle id
spelling, i.e. reflecting jujucharms.com paths,
like "mediawiki-single" or "u/who/bundle-name".

The old "bundle:basket/name" identifiers are
still supported but deprecated.
Deploying a bundle by specifying a directory
containing the YAML file is instead not
supported anymore.

Ok, after this brief summary let me take two
lines to really apologize for the huge diff.
While I was there, I refactored some historical
inconsistencies (e.g. models.Charm really being
just a charm or bundle reference), and I also
improved the bundle model API so that the work
is done in the model and not in manage as before.
There are a lot of tests too, and some documentation.
Nonetheless, let me say sorry again, this is
really too much stuff.

With this branch Juju Quickstart is quite ready for
the v4 world. The "deploy bundle" API call to the GUI
server still uses the legacy format, but the ugliness
of being backward compatible with namespaced bundles
is very restrained and implemented in private logic
in the bundles model module.

Tests: `make check`.

QA: run `devenv/bin/juju-quickstart` to deploy
new style and old style bundles, with both version
3 and 4 formats. Note that version 3 can only be
provided with arbitrary URLs or local files.

Thanks a lot!

https://codereview.appspot.com/207040043/

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :

Reviewers: mp+251156_code.launchpad.net,

Message:
Please take a look.

Description:
Support retrieving bundles from charm store v4.

This branch implements the ability to deploy
bundles from the new charm store, retrieving
them with the v4 API.

Also introduce the new preferred bundle id
spelling, i.e. reflecting jujucharms.com paths,
like "mediawiki-single" or "u/who/bundle-name".

The old "bundle:basket/name" identifiers are
still supported but deprecated.
Deploying a bundle by specifying a directory
containing the YAML file is instead not
supported anymore.

Ok, after this brief summary let me take two
lines to really apologize for the huge diff.
While I was there, I refactored some historical
inconsistencies (e.g. models.Charm really being
just a charm or bundle reference), and I also
improved the bundle model API so that the work
is done in the model and not in manage as before.
There are a lot of tests too, and some documentation.
Nonetheless, let me say sorry again, this is
really too much stuff.

With this branch Juju Quickstart is quite ready for
the v4 world. The "deploy bundle" API call to the GUI
server still uses the legacy format, but the ugliness
of being backward compatible with namespaced bundles
is very restrained and implemented in private logic
in the bundles model module.

Tests: `make check`.

QA: run `devenv/bin/juju-quickstart` to deploy
new style and old style bundles, with both version
3 and 4 formats. Note that version 3 can only be
provided with arbitrary URLs or local files.

Thanks a lot!

https://code.launchpad.net/~frankban/juju-quickstart/jujucharms-bundles/+merge/251156

(do not edit description out of merge proposal)

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

Affected files (+1520, -939 lines):
   M HACKING.rst
   A [revision details]
   M quickstart/__init__.py
   M quickstart/app.py
   M quickstart/juju.py
   M quickstart/jujutools.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/functional/test_functional.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
   M quickstart/tests/test_juju.py
   M quickstart/tests/test_jujutools.py
   M quickstart/tests/test_manage.py

Revision history for this message
Richard Harding (rharding) wrote :

Thanks for this branch. It looks really solid and loving the new
features.

I'm not a huge fan of the 'reference' but can't think of a great
replacement word so oh well.

I've got one question on the rpc call changes to the gui server.

Heads up that Kapil updated the python-jujuclient tonight and says
"pushed new py j client w/ iterator fix .. version incr math fixed as
well (0.50.1)"

So if you get time in the morning please see if you can do a round of QA
with that and we need to look at what it'll take to get that into the
stable PPA. I mentioned to him how we'd be able to help packaging and I
think the tox work done here in quickstart might be useful for
python-jujuclient and the deployer in the future.

https://codereview.appspot.com/207040043/diff/1/HACKING.rst
File HACKING.rst (right):

https://codereview.appspot.com/207040043/diff/1/HACKING.rst#newcode228
HACKING.rst:228: juju quickstart -e local mediawiki-single
<3 nice pretty command

https://codereview.appspot.com/207040043/diff/1/quickstart/__init__.py
File quickstart/__init__.py (right):

https://codereview.appspot.com/207040043/diff/1/quickstart/__init__.py#newcode48
quickstart/__init__.py:48: VERSION = (1, 7, 0)
what do you think of a 2.0? I guess it's not backward incompatible but
wondering if new charmstore/etc will be worthy of a big version update?

https://codereview.appspot.com/207040043/diff/1/quickstart/juju.py
File quickstart/juju.py (right):

https://codereview.appspot.com/207040043/diff/1/quickstart/juju.py#newcode68
quickstart/juju.py:68: params['Version'] = version
so this seems like a change to the call to the charm? Is Version going
to be supported in the charm then and name no longer required as a
param? This is the guiserver bits correct?

https://codereview.appspot.com/207040043/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/207040043/diff/1/quickstart/manage.py#newcode399
quickstart/manage.py:399: ''.format(jujucharms=settings.JUJUCHARMS_URL))
<3 this looks great.

https://codereview.appspot.com/207040043/diff/1/quickstart/tests/test_juju.py
File quickstart/tests/test_juju.py (right):

https://codereview.appspot.com/207040043/diff/1/quickstart/tests/test_juju.py#newcode207
quickstart/tests/test_juju.py:207: 'Version': 4,
Yea, so this is where I'm curious on the changes on the gui charm end.

https://codereview.appspot.com/207040043/

135. By Francesco Banconi

Bump version up to 2.0.0

Revision history for this message
Francesco Banconi (frankban) wrote :

Please take a look.

https://codereview.appspot.com/207040043/diff/1/HACKING.rst
File HACKING.rst (right):

https://codereview.appspot.com/207040043/diff/1/HACKING.rst#newcode228
HACKING.rst:228: juju quickstart -e local mediawiki-single
On 2015/02/27 03:00:09, rharding wrote:
> <3 nice pretty command

Indeed!

https://codereview.appspot.com/207040043/diff/1/quickstart/__init__.py
File quickstart/__init__.py (right):

https://codereview.appspot.com/207040043/diff/1/quickstart/__init__.py#newcode48
quickstart/__init__.py:48: VERSION = (1, 7, 0)
On 2015/02/27 03:00:10, rharding wrote:
> what do you think of a 2.0? I guess it's not backward incompatible but
wondering
> if new charmstore/etc will be worthy of a big version update?

Sounds good, bumped version up to 2.0.

https://codereview.appspot.com/207040043/diff/1/quickstart/juju.py
File quickstart/juju.py (right):

https://codereview.appspot.com/207040043/diff/1/quickstart/juju.py#newcode68
quickstart/juju.py:68: params['Version'] = version
On 2015/02/27 03:00:10, rharding wrote:
> so this seems like a change to the call to the charm? Is Version going
to be
> supported in the charm then and name no longer required as a param?
This is the
> guiserver bits correct?

Yes this is a call to the GUI server.
Version is already supported in the charm, as implemented by Madison.
Name has always been optional, only required when sending a YAML with
more than one bundle, which is never the case in quickstart.

https://codereview.appspot.com/207040043/

Revision history for this message
Richard Harding (rharding) wrote :

LGTM with the feedback thanks Francesco!

https://codereview.appspot.com/207040043/

Revision history for this message
Madison Scott-Clary (makyo) wrote :

LGTM - thanks for this, I think it's a great cleanup and implementation
of the newer urls.

https://codereview.appspot.com/207040043/

136. By Francesco Banconi

Update jujuclient to version 0.50.1.

Revision history for this message
Francesco Banconi (frankban) wrote :
Revision history for this message
Francesco Banconi (frankban) wrote :

*** Submitted:

Support retrieving bundles from charm store v4.

This branch implements the ability to deploy
bundles from the new charm store, retrieving
them with the v4 API.

Also introduce the new preferred bundle id
spelling, i.e. reflecting jujucharms.com paths,
like "mediawiki-single" or "u/who/bundle-name".

The old "bundle:basket/name" identifiers are
still supported but deprecated.
Deploying a bundle by specifying a directory
containing the YAML file is instead not
supported anymore.

Ok, after this brief summary let me take two
lines to really apologize for the huge diff.
While I was there, I refactored some historical
inconsistencies (e.g. models.Charm really being
just a charm or bundle reference), and I also
improved the bundle model API so that the work
is done in the model and not in manage as before.
There are a lot of tests too, and some documentation.
Nonetheless, let me say sorry again, this is
really too much stuff.

With this branch Juju Quickstart is quite ready for
the v4 world. The "deploy bundle" API call to the GUI
server still uses the legacy format, but the ugliness
of being backward compatible with namespaced bundles
is very restrained and implemented in private logic
in the bundles model module.

Tests: `make check`.

QA: run `devenv/bin/juju-quickstart` to deploy
new style and old style bundles, with both version
3 and 4 formats. Note that version 3 can only be
provided with arbitrary URLs or local files.

Thanks a lot!

R=rharding, matthew.scott
CC=
https://codereview.appspot.com/207040043

https://codereview.appspot.com/207040043/

Revision history for this message
Francesco Banconi (frankban) wrote :

Thanks a lot for the reviews!

https://codereview.appspot.com/207040043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches