On 2014/08/21 16:06:06, frankban wrote:
> 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?

I agree with all of the issues you have raised. I feel that addressing
them now is out of scope of the original intent of the change, which was
for debugging purposes for an expert experienced user, not the user who
needs dependencies for juju resolved.



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.


The cover tool is either wrong when it says 100% or
test_manage.TestRun.test_juju_environ_var_set is testing this.



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)


Will this get printed to the user?

I followed the existing warning in ensure_ssh_keys print('Warning: no
SSH keys were found in ~/.ssh


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


I'm still waiting for that team coding standard document. :)


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.

I am surprised it wasn't already there.

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