Merge lp://staging/~frankban/juju-quickstart/trusty-charm into lp://staging/juju-quickstart

Reviewers: mp+216921_code.launchpad.net,

Message:
Please take a look.

Description:
Support trusty environments.

Add the ability to deploy the trusty charm.
Introduced the concept of multiple supported
series for the Juju GUI charm.

Split the app.deploy_gui function in two
separate function:
- check_environment inspects the environment
   and returns the data required to deploy the GUI;
- deploy_gui's only responsibility is to
   return when the GUI service is deployed/exposed
   and the unit created.

Include the default-series field in the auto-generated
local environment. This is the environment that
quickstart offers to automatically create when no other
environments are found.

Also propose "trusty" as the default series when manually
creating new environments.

Bump version up: while this branch
incidentally fixes bug 1306537 [1],
the ability to deploy the GUI on trusty
can be considered a new feature.

My apologies for the long diff.

Tests: `make check`.

QA:
Use quickstart like the following:
   `.venv/bin/python juju-quickstart [-i]`.

You should be able to deploy the trusty GUI charm.

If you are on trusty, the trusty charm should be deployed
when the default-series field is missing.
This must be tested also using the local provider, in which case
Juju is currently not able to deploy precise charms when the
bootstrap node is trusty (bug 1306537).

In general quickstart should deploy the charm series corresponding
to the bootstrap node series: so on trusty environments the trusty
charm should be installed, on precise environments the precise one.

This way, at least when the bootstrap node series is precise or trusty,
quickstart is able to add the GUI unit to machine 0. You can test it
using, e.g. an ec2 environment.

This is true also when providing a custom charm URL, e.g.:
   `.venv/bin/python juju-quickstart --gui-charm-url
cs:~juju-gui/trusty/juju-gui-1`

In all the other cases, quickstart uses the trusty charm. You can test
this
by using quickstart with an ec2 environment having "default-series:
saucy":
a trusty GUI should be deployed on machine 1.

Two final checks:
- try to create a new environment with quickstart: the default-series
   field should be pre-filled with "trusty";
- move temporarily your environments.yaml somewhere else, and let
quickstart
   auto-generate a local environment for you: the deployment process
should
   succeed and the environment should include the "trusty" default
series.

Thanks a lot for all of this, and sorry for the long QA: this is going
to
be released in trusty, so your efforts are really appreciated!

[1] https://bugs.launchpad.net/juju-core/+bug/1306537

https://code.launchpad.net/~frankban/juju-quickstart/trusty-charm/+merge/216921

(do not edit description out of merge proposal)

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

Affected files (+605, -294 lines):
   A [revision details]
   M quickstart/__init__.py
   M quickstart/app.py
   M quickstart/cli/views.py
   M quickstart/manage.py
   M quickstart/models/envs.py
   M quickstart/settings.py
   M quickstart/tests/cli/test_views.py
   M quickstart/tests/helpers.py
   M quickstart/tests/models/test_envs.py
   M q...

LGTM, Francesco. I have one comment that is probably more about clearing
up my own ignorance then an issue with the branch.

Thanks.

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

https://codereview.appspot.com/90570044/diff/1/quickstart/app.py#newcode387
quickstart/app.py:387: charm_url = service_data['CharmURL']
Is it impossible for a situation to rise where both service_data and
charm_url are not None? It would seem in that instance the provided
charm_url is going to get clobbered by service_data, in which case I
just want to verify that's the desired behavior.

https://codereview.appspot.com/90570044/

Please take a look.

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

https://codereview.appspot.com/90570044/diff/1/quickstart/app.py#newcode387
quickstart/app.py:387: charm_url = service_data['CharmURL']
On 2014/04/24 15:21:27, j.c.sackett wrote:
> Is it impossible for a situation to rise where both service_data and
charm_url
> are not None? It would seem in that instance the provided charm_url is
going to
> get clobbered by service_data, in which case I just want to verify
that's the
> desired behavior.

Yeah, basically if service_data is not None it means a GUI is already
deployed, and we ignore the provided charm URL. I'll add a comment to
clarify that.

https://codereview.appspot.com/90570044/

On 2014/04/24 15:33:54, frankban wrote:

> Yeah, basically if service_data is not None it means a GUI is already
deployed,
> and we ignore the provided charm URL. I'll add a comment to clarify
that.

Awesome, thanks.

https://codereview.appspot.com/90570044/

The code looks good. I can't help but feel like we're doing things to
make it work, but we should be bringing something to the attention of
the juju store, the ecosystem, etc about how this is becoming a mess to
maintain. I want to work on a list that details what we're doing and why
we have to do each thing and bring it to the attention of others to see
if we can effect some change on this.

For instance, if submitting an existing charm to a new revision did not
reset the juju core store revno, then it would be possible to keep a
constant "this feature is < ver 80" setting and avoid some need for
this.

The colocation of a charm of one series with a machine bootstrapped on
another seems like something we shouldn't have to worry about.

Will QA.

https://codereview.appspot.com/90570044/diff/20001/quickstart/__init__.py
File quickstart/__init__.py (right):

https://codereview.appspot.com/90570044/diff/20001/quickstart/__init__.py#newcode48
quickstart/__init__.py:48: VERSION = (1, 4, 0)
Can this be a 1.3.2? It's not backward incompatible is it? I worry that
bumping the 'major' revision to 4 will make it harder for us to get this
update into the trusty package as a 'bug fix' release.

https://codereview.appspot.com/90570044/diff/20001/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/90570044/diff/20001/quickstart/app.py#newcode373
quickstart/app.py:373: if bootstrap_node_series in
settings.JUJU_GUI_SUPPORTED_SERIES:
I'm hesitant about defining 'supported series' as it requires us to
maintain this as the package if copied from release to release.

The juju core store is being updated to support series sanity checks and
soon charms will be able to declare a series. Can we not just let them
do the work rather than build it in here?

https://codereview.appspot.com/90570044/diff/20001/quickstart/cli/views.py
File quickstart/cli/views.py (right):

https://codereview.appspot.com/90570044/diff/20001/quickstart/cli/views.py#newcode236
quickstart/cli/views.py:236: # series matches one of the series
supported by the GUI.
I see, there's the colocation issue at heart here. Hmmm

https://codereview.appspot.com/90570044/diff/20001/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/90570044/diff/20001/quickstart/manage.py#newcode158
quickstart/manage.py:158: charm.revision <
settings.MINIMUM_REVISIONS_FOR_BUNDLES[charm.series]
Oh, and the revision count reset with trusty so we can't know the same
bzr start point. This is kind of a mess.

https://codereview.appspot.com/90570044/diff/20001/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/90570044/diff/20001/quickstart/models/envs.py#newcode351
quickstart/models/envs.py:351: # Workaround for bug 1306537: force tools
for the most recent Juju GUI
Should we make this an XXX to remove this when they fix their bug and
release? Or maybe we can't do that because the bad versions are out
there as stable/supported releases.

https://codereview.appspot.com/90570044/diff/20001/quickstart/settings.py
File quickstart/settings.py (right):

https://codereview.appspot.com/90570044/diff/20001/quickstart/settings....

QA is ok.

https://codereview.appspot.com/90570044/

LGTM

https://codereview.appspot.com/90570044/

On 2014/04/24 17:33:19, rharding wrote:
> The code looks good. I can't help but feel like we're doing things to
make it
> work, but we should be bringing something to the attention of the juju
store,
> the ecosystem, etc about how this is becoming a mess to maintain. I
want to work
> on a list that details what we're doing and why we have to do each
thing and
> bring it to the attention of others to see if we can effect some
change on this.

Well, juju-core does a great job deploying the right charm version when
you use the CLI.
E.g. "juju deploy juju-gui" usually does the right thing.
When using the API (like in quickstart) you must be quite more specific:
the ServiceDeploy API call requires a complete charm URL, including
series and revision. This means that it's client responsibility to check
whether a specific series/revision can be deployed to a specific
machine. And for this reason, API client code cannot be as
straightforward as we would like. Add to this the "idempotent" feature
of quickstart, in which we want to reuse an already deployed
service/unit, and we end up with a slightly complex code. Not
overcomplicated IMHO, as the code paths are still quite easy to follow,
but there is room for improvement (e.g. make ServiceDeploy not require
the charm revision etc.).

> For instance, if submitting an existing charm to a new revision did
not reset
> the juju core store revno, then it would be possible to keep a
constant "this
> feature is < ver 80" setting and avoid some need for this.

Yeah, that's true. I think juju considers precise/juju-gui and
trusty/juju-gui as two completely different charms. And it's not wrong
in this assumption, given the current charm repository design requires
different charm series to be in different Bazaar repositories. The fact
that we use the same code base for both charms is incidental, but in
general juju cannot assume precise/juju-gui-42 == trusty/juju-gui-42.

> The colocation of a charm of one series with a machine bootstrapped on
another
> seems like something we shouldn't have to worry about.

Colocation has its limits IMHO. One of those limits is over-exposing the
series concept to the user. I.e. when you "juju deploy --to" a machine,
the series must match and this currently means:
- if you use a specific series ("juju deploy to cs:{series}/wordpress
--to {a trusty machine}"), then {series} must be trusty;
- if you just use the charm name ("juju deploy wordpress --to {a trusty
machine}") then a trusty wordpress charm must be available in the charm
store.

https://codereview.appspot.com/90570044/diff/20001/quickstart/__init__.py#newcode48
> quickstart/__init__.py:48: VERSION = (1, 4, 0)
> Can this be a 1.3.2? It's not backward incompatible is it? I worry
that bumping
> the 'major' revision to 4 will make it harder for us to get this
update into the
> trusty package as a 'bug fix' release.

I bumped the minor version because I considered trusty support a new
feature. But having this accepted in trusty is a more important goal, so
I'll change this.

https://codereview.appspot.com/90570044/diff/20001/quickstart/app.py#newcode373
> quickstart/app.py:373: if bootstrap_node_series in
> settings.JUJU_GUI...

On Fri, 25 Apr 2014, Francesco Banconi wrote:

> Well, juju-core does a great job deploying the right charm version when
> you use the CLI.
> E.g. "juju deploy juju-gui" usually does the right thing.
> When using the API (like in quickstart) you must be quite more specific:
> the ServiceDeploy API call requires a complete charm URL, including
> series and revision. This means that it's client responsibility to check
> whether a specific series/revision can be deployed to a specific
> machine. And for this reason, API client code cannot be as
> straightforward as we would like. Add to this the "idempotent" feature
> of quickstart, in which we want to reuse an already deployed
> service/unit, and we end up with a slightly complex code. Not
> overcomplicated IMHO, as the code paths are still quite easy to follow,
> but there is room for improvement (e.g. make ServiceDeploy not require
> the charm revision etc.).

Yes, none if this is meant to say the code itself isn't right and correct.
It just strikes me that these are pain points other charm authors will
face as we have this support for charms in multiple series.

> Yeah, that's true. I think juju considers precise/juju-gui and
> trusty/juju-gui as two completely different charms. And it's not wrong
> in this assumption, given the current charm repository design requires
> different charm series to be in different Bazaar repositories. The fact
> that we use the same code base for both charms is incidental, but in
> general juju cannot assume precise/juju-gui-42 == trusty/juju-gui-42.

Yes, I think I'm mixing up the issues of quickstart with the charm itself.
It feels like there's a hole in charm authoring around this, but we've not
had the issue in the actual charm development, just quickstart.

> Colocation has its limits IMHO. One of those limits is over-exposing the
> series concept to the user. I.e. when you "juju deploy --to" a machine,
> the series must match and this currently means:
> - if you use a specific series ("juju deploy to cs:{series}/wordpress
> --to {a trusty machine}"), then {series} must be trusty;
> - if you just use the charm name ("juju deploy wordpress --to {a trusty
> machine}") then a trusty wordpress charm must be available in the charm
> store.

Yes, I like how quickstart 'does the right thing' based on if you give it a
compatible charm or not. I was just pondering if juju should 'do the right
thing' as well. Then again, if you tell it to deploy into a mixed series
install/setup I guess it would just error and not bring up a new machine
like quickstart is doing here.

> https://codereview.appspot.com/90570044/diff/20001/quickstart/__init__.py#newcode48
> > quickstart/__init__.py:48: VERSION = (1, 4, 0)
> > Can this be a 1.3.2? It's not backward incompatible is it? I worry
> that bumping
> > the 'major' revision to 4 will make it harder for us to get this
> update into the
> > trusty package as a 'bug fix' release.
>
> I bumped the minor version because I considered trusty support a new
> feature. But having this accepted in trusty is a more important goal, so
> I'll change this.

I'm only guessing here, we can feel free to run it by Robie or James to be
sure.

> ...

*** Submitted:

Support trusty environments.

Add the ability to deploy the trusty charm.
Introduced the concept of multiple supported
series for the Juju GUI charm.

Split the app.deploy_gui function in two
separate function:
- check_environment inspects the environment
   and returns the data required to deploy the GUI;
- deploy_gui's only responsibility is to
   return when the GUI service is deployed/exposed
   and the unit created.

Include the default-series field in the auto-generated
local environment. This is the environment that
quickstart offers to automatically create when no other
environments are found.

Also propose "trusty" as the default series when manually
creating new environments.

Bump version up: while this branch
incidentally fixes bug 1306537 [1],
the ability to deploy the GUI on trusty
can be considered a new feature.

My apologies for the long diff.

Tests: `make check`.

QA:
Use quickstart like the following:
   `.venv/bin/python juju-quickstart [-i]`.

You should be able to deploy the trusty GUI charm.

If you are on trusty, the trusty charm should be deployed
when the default-series field is missing.
This must be tested also using the local provider, in which case
Juju is currently not able to deploy precise charms when the
bootstrap node is trusty (bug 1306537).

In general quickstart should deploy the charm series corresponding
to the bootstrap node series: so on trusty environments the trusty
charm should be installed, on precise environments the precise one.

This way, at least when the bootstrap node series is precise or trusty,
quickstart is able to add the GUI unit to machine 0. You can test it
using, e.g. an ec2 environment.

This is true also when providing a custom charm URL, e.g.:
   `.venv/bin/python juju-quickstart --gui-charm-url
cs:~juju-gui/trusty/juju-gui-1`

In all the other cases, quickstart uses the trusty charm. You can test
this
by using quickstart with an ec2 environment having "default-series:
saucy":
a trusty GUI should be deployed on machine 1.

Two final checks:
- try to create a new environment with quickstart: the default-series
   field should be pre-filled with "trusty";
- move temporarily your environments.yaml somewhere else, and let
quickstart
   auto-generate a local environment for you: the deployment process
should
   succeed and the environment should include the "trusty" default
series.

Thanks a lot for all of this, and sorry for the long QA: this is going
to
be released in trusty, so your efforts are really appreciated!

[1] https://bugs.launchpad.net/juju-core/+bug/1306537

R=
CC=
https://codereview.appspot.com/90570044

https://codereview.appspot.com/90570044/