Code review comment for lp:~evarlast/juju-quickstart/which-juju

Thanks for this branch Jay.
Please take a look at my concerns below: if I am right then some more
work (and thinking) is required before being able to include this
functionality.

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

https://codereview.appspot.com/132770043/diff/1/quickstart/manage.py#newcode485
quickstart/manage.py:485: options.distro_only, options.platform,
juju_command)
We are using juju_command here to check if juju is installed and
retrieve its version. This means we are already using a possible
injected juju path. Later, if sudo is required, we end up changing the
juju command, invalidating the information retrieved here.
In theory, when switching the juju_command path, we should re-ensure its
dependencies.
For instance, assume I don't have juju installed, but I have an old
compiled version which requires sudo: with the current logic, the
dependency check pass, but the the application proceed with an invalid
juju version until it crashes because the default path is not found
(assuming I understand the code flow correctly).

It seems to me that a refactoring is required here to accomplish the
goal. Perhaps get_juju_command can return the path and a "customized"
flag? And then move the path switching logic to ensure_dependencies? Do
you have a better idea?

https://codereview.appspot.com/132770043/diff/1/quickstart/manage.py#newcode499
quickstart/manage.py:499: requires_sudo = juju_version < (1, 17, 2)
At this point I'd just do:
if juju_version < (1, 17, 2):
     requires_sudo = True
     ...

But as described above, this part is likely to change.

https://codereview.appspot.com/132770043/diff/1/quickstart/manage.py#newcode504
quickstart/manage.py:504: print("Warning: ignoring JUJU environment
variable")
Please use logging.warn("ignoring JUJU environment variable") (no need
for capitalization).

Also, this if block does not seem to be tested.
If we move the path switching logic somewhere else I presume it would be
easier to test this case.

https://codereview.appspot.com/132770043/diff/1/quickstart/platform_support.py
File quickstart/platform_support.py (right):

https://codereview.appspot.com/132770043/diff/1/quickstart/platform_support.py#newcode120
quickstart/platform_support.py:120: returned.
If the environment variable JUJU is set and ignore_env_var is False,
then its value will be returned.

https://codereview.appspot.com/132770043/diff/1/quickstart/platform_support.py#newcode124
quickstart/platform_support.py:124: print("Warning: a customized juju is
being used.")
Please use logging.warn, e.g.:
logging.warn('a customized juju is being used: ' + juju_command)

https://codereview.appspot.com/132770043/diff/1/quickstart/platform_support.py#newcode126
quickstart/platform_support.py:126: else:
Unnecessary else block.

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

https://codereview.appspot.com/132770043/diff/1/quickstart/tests/test_manage.py#newcode995
quickstart/tests/test_manage.py:995: def test_juju_environ_var_set(self,
mock_app, mock_open):
Very nice test.

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

https://codereview.appspot.com/132770043/diff/1/quickstart/tests/test_platform_support.py#newcode170
quickstart/tests/test_platform_support.py:170: class
TestGetJujuCommand(unittest.TestCase):
We should add a test here for the common case when the env var is not
set at all.

https://codereview.appspot.com/132770043/