Merge lp://qastaging/~bac/juju-quickstart/1309678 into lp://qastaging/juju-quickstart

Reviewers: mp+216634_code.launchpad.net,

Message:
Please take a look.

Description:
Make control-bucket optional.

If not control-bucket is present in the environments.yaml file, read
from
generated .jenv file as Juju now creates a bucket name if none is given.

The existing function get_admin_secret is made generic and the name is
changed
to get_value_from_jenv. It now takes a keyname to be fetched from the
generated file.

For both ec2 and openstack the control-bucket field has been made
optional
with respect to the environments.yaml file but is still required to be
in one
of the two files.

Other than the error checking provided by fetching from the jenv file it
is
unclear whether the actual value of the control bucket is of any use to
Juju
Quickstart. It looks as if it isn't. If so, that can be made explicit
by
inserting a comment and by removing the assignment to
options.control_bucket,
though that value is very useful for testing.

Finally, is there a danger that older versions of Juju will break?

QA:

1) Create an ec2 environment with no control-bucket.
2) Bootstrap that environment. Ensure a control-bucket is in the .jenv
file.
3) Run quickstart and see that it uses the existing environment and does
not
raise an error.

https://code.launchpad.net/~bac/juju-quickstart/1309678/+merge/216634

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/90060043/

Affected files (+86, -30 lines):
   A [revision details]
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/models/envs.py
   M quickstart/tests/models/test_envs.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_manage.py

Hi Brad,

this branch looks nice.
As you mentioned, I am not sure why we want to retrieve the control
bucket from the jenv: while quickstart needs the admin secret so that it
can connect to the Juju API, the control bucket has no use, except
validating the jenv file, which I think we can assume to be correctly
generated by juju-core.

What do you think about the following?
1) we just mark the control bucket fields as optional in the ec2 and
openstack metadata;
2) in newer juju-core version, a missing value should just work;
3) in older juju-core, I'd expect bootstrap to exit with an error, and
the user can still use quickstart to set the value;
4) in both cases, if bootstrap succeeds, we can continue the process
without checking the control-bucket value in the jenv.

I expect the above to simplify this branch a lot.
What do you think?

https://codereview.appspot.com/90060043/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/90060043/diff/1/quickstart/manage.py#newcode481
quickstart/manage.py:481: control_bucket = options.control_bucket
AFAICT the control bucket is never set in the options namespace. For
this reason. I'd expect this to raise an AttributeError in the unlikely
case the value is not in the jenv, but maybe I am missing something.

Anyway, I am concerned about the need to retrieve the control bucket.
Please see my comment.

https://codereview.appspot.com/90060043/

Please take a look.

https://codereview.appspot.com/90060043/

LGTM QA ok.

https://codereview.appspot.com/90060043/

*** Submitted:

Make control-bucket optional.

The existing function get_admin_secret is made generic and the name is
changed
to get_value_from_jenv. It now takes a keyname to be fetched from the
generated file.

For both ec2 and openstack the control-bucket field has been made
optional
with respect to the environments.yaml file.

QA:

1) Create an ec2 environment with no control-bucket.
2) Bootstrap that environment. Ensure a control-bucket is in the .jenv
file.
3) Run quickstart and see that it uses the existing environment and does
not
raise an error.

R=frankban
CC=
https://codereview.appspot.com/90060043

https://codereview.appspot.com/90060043/