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

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

This branch is nice Brad, thank you.
LGTM with minor changes and a less trivial suggestion I'd like you to
consider.

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

https://codereview.appspot.com/57900043/diff/1/quickstart/manage.py#newcode294
quickstart/manage.py:294: At bootstrap, juju (v1.17 and later) writes
the admin-secret to a
IIRC Jenv files are generated starting form v1.16.

https://codereview.appspot.com/57900043/diff/1/quickstart/manage.py#newcode309
quickstart/manage.py:309: raise ValueError('admin-secret not found in
{}.'.format(filename))
Please encode('utf-8') the ValueError message.

https://codereview.appspot.com/57900043/diff/1/quickstart/manage.py#newcode316
quickstart/manage.py:316: - admin_secret: the password to use to access
the Juju API;
... or None if the admin-secret field is not found in the environments
YAML file... or similar?

https://codereview.appspot.com/57900043/diff/1/quickstart/manage.py#newcode419
quickstart/manage.py:419: options.admin_secret = _get_admin_secret(
It seems _get_admin_secret can raise exceptions we never catch. I am
considering a different strategy for this, please let me know what you
think about the following:
We always try to parse the jenv file, even if we already have an
admin_secret. Subsequently, we make the following check:
- if the password is found in the jenv, we use that, done.
- if the password is not found in the jenv:
   - if we already have an admin secret, we use that;
   - othrwise we exit with a (nice) error.
As a side note, this can involve moving the _get_admin_secret function
to the app module.
AFAICT this way we achieve two goals:
- we are still backward compatible with old versions of Juju;
- we support a use case we don't currently handle (in trunk): the case
when a user has a jenv enabled juju-core, runs quickstart to bootstrap
an environment, then runs quickstart interactively to explicitly provide
an admin-secret, and then re-invoke quickstart again on the same
environment (say, in order to deploy a bundle or reopen the GUI). Now
the passwords from the environments file and from the jenv are not the
same, but since we made the latter override the former, everything works
well.
How does it sound?

Another, less important, suggestion follows.
Rather than modifying options, we might just define admin_secret in this
scope, e.g.:

admin_secret = options.admin_secret
if admin_secret is None:
    admin_secret = ...

And then s/options.admin_secret/admin_secret/ below.
This way we keep the (very weak) separation between setup (which sets up
options) and run (which just uses them). <shrug>

https://codereview.appspot.com/57900043/diff/1/quickstart/models/envs.py
File quickstart/models/envs.py (right):

https://codereview.appspot.com/57900043/diff/1/quickstart/models/envs.py#newcode177
quickstart/models/envs.py:177: contents = _load_file(env_file)
Very nice.

https://codereview.appspot.com/57900043/

« Back to merge proposal