Support trusty environments.

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)

[frankban, 2014-04-23 16:14:37]

Please take a look.

[j.c.sackett, 2014-04-24 15:21:27]

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.

[frankban, 2014-04-24 15:33:54]

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.

[j.c.sackett, 2014-04-24 15:36:13]

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.

[rharding, 2014-04-24 17:33:19]

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#new...
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#ne...
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#newco...
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#...
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.py#new...
quickstart/settings.py:65: lambda: 0, {'precise': 80})
Can we just make this part of the dict for charm urls? I guess I'm thinking of a
supported_release_info: name: xx, url: yy, rev_for_bundles: 0?

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

https://codereview.appspot.com/90570044/diff/20001/quickstart/tests/test_util...
quickstart/tests/test_utils.py:319:
'http://manage.jujucharms.com/api/3/charm/trusty/juju-gui')
do we not have the version attached? Why do we have the version in the supported
url list?

[rharding, 2014-04-24 18:52:41]

QA is ok.

[rharding, 2014-04-24 18:57:27]

LGTM

[frankban, 2014-04-25 11:19:41]

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#new...
> 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_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?

JUJU_GUI_SUPPORTED_SERIES is also used to suggest the default-series in the
environments quickstart creates.
And we need to know in the program what Juju GUI charm series are available to
be deployed.
I am not worried about maintenance costs on this: I guess we will need to update
the value in ~2 years.

>
https://codereview.appspot.com/90570044/diff/20001/quickstart/cli/views.py#ne...
> 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

Indeed.

>
https://codereview.appspot.com/90570044/diff/20001/quickstart/manage.py#newco...
> 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.

Time will reduce the mess, this bundle support check will become less and less
relevant.
Even now I don't expect so many real environments out there to use such an
outdated charm.
I suppose that eventually we will just remove MINIMUM_REVISIONS_FOR_BUNDLES.

>
https://codereview.appspot.com/90570044/diff/20001/quickstart/models/envs.py#...
> 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.

The latter, and also, since quickstart is an opinionated tool, I don't think is
so bad to preset a sane default-series for the auto-generated env in general.

>
https://codereview.appspot.com/90570044/diff/20001/quickstart/settings.py#new...
> quickstart/settings.py:65: lambda: 0, {'precise': 80})
> Can we just make this part of the dict for charm urls? I guess I'm thinking of
a
> supported_release_info: name: xx, url: yy, rev_for_bundles: 0?

That was my first idea. Then I realized this is not important enough and could
be eventually removed (see above). I preferred to keep DEFAULT_CHARM_URLS as
simple as it is, and not influenced by a rev_for_bundles that only makes sense
on precise. Moreover the default dict does not force us to update multiple data
structures when we add a new charm series, so does not add any maintenance
costs.
What do you think?

>
https://codereview.appspot.com/90570044/diff/20001/quickstart/tests/test_util...
> quickstart/tests/test_utils.py:319:
> 'http://manage.jujucharms.com/api/3/charm/trusty/juju-gui')
> do we not have the version attached? Why do we have the version in the
supported
> url list?

Not sure I understood the question. Maybe you mean why we store charm URLS in
settings.DEFAULT_CHARM_URLS if we fetch the latest version from charmworld? If
so, as described in the comment in settings, the value we store in quickstart is
only used when charmworld is not reachable.

Thanks a lot to both of you for reviewing and QAing this branch!

[frankban, 2014-04-30 18:20:05]

*** 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