Code review comment for lp:~frankban/juju-quickstart/more-cloud-providers

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.

https://codereview.appspot.com/52080044/