Code review comment for lp://qastaging/~bac/juju-quickstart/1309678

Revision history for this message
Francesco Banconi (frankban) wrote :

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/

« Back to merge proposal