Code review comment for lp:~frankban/juju-gui/quickstart-bootstrap

Revision history for this message
Gary Poster (gary) wrote :

LGTM with a few trivials. Thank you.

https://codereview.appspot.com/14669047/diff/1/Makefile
File Makefile (right):

https://codereview.appspot.com/14669047/diff/1/Makefile#newcode20
Makefile:20: JUJU_ENV ?= quickstart
So...this is used by make run to override the normal Python logic of the
command. I'm not quite sure of the value, but I'm fine with it.

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

https://codereview.appspot.com/14669047/diff/1/quickstart/tests/helpers.py#newcode17
quickstart/tests/helpers.py:17: """Tests helpers for the Juju Quickstart
plugin."""
Trivial: Test helpers

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

https://codereview.appspot.com/14669047/diff/1/quickstart/tests/test_manage.py#newcode119
quickstart/tests/test_manage.py:119: and a it is also possible to
simulate an arbitrary environment name.
trivial: and it is

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

https://codereview.appspot.com/14669047/diff/1/quickstart/tests/test_utils.py#newcode48
quickstart/tests/test_utils.py:48: self.assertEqual(2, retcode)
checking precise non-zero retcode seems a bit risky, even though it
probably is practically fine for something as ancient as ls.

https://codereview.appspot.com/14669047/diff/1/quickstart/tests/test_utils.py#newcode58
quickstart/tests/test_utils.py:58: self.assertEqual(127, retcode)
likewise

https://codereview.appspot.com/14669047/diff/1/quickstart/tests/test_utils.py#newcode112
quickstart/tests/test_utils.py:112: with self.patch_call(0,
output='Exterminate!') as mock_call:
lol

https://codereview.appspot.com/14669047/diff/1/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/14669047/diff/1/quickstart/utils.py#newcode58
quickstart/utils.py:58: retcode, output, _ = call('juju', 'switch')
I wonder which is more fragile: relying on a regex of a stdout reply, or
looking at ~/.juju/current-environment and
~/.juju/environments.yaml[default]? This is certainly easier, so I
suppose that's a compelling argument, given that the answer to my
question is not obvious to me.

https://codereview.appspot.com/14669047/

« Back to merge proposal