Merge lp://qastaging/~frankban/juju-quickstart/improve-version-handling into lp://qastaging/juju-quickstart

Reviewers: mp+210210_code.launchpad.net,

Message:
Please take a look.

Description:
SSH code reorganization.

This branch introduces a new ssh module
containing SSH handling related code.

The SSH functions previously in app are now
library functions in the ssh module.

ensure_ssh_keys is now the only app entry point
for the whole SSH keys management logic.

For the reasons above this branch mostly includes
code movements + some docstring/comments changes +
some exception handling changes.

https://code.launchpad.net/~frankban/juju-quickstart/improve-version-handling/+merge/210210

(do not edit description out of merge proposal)

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

Affected files (+425, -330 lines):
   A [revision details]
   M quickstart/app.py
   A quickstart/ssh.py
   M quickstart/tests/test_app.py
   A quickstart/tests/test_ssh.py
   M quickstart/tests/test_utils.py
   M quickstart/utils.py

LGTM Thanks for this cleanup. I only have one code re-org suggestion if
you choose.

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

https://codereview.appspot.com/72520044/diff/1/quickstart/app.py#newcode106
quickstart/app.py:106: ssh.start_agent()
I feel like this should be moved into the check_keys function so the app
only needs to call check_keys once

https://codereview.appspot.com/72520044/

LGTM -- thanks for the re-org.

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

https://codereview.appspot.com/72520044/diff/1/quickstart/app.py#newcode123
quickstart/app.py:123: 'or cancel [C]? ').lower()
Why tell the user to use upper case C if you're going to lower it?

https://codereview.appspot.com/72520044/diff/1/quickstart/ssh.py
File quickstart/ssh.py (right):

https://codereview.appspot.com/72520044/diff/1/quickstart/ssh.py#newcode73
quickstart/ssh.py:73: key_file = os.path.join(os.path.expanduser('~'),
'.ssh', 'id_rsa')
Is it safe to use 'id_rsa'? I haven't followed the call sites but are
we sure these keys don't already exist? I'd hate to see us overwrite a
user's keys. Would it not be better to use something novel like
'id_rsa_quickstart'?

https://codereview.appspot.com/72520044/

*** Submitted:

SSH code reorganization.

This branch introduces a new ssh module
containing SSH handling related code.

The SSH functions previously in app are now
library functions in the ssh module.

ensure_ssh_keys is now the only app entry point
for the whole SSH keys management logic.

For the reasons above this branch mostly includes
code movements + some docstring/comments changes +
some exception handling changes.

R=jeff.pihach, bac
CC=
https://codereview.appspot.com/72520044

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

https://codereview.appspot.com/72520044/diff/1/quickstart/app.py#newcode123
quickstart/app.py:123: 'or cancel [C]? ').lower()
On 2014/03/10 14:20:16, bac wrote:
> Why tell the user to use upper case C if you're going to lower it?

Done.

https://codereview.appspot.com/72520044/diff/1/quickstart/ssh.py
File quickstart/ssh.py (right):

https://codereview.appspot.com/72520044/diff/1/quickstart/ssh.py#newcode73
quickstart/ssh.py:73: key_file = os.path.join(os.path.expanduser('~'),
'.ssh', 'id_rsa')
On 2014/03/10 14:20:16, bac wrote:
> Is it safe to use 'id_rsa'? I haven't followed the call sites but are
we sure
> these keys don't already exist? I'd hate to see us overwrite a user's
keys.
> Would it not be better to use something novel like
'id_rsa_quickstart'?

This pre-existed, but IIUC, this is only called if no keys are found
(see check_keys above). For this reason it seems safe to me to create
the first one with the default name.

https://codereview.appspot.com/72520044/

Thank you both!

https://codereview.appspot.com/72520044/