Add support for OpenStack and Azure.

Add support for OpenStack and Azure.

Tests: `make check`

QA: `.venv/bin/python juju-quickstart -i`
Ensure you can successfully create an openstack/HP cloud
and an azure environment.
If you already subscribed to any of those, please
check everything works ok. If not, no problem,
I already bootstrapped HP Cloud and azure.
Since this is the 1.0 version, please ensure
the environment management works well, and in general
quickstart bootstraps the environments and 
deploys the GUI as expected.
Thank you!

https://code.launchpad.net/~frankban/juju-quickstart/more-cloud-providers/+merge/201616

(do not edit description out of merge proposal)

[frankban, 2014-01-14 16:01:35]

Please take a look.

[rharding, 2014-01-15 19:29:04]

Code looks good, a couple of comments. 

Starting QA.

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

https://codereview.appspot.com/52080044/diff/1/quickstart/__init__.py#newcode25
quickstart/__init__.py:25: VERSION = (1, 0, 0)
wahoo!

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

https://codereview.appspot.com/52080044/diff/1/quickstart/models/envs.py#newc...
quickstart/models/envs.py:529: fields.ChoiceField(
should this be a suggestion field to help users move along faster? Just suggest
a sensible default like US East which I think even things like the AMZ web ui
does? It's a suggestion field in the open stack example above.

https://codereview.appspot.com/52080044/diff/1/quickstart/tests/cli/test_form...
File quickstart/tests/cli/test_forms.py (right):

https://codereview.appspot.com/52080044/diff/1/quickstart/tests/cli/test_form...
quickstart/tests/cli/test_forms.py:174: class
TestCreateStringWidget(ChoicesTestsMixin, unittest.TestCase):
I think that the suggestion field is its own new class and should have its own
tests vs getting shoed into the String widget tests. This seems like a path down
like the one in the charm where the one suite of tests contained others that
weren't directly related.

[rharding, 2014-01-15 20:51:45]

In creating an Azure environment there's a space issue before 'saucy' in the
list of default series.

The dual nature of this is kind of strange. There's a lot of text/links there.
I'd probably leave off the 'left_empty' since that's just defaulting to precise.
Why not just prefill that in and let users change it?

Do you think we can skip the "Click the choices to auto-fill..." on each field?
Are the underline links a common enough/discoverable? If you don't have/use a
mouse does it help/matter?

The list of providers to create config for says 'openstack' but everything
inside of that references both openstack and HP. Should that original selection
be "openstack (HP Cloud)"?

I don't have openstack creds but looks sane.

I created 3 environments and removed two of them. One was the default. I'd
expect the last one left to be made the default now.

On the main interactive landing page it says "Use the links below to create new
environments." I don't really think of those as links. The underlined bits in
the choice fields are more "link like". I'd suggest rephrasing this as just
"Create new environments:"

Better yet, I'd try to match this up a bit. The main header can just be "Manage
Juju environments" and add a header to the top section as "Manage existing
environments:" and then the lower one could be the "Create a new environment". I
think it would flow and read a bit nicer.

When you select an existing environment the header is "name (type: local,
default). However that same information is listed below. Can it be simpler and
just be "name"?

I created a local environment, set it up, and got the gui loaded up on it. I
then changed it to be the default and selected 'use' think it would open my
existing session. Instead I got a request for my sudo password, it went through
the 'installing ppa...', asking for my ssh passphrase, and only after that did
it open. Can this shortcut to just open it up and detect/know the other steps
aren't necessary?

I removed my last environment and got the 'please create one' which is nice. The
bullet/selection for 'automatically create and bootstrap a local' does not have
a line of space between it and the above content like the other ones do. I'd
propose moving that down one newline.

One other note, removing the environment did not ask or do anything about
destroying it. Now that I've removed my config there's not a good way for me to
shut it down? Should we ask to destroy-environment when removing a config?

Overall this is really great and nice to use. I'm with-holding the L G T M
because I think the destroy question and the re-opening without asking might
require enough code changes (if there's agreement to make those changes) to want
to re-review.

[matthew.scott, 2014-01-16 00:09:46]

I agree that some of the points Rick brings up - notably destroying an
environment - are worth considering, though I think they may be worth their own
branches.  However, if that's the case, then I think we should withhold the move
to 1.0.0 until the destroy confirmation is made, since that's potentially big. 
That means, either include that here or push to another branch with 1.0.0;
otherwise, LGTM!  Thanks for the work!

[frankban, 2014-01-16 12:56:35]

Please take a look.

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

https://codereview.appspot.com/52080044/diff/1/quickstart/models/envs.py#newc...
quickstart/models/envs.py:529: fields.ChoiceField(
On 2014/01/15 19:29:04, rharding wrote:
> should this be a suggestion field to help users move along faster? Just
suggest
> a sensible default like US East which I think even things like the AMZ web ui
> does? It's a suggestion field in the open stack example above.

Suggestion fields are used to suggest a value while leaving the field open to
arbitrary values. The choice field here means validation is involved. AFAICT
Azure does not use US East as default.

https://codereview.appspot.com/52080044/diff/1/quickstart/tests/cli/test_form...
File quickstart/tests/cli/test_forms.py (right):

https://codereview.appspot.com/52080044/diff/1/quickstart/tests/cli/test_form...
quickstart/tests/cli/test_forms.py:174: class
TestCreateStringWidget(ChoicesTestsMixin, unittest.TestCase):
On 2014/01/15 19:29:04, rharding wrote:
> I think that the suggestion field is its own new class and should have its own
> tests vs getting shoed into the String widget tests. This seems like a path
down
> like the one in the charm where the one suite of tests contained others that
> weren't directly related.

I am not sure I understood this comment. The suggestion field itself already has
its own tests in test_fields.py. Here we are just testing that suggestions are
included in the string widget if required.

[frankban, 2014-01-16 12:57:14]

This is really good stuff Rick and Matthew.
Thank you both for your great reviews!
Please see the inline comments below, and,
if you can, please QA the new changes.

On 2014/01/15 20:51:45, rharding wrote:
> In creating an Azure environment there's a space issue before 'saucy' in the
> list of default series.

Unfortunately this is due to the fact we are
using a Grid widget to display choices.
This is done so that line wrapping works as
expected on small terminals. This can be better
seen, e.g., in the azure location choices.
There is no easy way to fix this in Urwid.
I agree it's a bit ugly but I also think
we can live with that for 1.0. What do you think?

> The dual nature of this is kind of strange. There's a lot of text/links there.
> I'd probably leave off the 'left_empty' since that's just defaulting to
precise.
> Why not just prefill that in and let users change it?

For optional fields, rather than pre-filling
values, I'd prefer to leave the field empty
(and consequently expose an option to empty
the field). Defaults can change over time,
and pre-filling with a value is semantically
different than not writing the field at all
in the envs.yaml file. The default series is
a good example of this. Leaving the field
empty means that juju-core, in a few months, will
bootstrap trusty machines. Explicitly selecting
precise avoids this behavior, and we want to
support both needs.

> Do you think we can skip the "Click the choices to auto-fill..." on each
field?
> Are the underline links a common enough/discoverable? If you don't have/use a
> mouse does it help/matter?

This makes the UI more explicit and has been
requested in previous reviews.

> The list of providers to create config for says 'openstack' but everything
> inside of that references both openstack and HP. Should that original
selection
> be "openstack (HP Cloud)"?

This is a very good point Rick.
Fixed, now the labels for new environments
are more human readable.

> I don't have openstack creds but looks sane.

Cool.

> I created 3 environments and removed two of them. One was the default. I'd
> expect the last one left to be made the default now.

Good idea, done.

> On the main interactive landing page it says "Use the links below to create
new
> environments." I don't really think of those as links. The underlined bits in
> the choice fields are more "link like". I'd suggest rephrasing this as just
> "Create new environments:"

Done.

> Better yet, I'd try to match this up a bit. The main header can just be
"Manage
> Juju environments" and add a header to the top section as "Manage existing
> environments:" and then the lower one could be the "Create a new environment".
I
> think it would flow and read a bit nicer.

Done.

> When you select an existing environment the header is "name (type: local,
> default). However that same information is listed below. Can it be simpler and
> just be "name"?

I find that summary useful, especially when the
environment is the default one: most of the times
you get all the info you require without having
to read the table below. I'll change it if you are
not convinced.

> I created a local environment, set it up, and got the gui loaded up on it. I
> then changed it to be the default and selected 'use' think it would open my
> existing session. Instead I got a request for my sudo password, it went
through
> the 'installing ppa...', asking for my ssh passphrase, and only after that did
> it open. Can this shortcut to just open it up and detect/know the other steps
> aren't necessary?

I am worried about the 'installing ppa...' bit.
It is expected that quickstart requires sudo privileges: the
strategy used to check if an environment is bootstrapped
is to try to bootstrap it and check the error. For this
reason sudo is always required when using local environments.
But it should not re-install the PPA: this should be done
only if lxc or juju are not available in the system.
I was not able to reproduce this, could you please
double check or give me more info to dupe?

> I removed my last environment and got the 'please create one' which is nice.
The
> bullet/selection for 'automatically create and bootstrap a local' does not
have
> a line of space between it and the above content like the other ones do. I'd
> propose moving that down one newline.

Done.

> One other note, removing the environment did not ask or do anything about
> destroying it. Now that I've removed my config there's not a good way for me
to
> shut it down? Should we ask to destroy-environment when removing a config?

As mentioned above, the UI does not know whether
an environment is bootstrapped or not. The only
way to retrieve that information is `juju status`,
which can take minutes to execute. This is why
we don't show whether an environment is bootstrapped
in the Urwid UI. If you accidentally removed a
bootstrapped environment from the envs.yaml file
you can still destroy it passing its name to
`juju destroy-environment`. Given this, and given
what you suggest is a little out of scope for
quickstart 1.0, I'd be inclined to avoid destroying
an environment when managing envs.yaml, and keep these
two concepts separated.
My proposal is the following, already implemented:
at the end of the quickstart execution we print
to the user the command he needs to use if he wants
to destroy the environment just created. This way
we also introduce to the user another useful command
he will need in his Juju experience.
How does it sound?

> Overall this is really great and nice to use. I'm with-holding the L G T M
> because I think the destroy question and the re-opening without asking might
> require enough code changes (if there's agreement to make those changes) to
want
> to re-review.

[rharding, 2014-01-16 14:13:41]

Thanks for the out of band email and updates. 

LGTM with the notes that this is 0.9 and 1.0 will have the checking for catching
Juju errors in bringing up the machine as a follow up card.

[frankban, 2014-01-16 14:30:40]

*** Submitted:

Add support for OpenStack and Azure.

Tests: `make check`

QA: `.venv/bin/python juju-quickstart -i`
Ensure you can successfully create an openstack/HP cloud
and an azure environment.
If you already subscribed to any of those, please
check everything works ok. If not, no problem,
I already bootstrapped HP Cloud and azure.
Since this is the 1.0 version, please ensure
the environment management works well, and in general
quickstart bootstraps the environments and 
deploys the GUI as expected.
Thank you!

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