Implement the environment create/edit view.

Implement the environment create/edit view.

Add a cli/forms.py module including helpers
to create and handle Urwid forms.

Allow for creating a new environment from the 
env_index view.

Implement the env_edit view, used for creating
or updating environments.

Fix a corner case in envs.set_env_data: delete
the env_db default key only if the environments
has been modified.

Tests: `make check`.

QA: start the demo app 
(`make` and `./cli-app-demo.py).
Use it to edit existing environments and
to create new ones (ec2 and local).
Check form errors are correctly handled.
Exit the demo app with either ^X or 
selecting an environment to use. The
demo app should show changes on exit,
and notify whether a new db has been saved
and/or an environment has been selected.

https://code.launchpad.net/~frankban/juju-quickstart/env-manage-edit/+merge/200305

(do not edit description out of merge proposal)

[frankban, 2014-01-02 13:36:41]

Please take a look.

[frankban, 2014-01-02 13:41:25]

Sorry for the long diff, tests for views are a little verbose.

[gary.poster, 2014-01-02 14:24:57]

QA comments, pre-review:

- Looking very cool.
- I like the fact that I can right click on links to launch them.  That may well
be a terminal feature rather than a quickstart or urwid feature, but it all fits
together nicely.
- Trivial, but I continue to be dissatisfied with the footer formatting.  I
don't like the fact that the text in the legend line wraps.  For instance, when
creating, I want "field errors" to be on the same line as the dot it describes. 
Possible resolutions include the following: (a) we don't show "juju-quickstart
0.5.0" anywhere; (b) we show it in the right of the header; or (c) we show it in
its own line in the footer.
- It would be nice if you could tab between fields.  I assume this is an Urwid
limitation, in which case never mind.  Maybe we could clarify in the footer that
arrow keys move between fields?
- It was not clear to me that clicking on "automatically generate" would
generate the admin secret.  Could we maybe change that text to "Click here to
automatically generate this value"?
- For the default series, you say that you may leave the series empty.  It would
be nice if you explained the semantics of that in the help text.  Am I right
that the semantics are that the series of the host computer is used?
- Should we default the default series value to precise?
- Maybe a bad idea: we could make the "precise," "quantal," and other default
series text clickable.  When you click on them, they fill in the field with the
given value.  Same could be done with ec2 region.
- Maybe a better idea: Does Urwid support the idea of a combo box, somehow? 
Would be nice. :-)
- I wish the two "create" options in the main menu looked more different.  Any
bright UX ideas? :-)  Right now the whitespace separating the environments from
the creation options don't clarify their difference sufficiently for my eye.
- Relatedly, the header instructions in the main menu are currently "Select the
Juju environment you want to use."  That no longer seems to describe reality. 
"Select the Juju environment you want to use, edit, view, or remove." That's
wordy and leaves out the creation options.  "Select an existing Juju environment
or create a new one."  Maybe that's good enough?  The user can see what to do
with the environment once they select it.
- I know I suggested linking to https://juju.ubuntu.com/docs/config-aws.html for
help, but it is confusing since the first half of the page is not pertinent to
quickstart.  The good stuff begins with "You can retrieve these values easily
from your AWS Management Console at...."  My ideas so far include the following:
(a) confer with Nick to see if he has any ideas on whether we can add an anchor
in the page to the part we want, or if we can have a separate page with just the
part we want; (b) include the information for users actually in the help text
for access key ("You can retrieve these values easily from your AWS Management
Console at http://console.aws.amazon.com. Click on your name in the top-right
and then the "Security Credentials" link from the drop down menu.  Under the
"Access Keys" heading click the "Create New Root Key" button. You will be
prompted to "Download Key File" which by default is named rootkey.csv. Open this
file to get the access-key and secret-key for the environments.yaml
configuration file.") and then have the secret key help point to the access key
help.

General behavior looks very good.  These are a bunch of (hopefully at least
somewhat important/valuable) nitpicks.  Tests pass and have 100% coverage still.
:-) Now on to the code review.

[benji-york, 2014-01-02 15:05:31]

Looks great.

LGTM-ly, yrs
Benji

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcod...
quickstart/cli/forms.py:208: return ui.create_controls(*controls)
This is so small it is almost not worth mentioning, but I thought you might like
it:

return ui.create_controls(
    *(ui.MenuButton(caption, callback) for caption, callback in actions))


Or if we want to go full-on functional:

return ui.create_controls(*map(ui.MenuButton, actions))

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/ui.py#newcode49
quickstart/cli/ui.py:49: ('optional status', 'light magenta', 'light gray'),
Unfortunately the non-256 color terminal colors are not standardized so I
suggest testing on multiple terminals (I can test on xterm, gnome terminal, and
terminator if you want).

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:347: def save(env_data, get_new_env_data):
I like using functions defined in functions like this, but it means that they
are hard to test (or require functional tests instead of unit tests).  Consider
making them module-level functions so they can be tested directly.

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

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_form...
quickstart/tests/cli/test_forms.py:1: # This file is part of the Juju Quickstart
Plugin, which lets users set up a
These tests look really nice.  The running comments in each test really increase
understanding of what is going on.

[gary.poster, 2014-01-02 15:19:28]

LGTM!  Thank you!

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode44
quickstart/cli/forms.py:44: def create_string_widget(field, value, error):
Nice behavior/approach for the widget generation.  I like it.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode82
quickstart/cli/forms.py:82: if generate_method is not None:
For readability, I wonder if we ought to stick most of this block in a separate
function.

if generate_method is not None:
    widgets.append(
        create_generate_widget(generate_method, edit_widget))

...or similar?

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode98
quickstart/cli/forms.py:98: # edit widget when a choice is clicked.
Hah, ok, cool, that's what I mentioned in my qa. :-)

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcod...
quickstart/cli/forms.py:172: urwid.Text(('error', 'please correct the errors
below')),
Nice to have: a count.  ("Please correct the 2 errors below")

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcod...
quickstart/cli/forms.py:179: # Boolean values as represented as checkboxes.
s/as/are/

Boolean values are represented a checkboxes

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcod...
quickstart/cli/forms.py:195: return dict((key, value_getter()) for key,
value_getter in form.items())
Nice :-)

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:168: for position, env_data in enumerate(environments):
Just as a way to make the main function smaller, what would you think of
breaking this block out into its own function?

...eh, nevermind.  I tried to do it and you need default_found and
focus_position and errors_found, so it doesn't seem like there's much point.

The function is very clear and linear but I was hoping to make it shorter by
making some nice internal verbs.  Maybe you have another idea?  Could be for
later, or never. :-)

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:230: def use(env_data):
I wonder if the env_detail function would look less forbidding at first glance
if these internal functions were defined outside (_use, _set_default, etc.) and
then hooked up as partials.  I think the verbs are clear enough that it would
read well.  

A counter-argument is that you are defining everything locally, so you can read
what is going on inline, and it similar to what people are accustomed to doing
with OO.  If env_detail were in its own file as is, would it look super easy,
because it would look similar to a class with methods?  I don't know.

What do you think?

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

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_form...
quickstart/tests/cli/test_forms.py:1: # This file is part of the Juju Quickstart
Plugin, which lets users set up a
On 2014/01/02 15:05:32, benji1 wrote:
> These tests look really nice.  The running comments in each test really
increase
> understanding of what is going on.

+1 on both

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_form...
quickstart/tests/cli/test_forms.py:80: return (
Maybe use a dict or namedtuple instead?  Shrug.

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_form...
quickstart/tests/cli/test_forms.py:91: wrapper, edit, caption, help, error,
generate, choices, default = (
That works too. :-)

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_view...
File quickstart/tests/cli/test_views.py (right):

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_view...
quickstart/tests/cli/test_views.py:166:
@mock.patch('quickstart.cli.views.env_detail')
This sort of thing makes me think we ought to pass the needed env_* around as
arguments everywhere. :-)

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_view...
quickstart/tests/cli/test_views.py:200: for env_type, button in zip(env_types,
buttons[-len(env_types):]):
nice

[frankban, 2014-01-02 17:51:03]

Please take a look.

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcode82
quickstart/cli/forms.py:82: if generate_method is not None:
On 2014/01/02 15:19:28, gary.poster wrote:
> For readability, I wonder if we ought to stick most of this block in a
separate
> function.
> 
> if generate_method is not None:
>     widgets.append(
>         create_generate_widget(generate_method, edit_widget))
> 
> ...or similar?

Done.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcod...
quickstart/cli/forms.py:172: urwid.Text(('error', 'please correct the errors
below')),
On 2014/01/02 15:19:28, gary.poster wrote:
> Nice to have: a count.  ("Please correct the 2 errors below")

Done.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcod...
quickstart/cli/forms.py:179: # Boolean values as represented as checkboxes.
On 2014/01/02 15:19:28, gary.poster wrote:
> s/as/are/
> 
> Boolean values are represented a checkboxes

Done.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/forms.py#newcod...
quickstart/cli/forms.py:208: return ui.create_controls(*controls)
On 2014/01/02 15:05:32, benji1 wrote:
> This is so small it is almost not worth mentioning, but I thought you might
like
> it:
> 
> return ui.create_controls(
>     *(ui.MenuButton(caption, callback) for caption, callback in actions))
> 
> 
> Or if we want to go full-on functional:
> 
> return ui.create_controls(*map(ui.MenuButton, actions))

Done.

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/ui.py#newcode49
quickstart/cli/ui.py:49: ('optional status', 'light magenta', 'light gray'),
On 2014/01/02 15:05:32, benji1 wrote:
> Unfortunately the non-256 color terminal colors are not standardized so I
> suggest testing on multiple terminals (I can test on xterm, gnome terminal,
and
> terminator if you want).

My understanding is that these color combinations can be safely used
cross-terminal because the colors are handled by Urwid itself:
http://excess.org/urwid/docs/reference/constants.html?highlight=colors#foregr...
But I can be wrong, and maybe it is worth testing it.

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:168: for position, env_data in enumerate(environments):
On 2014/01/02 15:19:28, gary.poster wrote:
> Just as a way to make the main function smaller, what would you think of
> breaking this block out into its own function?
> 
> ...eh, nevermind.  I tried to do it and you need default_found and
> focus_position and errors_found, so it doesn't seem like there's much point.
> 
> The function is very clear and linear but I was hoping to make it shorter by
> making some nice internal verbs.  Maybe you have another idea?  Could be for
> later, or never. :-)

Not sure, a quick solution might be splitting the function into three parts
(existing envs selection, new env creation, status management) in a way similar
to what you suggested. It could be worth doing that, especially if we find in
the future that the code starts ti be too much nested. For now, as you
mentioned, the function is quite flat and easy to follow.

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:230: def use(env_data):
On 2014/01/02 15:19:28, gary.poster wrote:
> I wonder if the env_detail function would look less forbidding at first glance
> if these internal functions were defined outside (_use, _set_default, etc.)
and
> then hooked up as partials.  I think the verbs are clear enough that it would
> read well.  
> 
> A counter-argument is that you are defining everything locally, so you can
read
> what is going on inline, and it similar to what people are accustomed to doing
> with OO.  If env_detail were in its own file as is, would it look super easy,
> because it would look similar to a class with methods?  I don't know.
> 
> What do you think?

This is a really good question. I like the possibility to use closures so that
you can share the scope and group code logic, but, as you mentioned, this can be
surprising at first sight. If you add to this the good point Benji made below
(it is easier to directly unit test separate functions), then we have two good
arguments to make this change. Since it requires a bit of code reorganization
I'd be inclined to tackle this separately, how does it sound?

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:347: def save(env_data, get_new_env_data):
On 2014/01/02 15:05:32, benji1 wrote:
> I like using functions defined in functions like this, but it means that they
> are hard to test (or require functional tests instead of unit tests). 
Consider
> making them module-level functions so they can be tested directly.

Very good point. Please see my answer to a similar comment from Gary above.

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_view...
File quickstart/tests/cli/test_views.py (right):

https://codereview.appspot.com/44750044/diff/1/quickstart/tests/cli/test_view...
quickstart/tests/cli/test_views.py:166:
@mock.patch('quickstart.cli.views.env_detail')
On 2014/01/02 15:19:28, gary.poster wrote:
> This sort of thing makes me think we ought to pass the needed env_* around as
> arguments everywhere. :-)

Do you mean, as kwargs? e.g. def env_index(app, ..., env_detail=env_detail)?
That would avoid the need to patch the function here: it would be possible to
just pass a mock object as the kwarg.

[frankban, 2014-01-02 17:52:44]

Now working on the QA comments.
In the meanwhile, thank you for the great reviews!

[gary.poster, 2014-01-02 18:48:25]

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

https://codereview.appspot.com/44750044/diff/1/quickstart/cli/views.py#newcod...
quickstart/cli/views.py:230: def use(env_data):
On 2014/01/02 17:51:04, frankban wrote:
> On 2014/01/02 15:19:28, gary.poster wrote:
> > I wonder if the env_detail function would look less forbidding at first
glance
> > if these internal functions were defined outside (_use, _set_default, etc.)
> and
> > then hooked up as partials.  I think the verbs are clear enough that it
would
> > read well.  
> > 
> > A counter-argument is that you are defining everything locally, so you can
> read
> > what is going on inline, and it similar to what people are accustomed to
doing
> > with OO.  If env_detail were in its own file as is, would it look super
easy,
> > because it would look similar to a class with methods?  I don't know.
> > 
> > What do you think?
> 
> This is a really good question. I like the possibility to use closures so that
> you can share the scope and group code logic, but, as you mentioned, this can
be
> surprising at first sight. If you add to this the good point Benji made below
> (it is easier to directly unit test separate functions), then we have two good
> arguments to make this change. Since it requires a bit of code reorganization
> I'd be inclined to tackle this separately, how does it sound?

Sounds great to me.  Thank you.

[frankban, 2014-01-03 11:39:37]

Please take a look.

[frankban, 2014-01-03 11:42:10]

On 2014/01/02 14:24:57, gary.poster wrote:
> QA comments, pre-review:
>
> - Looking very cool.
> - I like the fact that I can right click on links to launch them.  That may
well
> be a terminal feature rather than a quickstart or urwid feature, but it all
fits
> together nicely.

I agree, that's cool.

> - Trivial, but I continue to be dissatisfied with the footer formatting.  I
> don't like the fact that the text in the legend line wraps.  For instance,
when
> creating, I want "field errors" to be on the same line as the dot it
describes.
> Possible resolutions include the following: (a) we don't show "juju-quickstart
> 0.5.0" anywhere; (b) we show it in the right of the header; or (c) we show it
in
> its own line in the footer.

Removed the quickstart brand and put the notifications widget in its own line
(above the footer). This way I see the status in a single line even when
the terminal size is 80x24.

> - It would be nice if you could tab between fields.  I assume this is an Urwid
> limitation, in which case never mind.  Maybe we could clarify in the footer
that
> arrow keys move between fields?

I'd also like tab navigation to be implemented as part of the UX improvement
card(s). I think it should not be too difficult to implement that in Urwid.
In the meanwhile, added navigation help to the status as you suggested.

> - It was not clear to me that clicking on "automatically generate" would
> generate the admin secret.  Could we maybe change that text to "Click here to
> automatically generate this value"?

Good idea, done.

> - For the default series, you say that you may leave the series empty.  It
would
> be nice if you explained the semantics of that in the help text.  Am I right
> that the semantics are that the series of the host computer is used?
> - Should we default the default series value to precise?

You are right. While we usually let juju itself handle the default values
where not set, for the default series quickstart should force an explicit
"precise" when the value is not specified. This means the default series
field must overwrite the normalize method, and that should be easy enough.
I'll do this in another card, perhaps the same as the one below (choices).

> - Maybe a bad idea: we could make the "precise," "quantal," and other default
> series text clickable.  When you click on them, they fill in the field with
the
> given value.  Same could be done with ec2 region.
> - Maybe a better idea: Does Urwid support the idea of a combo box, somehow?
> Would be nice. :-)

AFAIK Urwid does not have a combo widget out of the box, but we can try to
implement it on top of the other base widgets. The first solution sounds good
to me anyway, and as you noticed I already have an XXX for it, and I will
create a card as well.

> - I wish the two "create" options in the main menu looked more different.  Any
> bright UX ideas? :-)  Right now the whitespace separating the environments
from
> the creation options don't clarify their difference sufficiently for my eye.

Updated the index view so that the environment creation options are more
separated. Now there is a text indicating how to use the creation links.
Does it look better?

> - Relatedly, the header instructions in the main menu are currently "Select
the
> Juju environment you want to use."  That no longer seems to describe reality.
> "Select the Juju environment you want to use, edit, view, or remove." That's
> wordy and leaves out the creation options.  "Select an existing Juju
environment
> or create a new one."  Maybe that's good enough?  The user can see what to do
> with the environment once they select it.

Done.

> - I know I suggested linking to https://juju.ubuntu.com/docs/config-aws.html
for
> help, but it is confusing since the first half of the page is not pertinent to
> quickstart.  The good stuff begins with "You can retrieve these values easily
> from your AWS Management Console at...."  My ideas so far include the
following:
> (a) confer with Nick to see if he has any ideas on whether we can add an
anchor
> in the page to the part we want, or if we can have a separate page with just
the
> part we want; (b) include the information for users actually in the help text
> for access key ("You can retrieve these values easily from your AWS Management
> Console at http://console.aws.amazon.com. Click on your name in the top-right
> and then the "Security Credentials" link from the drop down menu.  Under the
> "Access Keys" heading click the "Create New Root Key" button. You will be
> prompted to "Download Key File" which by default is named rootkey.csv. Open
this
> file to get the access-key and secret-key for the environments.yaml
> configuration file.") and then have the secret key help point to the access
key
> help.

Done b).

Thank you for this great QA!

[gary.poster, 2014-01-03 13:51:06]

On 2014/01/03 11:42:10, frankban wrote:
> On 2014/01/02 14:24:57, gary.poster wrote:
> > QA comments, pre-review:
> >
> > - Looking very cool.
> > - I like the fact that I can right click on links to launch them.  That may
> well
> > be a terminal feature rather than a quickstart or urwid feature, but it all
> fits
> > together nicely.
> 
> I agree, that's cool.
> 
> > - Trivial, but I continue to be dissatisfied with the footer formatting.  I
> > don't like the fact that the text in the legend line wraps.  For instance,
> when
> > creating, I want "field errors" to be on the same line as the dot it
> describes.
> > Possible resolutions include the following: (a) we don't show
"juju-quickstart
> > 0.5.0" anywhere; (b) we show it in the right of the header; or (c) we show
it
> in
> > its own line in the footer.
> 
> Removed the quickstart brand and put the notifications widget in its own line
> (above the footer). This way I see the status in a single line even when
> the terminal size is 80x24.

Much better, thanks!

> > - It would be nice if you could tab between fields.  I assume this is an
Urwid
> > limitation, in which case never mind.  Maybe we could clarify in the footer
> that
> > arrow keys move between fields?
> 
> I'd also like tab navigation to be implemented as part of the UX improvement
> card(s). I think it should not be too difficult to implement that in Urwid.
> In the meanwhile, added navigation help to the status as you suggested.

Yes, looks good.

> > - It was not clear to me that clicking on "automatically generate" would
> > generate the admin secret.  Could we maybe change that text to "Click here
to
> > automatically generate this value"?
> 
> Good idea, done.
> 
> > - For the default series, you say that you may leave the series empty.  It
> would
> > be nice if you explained the semantics of that in the help text.  Am I right
> > that the semantics are that the series of the host computer is used?
> > - Should we default the default series value to precise?
> 
> You are right. While we usually let juju itself handle the default values
> where not set, for the default series quickstart should force an explicit
> "precise" when the value is not specified. This means the default series
> field must overwrite the normalize method, and that should be easy enough.
> I'll do this in another card, perhaps the same as the one below (choices).

Cool

> > - Maybe a bad idea: we could make the "precise," "quantal," and other
default
> > series text clickable.  When you click on them, they fill in the field with
> the
> > given value.  Same could be done with ec2 region.
> > - Maybe a better idea: Does Urwid support the idea of a combo box, somehow?
> > Would be nice. :-)
> 
> AFAIK Urwid does not have a combo widget out of the box, but we can try to
> implement it on top of the other base widgets. The first solution sounds good
> to me anyway, and as you noticed I already have an XXX for it, and I will
> create a card as well.

Great.  I think the links will be fine (instead of trying to implement a
combobox) as long as our text clearly indicates the functionality (e.g. "Click
one to autofill the field with these standard options: precise, quantal, raring,
...")

> > - I wish the two "create" options in the main menu looked more different. 
Any
> > bright UX ideas? :-)  Right now the whitespace separating the environments
> from
> > the creation options don't clarify their difference sufficiently for my eye.
> 
> Updated the index view so that the environment creation options are more
> separated. Now there is a text indicating how to use the creation links.
> Does it look better?

Yes, much, thanks.

> > - Relatedly, the header instructions in the main menu are currently "Select
> the
> > Juju environment you want to use."  That no longer seems to describe
reality.
> > "Select the Juju environment you want to use, edit, view, or remove." That's
> > wordy and leaves out the creation options.  "Select an existing Juju
> environment
> > or create a new one."  Maybe that's good enough?  The user can see what to
do
> > with the environment once they select it.
> 
> Done.

Cool.

> > - I know I suggested linking to https://juju.ubuntu.com/docs/config-aws.html
> for
> > help, but it is confusing since the first half of the page is not pertinent
to
> > quickstart.  The good stuff begins with "You can retrieve these values
easily
> > from your AWS Management Console at...."  My ideas so far include the
> following:
> > (a) confer with Nick to see if he has any ideas on whether we can add an
> anchor
> > in the page to the part we want, or if we can have a separate page with just
> the
> > part we want; (b) include the information for users actually in the help
text
> > for access key ("You can retrieve these values easily from your AWS
Management
> > Console at http://console.aws.amazon.com. Click on your name in the
top-right
> > and then the "Security Credentials" link from the drop down menu.  Under the
> > "Access Keys" heading click the "Create New Root Key" button. You will be
> > prompted to "Download Key File" which by default is named rootkey.csv. Open
> this
> > file to get the access-key and secret-key for the environments.yaml
> > configuration file.") and then have the secret key help point to the access
> key
> > help.
> 
> Done b).

Cool.

> Thank you for this great QA!

Thank you!  Looking great.

Gary

[frankban, 2014-01-03 13:57:55]

*** Submitted:

Implement the environment create/edit view.

Add a cli/forms.py module including helpers
to create and handle Urwid forms.

Allow for creating a new environment from the 
env_index view.

Implement the env_edit view, used for creating
or updating environments.

Fix a corner case in envs.set_env_data: delete
the env_db default key only if the environments
has been modified.

Tests: `make check`.

QA: start the demo app 
(`make` and `./cli-app-demo.py).
Use it to edit existing environments and
to create new ones (ec2 and local).
Check form errors are correctly handled.
Exit the demo app with either ^X or 
selecting an environment to use. The
demo app should show changes on exit,
and notify whether a new db has been saved
and/or an environment has been selected.

R=gary.poster, benji1
CC=
https://codereview.appspot.com/44750044