Code review comment for lp:~frankban/juju-quickstart/code-reorg

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

Thanks for the reviews!

https://codereview.appspot.com/169380043/diff/1/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/169380043/diff/1/quickstart/app.py#newcode263
quickstart/app.py:263: Parse the jenv file to retrieve the admin secret.
On 2014/11/11 15:52:36, rharding wrote:
> do you know if we can use the password field in the jenv for this?
Next week we
> hope to start updating the gui for the new username/password work in
juju 1.21
> and I know we'll need to update quickstart to pull the
username/password from
> the jenv file.

With this change it should be easy to grab what we want from the jenv
with jenv.get_value(). We will want to use some fallback logic to
support backward compatibility.
If the user is != user-admin, we will need to update the one-show
automatic authentication with token as well in the guiserver and maybe
in the GUI itself.

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

https://codereview.appspot.com/169380043/diff/1/quickstart/models/envs.py#oldcode202
quickstart/models/envs.py:202: def load_generated(env_file,
section='bootstrap-config'):
On 2014/11/11 15:52:36, rharding wrote:
> was this unused?

It was used, it is no longer required now.

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

https://codereview.appspot.com/169380043/diff/1/quickstart/models/jenv.py#newcode39
quickstart/models/jenv.py:39: def get_value(env_name, *args):
On 2014/11/11 15:37:55, bac wrote:
> Why did you chose to use *args here instead of a tuple of key names?

No real reason actually, I started with just one single key as the
argument and then, after deciding to allow nested keys, it just felt
natural.

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

https://codereview.appspot.com/169380043/diff/1/quickstart/tests/helpers.py#newcode156
quickstart/tests/helpers.py:156: In the contentx manager block, the
JUJU_HOME is set to the ancestor
On 2014/11/11 15:37:55, bac wrote:
> typo: contextx

Done.

https://codereview.appspot.com/169380043/

« Back to merge proposal