Code review comment for lp:~makyo/juju-quickstart/ssh-2-create-keys

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

LGTM with (possible) changes, thank you!

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

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode101
quickstart/app.py:101: if len(create_keys) > 0 and
create_keys[0].lower() == 'y':
This can be written just as:
if create_keys.lower() == 'y':

In general, for sequences (like strings), use the fact that if they are
empty they are also False, e.g.:
use "if seq" instead of "if len(seq) [> 0]".

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode109
quickstart/app.py:109: 'ssh-add')
Thinking about this in retrospective, I am not sure we need the user to
have an ssh agent set up. It seems all we need are ssh keys, so IIUC the
return codes you described, a 130 could still be ok for us. What do you
think? Feel free to disagree.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode121
quickstart/app.py:121: retcode, _, error =
utils.call('/usr/bin/ssh-keygen',
Here, if the user decides to continue, we just generate ssh keys without
starting the agent. This is ok IMHO, but then we don't want to require
the agent on the next quickstart run.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode128
quickstart/app.py:128: raise ProgramExit('Error generating ssh key!
{}'.format(error))
All the other quickstart messages are lower cased: we might want to
change this, but for now let's follow that pattern.

https://codereview.appspot.com/36080044/diff/1/quickstart/app.py#newcode129
quickstart/app.py:129: print('A new SSH key was generated in
{}'.format(key_file))
Ditto.

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

https://codereview.appspot.com/36080044/diff/1/quickstart/tests/test_app.py#newcode197
quickstart/tests/test_app.py:197: mock_call.assert_has_calls([
assert_called_with? <shrug>

https://codereview.appspot.com/36080044/diff/1/quickstart/tests/test_app.py#newcode239
quickstart/tests/test_app.py:239:
mock_call.assert_called_with('/usr/bin/ssh-keygen',
assert_called_once_with? Cheap double check.

https://codereview.appspot.com/36080044/

« Back to merge proposal