Set juju binary with JUJU env var

Set juju binary with JUJU env var

Print a warning if a customized juju is being used.

Do not continue if juju must be executed using sudo.

/usr/bin/juju was hard-coded.  Made interoperability testing with
  pre-release versions of juju difficult. This allows override with JUJU
  environment var.

https://code.launchpad.net/~evarlast/juju-quickstart/which-juju/+merge/227238

(do not edit description out of merge proposal)

[jrwren, 2014-08-21 15:24:40]

Please take a look.

[frankban, 2014-08-21 16:06:06]

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.p...
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.p...
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.p...
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....
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_platfor...
File quickstart/tests/test_platform_support.py (right):

https://codereview.appspot.com/132770043/diff/1/quickstart/tests/test_platfor...
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.

[jrwren, 2014-08-21 17:33:59]

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.p...
> 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.p...
> 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.p...
> 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....
> 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_platfor...
> File quickstart/tests/test_platform_support.py (right):
> 
>
https://codereview.appspot.com/132770043/diff/1/quickstart/tests/test_platfor...
> 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.

[jrwren, 2014-08-21 17:34:04]

Please take a look.

[jrwren, 2014-08-21 18:25:39]

Please take a look.

[frankban, 2014-08-22 09:48:13]

On 2014/08/21 17:33:59, jrwren wrote:

> 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.

I am not sure we want to achieve this goal while introducing a subtle bug.
I'd propose to simplify this, by not allowing quickstart to be run at all if
sudo is
required and a customized command is provided. I guess the main goal is to
allow people to run quickstart on a newer compiled version of Juju, and so
this should not be a problem.

A possible solution is like the following (code not tested).

1) Make get_juju_command return a customized flag, assuming that flag is true
   only when JUJU is set and it differs from the default command, e.g.:

def get_juju_command(platform):
    """Return the path to the Juju command on the given platform.

    Also return a flag indicating whether the user requested to customize
    the Juju command by providing a JUJU environment variable.

    If the platform does not have a novel location, the default will be
    returned.
    """
    platform_command = settings.JUJU_CMD_PATHS.get(
        platform,
        settings.JUJU_CMD_PATHS['default'])
    customized_command = os.getenv('JUJU', '').strip()
    if customized_command and (customized_command != platform_command):
        logging.warn('a customized Juju is being used: ' + customized_command)
        return customized_command, True
    return platform_command, False

Note that in the warning we don't want to repeat "Warning:" and we don't want
capitalization.

2) Add an app.juju_requires_sudo like the following:

def juju_requires_sudo(env_type, juju_version, customized):
    """Report whether the given Juju version requires "sudo".

    The env_type argument is the current environment type.

    Raise a ProgramExit if a customized version of Juju is being used and Juju
    itself requires "sudo".
    """
    # If the Juju version is less than 1.17.2 then use sudo for local envs.
    if env_type == 'local' and juju_version < (1, 17, 2):
        if customized:
            raise ProgramExit(b'cannot use a customized Juju for reasons...')
        return True
    return False

3) At this point, the logic in manage.run is very simple, as it is supposed
   to be:

    juju_command, customized = platform_support.get_juju_command(
        options.platform)

    logging.debug('ensuring Juju and dependencies are installed')
    juju_version = app.ensure_dependencies(
        options.distro_only, options.platform, juju_command)

    requires_sudo = app.juju_requires_sudo(
        options.env_type, juju_version, customized)

    logging.debug('ensuring SSH keys are available')
    app.ensure_ssh_keys()

    print('bootstrapping the {} environment (type: {})'.format(
        options.env_name, options.env_type))
    if options.env_type == 'local':
        # If this is a local environment, notify the user that "sudo" will be
        # required to bootstrap the application, even in newer Juju versions
        # where "sudo" is invoked by juju-core itself.
        print('sudo privileges will be required to bootstrap the environment')

    already_bootstrapped, bootstrap_node_series = app.bootstrap(
        options.env_name, juju_command,
        requires_sudo=requires_sudo, debug=options.debug,
        upload_tools=options.upload_tools,
        upload_series=options.upload_series,
        constraints=options.constraints)
    ...

Also note that my current compiled Juju version is "1.21-alpha1-trusty-amd64".
Yeah, unfortunately it's not a semantic version: we don't have a patch number.
This means the logic in utils.get_juju_version must be slightly changed to
support customized Juju.

I know this is not the original plan, and it's not as easy as it seemed
originally, and I am sorry about that. But I think if we want this 
functionality in quickstart we must deal with this stuff and do that properly.
On the other hand, I am not surprised by the fact that introducing support for 
non-stable versions of Juju introduces some complications.
Thanks a lot for your patience.

>
https://codereview.appspot.com/132770043/diff/1/quickstart/platform_support.p...
> > 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?

Yes it will.

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

That should not be a warning technically, it asks for user interaction and 
should be considered a normal step in the quickstart execution.
If you search for "logging.warn" you'll find more examples.

>
https://codereview.appspot.com/132770043/diff/1/quickstart/platform_support.p...
> > quickstart/platform_support.py:126: else:
> > Unnecessary else block.
> > 
> 
> I'm still waiting for that team coding standard document. :)

Heh, this is more a personal preference, not strictly a coding style I guess.
The less indentation levels the better.

>
https://codereview.appspot.com/132770043/diff/1/quickstart/tests/test_platfor...
> > 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.

Me too. Perhaps that's due to the fact the function used to be really
straightforward. That's no longer the case.

Thanks a lot Jay, feel free to ping me for any questions.

[jrwren, 2014-08-22 16:47:19]

Please take a look.

[frankban, 2014-08-22 17:37:45]

Very nice branch Jay.
I have some other minor suggestions, and I am well over my EOD, so I'll QA this
on Monday.
Thank you!

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

https://codereview.appspot.com/132770043/diff/60001/quickstart/platform_suppo...
quickstart/platform_support.py:26: import platform
Pre-existing, by could you please add a new line here separating stdlib imports
from quickstart ones?

The latter must also be put in alphabetical order.

from quickstart import (
    settings,
    utils,
)

https://codereview.appspot.com/132770043/diff/60001/quickstart/platform_suppo...
quickstart/platform_support.py:117: Also return a flag indicating whetehr the
user requested to customize
Typo: whetehr.

https://codereview.appspot.com/132770043/diff/60001/quickstart/platform_suppo...
quickstart/platform_support.py:130: if juju_command != "" and juju_command !=
platform_command:
"if juju_command:" is sufficient here, an empty string evaluates to False.
See PEP8: http://legacy.python.org/dev/peps/pep-0008/#id41

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

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_app...
quickstart/tests/test_app.py:1646: class TestRequiresSudo(unittest.TestCase):
TestJujuRequiresSudo?

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_app...
quickstart/tests/test_app.py:1659:
self.assertTrue(app.juju_requires_sudo('local', version, False))
We could pass the version as second argument to assertTrue here, so that, in
case of failure, we know what version failed:

self.assertTrue(
    app.juju_requires_sudo('local', version, False), 
    version)

Or similar.

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_app...
quickstart/tests/test_app.py:1664:
self.assertFalse(app.juju_requires_sudo('local', version, False))
Ditto.

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_app...
quickstart/tests/test_app.py:1668: with self.assertRaises(app.ProgramExit):
test_app defines a ProgramExitTestsMixin, which lets you easily ensure that the
exit message is also correct, e.g.:

with self.assert_program_exit('bad wolf'):
   app.juju_requires_sudo('local', version, True)

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

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_pla...
quickstart/tests/test_platform_support.py:178: def
test_getenv_fails_when_ignore_env_var(self):
This test seems no longer valid, and we need to add a test for the case JUJU is
set to the default command (customized should be false).

Also, expected is starting to be a little be vague on those tests.
I'd read them better, for example, like:

expected_command = '/custom/juju'
with mock.patch('os.environ', {'JUJU': expected_command}):
    command, customized = platform_support.get_juju_command(None)
self.assertEquals(expected_command, command)
self.assertTrue(customized)

https://codereview.appspot.com/132770043/diff/60001/quickstart/utils.py
File quickstart/utils.py (right):

https://codereview.appspot.com/132770043/diff/60001/quickstart/utils.py#newco...
quickstart/utils.py:347: Handles the special case of no patch level by
defaulting to 0.
Since the other parts of the docstring use imperative, I'd do the same here:
s/Handles/Handle/.

https://codereview.appspot.com/132770043/diff/60001/quickstart/utils.py#newco...
quickstart/utils.py:360: try:
Uhm... What if the ValueError is due to the int conversion?
Do we need to retry the same int conversion twice?
What do you think about something like:
    parts = version_string.split('.', 2)
    if len(parts) == 2:
        parts.append(0)
    try:
        major, minor, patch = map(int, parts)
    ...

[jrwren, 2014-08-22 18:44:50]

Please take a look.

[frankban, 2014-08-25 14:37:39]

LGTM with a couple of minor changes.
QA ok.
Thank you!

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

https://codereview.appspot.com/132770043/diff/80001/quickstart/platform_suppo...
quickstart/platform_support.py:30: utils
Missing comma at the end of the line (utils,).

https://codereview.appspot.com/132770043/diff/80001/quickstart/platform_suppo...
quickstart/platform_support.py:134: logging.warn("a customized juju is being
used.")
no need for the final . here

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

https://codereview.appspot.com/132770043/diff/80001/quickstart/tests/test_pla...
quickstart/tests/test_platform_support.py:170: class
TestGetJujuCommand(unittest.TestCase):
What do you think about changing from expected/actual to
expected_command/assertTrue also in the first and third tests here?
I read the second one way better.

https://codereview.appspot.com/132770043/diff/80001/quickstart/tests/test_pla...
quickstart/tests/test_platform_support.py:178: def
test_getenv_fails_when_ignore_env_var(self):
The test name here seems to no longer describe the goal.

[jrwren, 2014-08-25 14:46:40]

Please take a look.

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

https://codereview.appspot.com/132770043/diff/80001/quickstart/tests/test_pla...
quickstart/tests/test_platform_support.py:170: class
TestGetJujuCommand(unittest.TestCase):
On 2014/08/25 14:37:39, frankban wrote:
> What do you think about changing from expected/actual to
> expected_command/assertTrue also in the first and third tests here?
> I read the second one way better.

Done.

https://codereview.appspot.com/132770043/diff/80001/quickstart/tests/test_pla...
quickstart/tests/test_platform_support.py:178: def
test_getenv_fails_when_ignore_env_var(self):
On 2014/08/25 14:37:38, frankban wrote:
> The test name here seems to no longer describe the goal.

Done.

[bac, 2014-08-25 15:36:49]

The branch LGTM Jay.  Thanks for getting it done and working with Francesco to
improve it.

https://codereview.appspot.com/132770043/diff/100001/quickstart/app.py
File quickstart/app.py (right):

https://codereview.appspot.com/132770043/diff/100001/quickstart/app.py#newcod...
quickstart/app.py:550: The env_type argument is the current environment type.
I haven't looked at the quickstart code in a while so I didn't immediately know
what an 'env_type' was but could guess it meant "environment type". Then I was
in the unhappy place of not knowing what an environment type was.

Perhaps you could augment this comment with "e.g. local, ec2, azure".

[jrwren, 2014-08-25 16:18:33]

Please take a look.

[jrwren, 2014-08-25 16:58:41]

Please take a look.

[jrwren, 2014-08-25 17:00:22]

*** Submitted:

Set juju binary with JUJU env var

Print a warning if a customized juju is being used.

Do not continue if juju must be executed using sudo.

/usr/bin/juju was hard-coded.  Made interoperability testing with
  pre-release versions of juju difficult. This allows override with JUJU
  environment var.

R=frankban, bac
CC=
https://codereview.appspot.com/132770043