Implement the remove env functionality.

Implement the remove env functionality.

It is now possible to remove environments
using the interactive session.

A confirmation dialog prevents the user
to accidentally remove an environment.

Tests: `make check`.
QA: start the demo app 
(`make` and `./cli-app-demo.py).
Remove environments at will: exiting
from the app you should see the
new environments YAML without the
environments you deleted.
Any UX suggestion welcome.
Thank you!

https://code.launchpad.net/~frankban/juju-quickstart/env-manage-remove-environment/+merge/199698

(do not edit description out of merge proposal)

[frankban, 2013-12-19 18:38:21]

Please take a look.

[gary.poster, 2013-12-19 19:11:13]

LGTM.

The warning says "This action cannot be undone" but really it won't take effect
if the person exits early (^X), right?  Because we only will save at the end? 
If I'm right, maybe the following would work?

"""
Remove the private-cloud-errors-environment

This change will be saved once you choose an environment to deploy.  The action
cannot be undone.
"""

I deleted my default.  What is supposed to happen then?  You don't have a
default anymore?  (That's what happens now.)

QA good with those notes.

Thanks!

Gary

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py
File quickstart/cli/ui.py (right):

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py#newcode129
quickstart/cli/ui.py:129: original_contents = app.get_contents()
The way you can use this with the dismiss thunk is cool.  Feels nice.

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

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:277: ('control alert', 'remove'),
ui.thunk(confirm_removal, env_data)),
cool.  I really like how this and the show_dialog actions work.

https://codereview.appspot.com/44310043/diff/1/quickstart/tests/cli/helpers.py
File quickstart/tests/cli/helpers.py (right):

https://codereview.appspot.com/44310043/diff/1/quickstart/tests/cli/helpers.p...
quickstart/tests/cli/helpers.py:39: def emit(self, widget):
Don't you have this in another base class?  If so, maybe this ought to be a
standalone function?  it does not require "self."

[bac, 2013-12-19 20:29:16]

LGTM

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py
File quickstart/cli/ui.py (right):

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py#newcode125
quickstart/cli/ui.py:125: the function tries to generate suitable defaults.
If the units are pixels please say so.

[frankban, 2013-12-20 10:23:28]

On 2013/12/19 19:11:13, gary.poster wrote:
> LGTM.
> 
> The warning says "This action cannot be undone" but really it won't take
effect
> if the person exits early (^X), right?  Because we only will save at the end? 
> If I'm right, maybe the following would work?

In the current implementation, you pass a save_callable to the views, which is
called when required, e.g. when the user removes an environment.
The save_callable accepts an env_db and it's not yet implemented:
it will likely be something like functools.partial(envs.save, env_file).
We can decide to change this, but I think this strategy gives us two things:
1) the UX is less surprising for the user: when he performs an action, that
action is done, and not just scheduled;
2) it is possible to use the quickstart interactive session to manage
environments without starting Juju: the user starts quickstart, 
creates/edits/removes environments and then exits (^X).

> 
> """
> Remove the private-cloud-errors-environment
> 
> This change will be saved once you choose an environment to deploy.  The
action
> cannot be undone.
> """
> 
> I deleted my default.  What is supposed to happen then?  You don't have a
> default anymore?  (That's what happens now.)

Yes, in that case there is no longer a default in the environments.yaml file,
and 
I have to check if juju-core is ok with this. If not, we need to change
this behavior. It's also important to define the concept of "default"
in the quickstart context: for now in Urwid it is just the YAML default, but
in the real world it can be overwritten by juju switch (which, in turn,
looks for that value in the JUJU_ENV env var and in the current_env file,
the latter being an implementation detail). When you run quickstart
without specifying -e, the default environment from juju switch is started.
In the interactive session, the default environment is instead the one in the 
YAML file, and that's because the interactive session is intended to be
a CRUD for envs.yaml. We might want to either unify those two concepts or 
keep them separate and show both in the UX.

> 
> QA good with those notes.
> 
> Thanks!

Thank you!


>
https://codereview.appspot.com/44310043/diff/1/quickstart/tests/cli/helpers.p...
> quickstart/tests/cli/helpers.py:39: def emit(self, widget):
> Don't you have this in another base class?  If so, maybe this ought to be a
> standalone function?  it does not require "self."

We had that, but now it is abstracted in this Mixin, used by
both test cases. That said, you are right, this does not
require self, so it can be just a function. The same for
get_button_caption and inspect_dialog. Fixed.

[frankban, 2013-12-20 10:35:05]

*** Submitted:

Implement the remove env functionality.

It is now possible to remove environments
using the interactive session.

A confirmation dialog prevents the user
to accidentally remove an environment.

Tests: `make check`.
QA: start the demo app 
(`make` and `./cli-app-demo.py).
Remove environments at will: exiting
from the app you should see the
new environments YAML without the
environments you deleted.
Any UX suggestion welcome.
Thank you!

R=gary.poster, bac
CC=
https://codereview.appspot.com/44310043

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py
File quickstart/cli/ui.py (right):

https://codereview.appspot.com/44310043/diff/1/quickstart/cli/ui.py#newcode125
quickstart/cli/ui.py:125: the function tries to generate suitable defaults.
On 2013/12/19 20:29:17, bac wrote:
> If the units are pixels please say so.

Those are Urwid specific values: if you pass
a number, the units are rows/columns, but you
can provide also other values: see 
http://excess.org/urwid/docs/reference/widget.html#overlay
Updated the docstring.

[frankban, 2013-12-20 10:38:17]

Thank you both for the reviews!