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

Proposed by Francesco Banconi
Status: Merged
Merged at revision: 135
Proposed branch: lp://qastaging/~frankban/juju-quickstart/uncommitted-bundles
Merge into: lp://qastaging/juju-quickstart
Diff against target: 665 lines (+332/-57)
13 files modified
quickstart/__init__.py (+1/-1)
quickstart/app.py (+62/-6)
quickstart/juju.py (+9/-0)
quickstart/jujugui.py (+28/-0)
quickstart/jujutools.py (+2/-5)
quickstart/manage.py (+31/-21)
quickstart/settings.py (+7/-2)
quickstart/tests/functional/test_functional.py (+8/-0)
quickstart/tests/test_app.py (+99/-16)
quickstart/tests/test_juju.py (+12/-0)
quickstart/tests/test_jujugui.py (+46/-0)
quickstart/tests/test_manage.py (+23/-2)
tox.ini (+4/-4)
To merge this branch: bzr merge lp://qastaging/~frankban/juju-quickstart/uncommitted-bundles
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code + qa Approve
Review via email: mp+261200@code.qastaging.launchpad.net

Commit message

Add support for uncommitted bundles.

Description of the change

Add support for uncommitted bundles.

Introduce the -u/--uncommitted flag, which enables
uncommitted bundle support.

Improve output messages and tokens handling.

Also update jujubundlelib dep to latest version.

TESTS:
`make fcheck` and wait a while for the functional tests
to complete.

QA:
- deploy bundles as usual:
  `devenv/bin/juju-quickstart mediawiki-single`;
- deploy uncommitted bundles:
  `devenv/bin/juju-quickstart -u u/openstack-charmers/openstack`;

https://codereview.appspot.com/247800043/

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

Reviewers: mp+261200_code.launchpad.net,

Message:
Please take a look.

Description:
Add support for uncommitted bundles.

Introduce the -u/--uncommitted flag, which enables
uncommitted bundle support.

Improve output messages and tokens handling.

Also update jujubundlelib dep to latest version.

TESTS:
`make fcheck` and wait a while for the functional tests
to complete.

QA:
- deploy bundles as usual:
   `devenv/bin/juju-quickstart mediawiki-single`;
- deploy uncommitted bundles:
   `devenv/bin/juju-quickstart -u u/openstack-charmers/openstack`;

https://code.launchpad.net/~frankban/juju-quickstart/uncommitted-bundles/+merge/261200

(do not edit description out of merge proposal)

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

Affected files (+329, -57 lines):
   A [revision details]
   M quickstart/__init__.py
   M quickstart/app.py
   M quickstart/juju.py
   M quickstart/jujugui.py
   M quickstart/jujutools.py
   M quickstart/manage.py
   M quickstart/settings.py
   M quickstart/tests/functional/test_functional.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_juju.py
   M quickstart/tests/test_jujugui.py
   M quickstart/tests/test_manage.py
   M tox.ini

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

LGTM, I think most of my questions are answered in some way or another.
I understand that this is really checking is the version of the charm
ready, not the gui, so checking the version.js isn't a perfect match. I
still can't help but feel that the charm revision is tied to the gui
release inside and maybe it's a better overall solution to this.
However, this is a perfectly reasonable way to go about it and should
work out most of the time.

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py#newcode659
quickstart/app.py:659: 'deployed Juju GUI charm ({}): requested bundle
deployment '
should we hint at a juju-gui upgrade here?

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py#newcode663
quickstart/app.py:663: print('requesting uncommitted deployment of {}
with the following '
"uncommitted deployment" to "uncomitted loading"? deployment just
strikes me as very...deployed.

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py#newcode679
quickstart/app.py:679: return response['Token']
does this open the gui url as it does when you just run juju-quickstart
without the deployment?

https://codereview.appspot.com/247800043/diff/1/quickstart/jujugui.py
File quickstart/jujugui.py (right):

https://codereview.appspot.com/247800043/diff/1/quickstart/jujugui.py#newcode62
quickstart/jujugui.py:62: def is_promulgated(reference):
I don't get this part. Can this not work if the gui you deployed is a
local deployment of the GUI?

Should this better be a check for a flag/config/call to the GUI source
instead? Can it just check something like a check against version.js in
some way?

https://codereview.appspot.com/247800043/diff/1/quickstart/jujutools.py
File quickstart/jujutools.py (right):

https://codereview.appspot.com/247800043/diff/1/quickstart/jujutools.py#newcode60
quickstart/jujutools.py:60: if not jujugui.is_promulgated(charm_ref):
oic, so we assume. I still wonder about using version.js vs
is_promulgated.

https://codereview.appspot.com/247800043/

145. By Francesco Banconi

Changes as per review.

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

Please take a look.

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py#newcode659
quickstart/app.py:659: 'deployed Juju GUI charm ({}): requested bundle
deployment '
On 2015/06/05 12:28:41, rharding wrote:
> should we hint at a juju-gui upgrade here?

Good idea, done.

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py#newcode663
quickstart/app.py:663: print('requesting uncommitted deployment of {}
with the following '
On 2015/06/05 12:28:41, rharding wrote:
> "uncommitted deployment" to "uncomitted loading"? deployment just
strikes me as
> very...deployed.

Done.

https://codereview.appspot.com/247800043/diff/1/quickstart/app.py#newcode679
quickstart/app.py:679: return response['Token']
On 2015/06/05 12:28:41, rharding wrote:
> does this open the gui url as it does when you just run
juju-quickstart without
> the deployment?

This token, if returned, is used to build the GUI URL.
Opening the GUI on the browser is done by manage.run.

https://codereview.appspot.com/247800043/diff/1/quickstart/jujutools.py
File quickstart/jujutools.py (right):

https://codereview.appspot.com/247800043/diff/1/quickstart/jujutools.py#newcode60
quickstart/jujutools.py:60: if not jujugui.is_promulgated(charm_ref):
On 2015/06/05 12:28:42, rharding wrote:
> oic, so we assume. I still wonder about using version.js vs
is_promulgated.

Well, some of the features we check (e.g. bundle deployment, uncommitted
bundles or new API endpoints) must be implemented principally on the
server (because qucikstart interacts mostly with the GUI server via its
API), and that's why in the cases we know the promulgated revision we
can safely check for their availability.
Given this is quickstart, I'd expect that we are dealing with
promulgated GUI charms 99% of the times, as you already mentioned. For
the remaining cases we trust the user and we expect her to be smart
enough to handle possible (and graceful) quickstart errors, which is the
only bad thing that can happen if the current customized charm does not
support a requested feature.

https://codereview.appspot.com/247800043/

Revision history for this message
Brad Crittenden (bac) wrote :

Francesco I can no longer log in to Rietveld so I'm making comments here only.

The code looks good. I'll do QA now.

Revision history for this message
Brad Crittenden (bac) wrote :

QA was OK.

review: Approve (code + qa)

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