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#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.
> 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.
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 manage. py (right):
> File quickstart/
https:/ /codereview. appspot. com/132770043/ diff/1/ quickstart/ manage. py#newcode485 manage. py:485: options. distro_ only, options.platform,
> quickstart/
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 dependencies? Do you have a
goal.
> Perhaps get_juju_command can return the path and a "customized" flag?
And then
> move the path switching logic to ensure_
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 manage. py:499: requires_sudo = juju_version < (1, 17, 2)
> quickstart/
> 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 manage. py:504: print("Warning: ignoring JUJU environment warn("ignoring JUJU environment variable") (no need
> quickstart/
variable")
> Please use logging.
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 TestRun. test_juju_ environ_ var_set is testing this.
test_manage.
https:/ /codereview. appspot. com/132770043/ diff/1/ quickstart/ platform_ support. py platform_ support. py (right):
> File quickstart/
https:/ /codereview. appspot. com/132770043/ diff/1/ quickstart/ platform_ support. py#newcode120 platform_ support. py:120: returned.
> quickstart/
> 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 platform_ support. py:124: print("Warning: a customized juju
> quickstart/
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 platform_ support. py:126: else:
> quickstart/
> 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 tests/test_ manage. py (right):
> File quickstart/
https:/ /codereview. appspot. com/132770043/ diff/1/ quickstart/ tests/test_ manage. py#newcode995 tests/test_ manage. py:995: def environ_ var_set( self,
> quickstart/
test_juju_
> mock_app, mock_open):
> Very nice test.
https:/ /codereview. appspot. com/132770043/ diff/1/ quickstart/ tests/test_ platform_ support. py tests/test_ platform_ support. py (right):
> File quickstart/
https:/ /codereview. appspot. com/132770043/ diff/1/ quickstart/ tests/test_ platform_ support. py#newcode170 tests/test_ platform_ support. py:170: class and(unittest. TestCase) :
> quickstart/
> TestGetJujuComm
> 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/