Merge lp:~evarlast/juju-quickstart/which-juju into lp:juju-quickstart

[Jay R. Wren]

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://codereview.appspot.com/132770043/

[Francesco Banconi (frankban)]

This branch is nice Jay, thank you.
However, this change makes me a little bit nervous, for two reasons.

1) In older versions of Juju, we require to bootstrap using sudo on local envs. Due to the fact we support those versions, that logic is indeed in quickstart. This means that, under certain circumstances, we can end up calling "sudo $JUJU". I am not convinced that the use cases for this change justify the security issues that can be be raised.

2) I like the fact that Quickstart only supports stable/packaged versions of juju (/usr/bin/juju). This means it's relatively easy to dupe possible bugs being filed.

As mentioned, I am not sure about introducing this feature, but if we decide to follow this path, I'd like quickstart to at least do the following:

- check that the given path exists and it's executable;
- expose a big WARNING to the user, informing she's using a customized juju binary, and requiring a user input in order to proceed.

I'd also like to s/$JUJU/$JUJU_CMD or similar.

What do you think?

[Francesco Banconi (frankban)]

9 + if os.environ.get('JUJU', False):

This can be written as:

juju_command = os.getenv('JUJU', '').strip()
if not juju_command:
    juju_command = platform_support.get_juju_command(options.platform)

However, I'd like this logic to be placed in get_juju_command(), so that the function can still be potentially re-used in other parts of the code.

[lp:~evarlast/juju-quickstart/which-juju]

91. By Jay R. Wren on 2014-08-01

addressing review concerns

[Francesco Banconi (frankban)]

Hey Jay, thanks for the work on this branch.
I don't see here the changes I requested in my initial comment.
I am still not sure about the introduction of this feature.
Maybe we should ask for another opinion.

[Jay R. Wren (evarlast)]

I somehow missed the entire first comment. I retract my request for reconsideration.

[Francesco Banconi (frankban)]

As discussed on IRC, let's change this to:
- print a warning if a customized juju is being used;
- ignore the env var (and output a warning) if we need to use sudo.

Also, please add a test for the get_juju_command changes.

[lp:~evarlast/juju-quickstart/which-juju]

92. By Jay R. Wren on 2014-08-21

custom juju warnings

- print a warning if a customized juju is being used;
- ignore the env var (and output a warning) if we need ot use sudo.

test the get_juju_command changes.

[Jay R. Wren (evarlast)]

Reviewers: mp+227238_code.launchpad.net,

Message:
Please take a look.

Description:

/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)

Please review this at https://codereview.appspot.com/132770043/

Affected files (+50, -5 lines):
   A [revision details]
   M quickstart/manage.py
   M quickstart/platform_support.py
   M quickstart/tests/test_manage.py
   M quickstart/tests/test_platform_support.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision:
<email address hidden>
+New revision: <email address hidden>

Index: quickstart/manage.py
=== modified file 'quickstart/manage.py'
--- quickstart/manage.py 2014-06-13 15:19:46 +0000
+++ quickstart/manage.py 2014-08-21 15:20:55 +0000
@@ -497,6 +497,12 @@
          print('sudo privileges will be required to bootstrap the
environment')
          # If the Juju version is less than 1.17.2 then use sudo for local
envs.
          requires_sudo = juju_version < (1, 17, 2)
+ if requires_sudo:
+ non_env_juju_command = platform_support.get_juju_command(
+ options.platform, True)
+ if non_env_juju_command != juju_command:
+ print("Warning: ignoring JUJU environment variable")
+ juju_command = non_env_juju_command

      already_bootstrapped, bootstrap_node_series = app.bootstrap(
          options.env_name, juju_command,

Index: quickstart/platform_support.py
=== modified file 'quickstart/platform_support.py'
--- quickstart/platform_support.py 2014-06-13 14:36:30 +0000
+++ quickstart/platform_support.py 2014-08-21 15:20:55 +0000
@@ -110,15 +110,23 @@
  }

-def get_juju_command(platform):
+def get_juju_command(platform, ignore_env_var=False):
      """Return the path to the Juju command on the given platform.

      If the platform does not have a novel location, the default will be
      returned.
+
+ If the environment variable JUJU is set, then its value will be
+ returned.
      """
- return settings.JUJU_CMD_PATHS.get(
- platform,
- settings.JUJU_CMD_PATHS['default'])
+ juju_command = os.getenv('JUJU', '').strip()
+ if juju_command is not None and not ignore_env_var:
+ print("Warning: a customized juju is being used.")
+ return juju_command
+ else:
+ return settings.JUJU_CMD_PATHS.get(
+ platform,
+ settings.JUJU_CMD_PATHS['default'])

  def get_juju_installer(platform):

Index: quickstart/tests/test_manage.py
=== modified file 'quickstart/tests/test_manage.py'
--- quickstart/tests/test_manage.py 2014-06-13 15:19:46 +0000
+++ quickstart/tests/test_manage.py 2014-08-21 15:20:55 +0000
@@ -898,7 +898,7 @@
              mock_app.ensure_dependencies.return_value = version
              with mock.patch(
           ...

[Francesco Banconi (frankban)]

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

[lp:~evarlast/juju-quickstart/which-juju]

93. By Jay R. Wren on 2014-08-21

unnecessary if block

[Jay R. Wren (evarlast)]

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

[Jay R. Wren (evarlast)]

Please take a look.

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

[lp:~evarlast/juju-quickstart/which-juju]

94. By Jay R. Wren on 2014-08-21

use logging.warn instead of print

95. By Jay R. Wren on 2014-08-21

re-ensure dependencies when JUJU unused

When skipping the JUJU in environment variable because sudo is required, check the dependencies again.

Add the common no JUJU environment variable test case.

[Jay R. Wren (evarlast)]

Please take a look.

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

[Francesco Banconi (frankban)]

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

[lp:~evarlast/juju-quickstart/which-juju]

96. By Jay R. Wren on 2014-08-22

refactor manage.run

extract requires_sudo

[Jay R. Wren (evarlast)]

Please take a look.

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

[Francesco Banconi (frankban)]

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_support.py
File quickstart/platform_support.py (right):

https://codereview.appspot.com/132770043/diff/60001/quickstart/platform_support.py#newcode26
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_support.py#newcode117
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_support.py#newcode130
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.py#newcode1646
quickstart/tests/test_app.py:1646: class
TestRequiresSudo(unittest.TestCase):
TestJujuRequiresSudo?

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_app.py#newcode1659
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.py#newcode1664
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.py#newcode1668
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_platform_support.py
File quickstart/tests/test_platform_support.py (right):

https://codereview.appspot.com/132770043/diff/60001/quickstart/tests/test_platform_support.py#newcode178
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_supp...

[lp:~evarlast/juju-quickstart/which-juju]

97. By Jay R. Wren on 2014-08-22

pep8; convert once

[Jay R. Wren (evarlast)]

Please take a look.

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

[Francesco Banconi (frankban)]

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

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

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

https://codereview.appspot.com/132770043/diff/80001/quickstart/platform_support.py#newcode134
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_platform_support.py
File quickstart/tests/test_platform_support.py (right):

https://codereview.appspot.com/132770043/diff/80001/quickstart/tests/test_platform_support.py#newcode170
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_platform_support.py#newcode178
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.

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

[lp:~evarlast/juju-quickstart/which-juju]

98. By Jay R. Wren on 2014-08-25

commas; periods; names

[Jay R. Wren (evarlast)]

Please take a look.

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

https://codereview.appspot.com/132770043/diff/80001/quickstart/tests/test_platform_support.py#newcode170
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_platform_support.py#newcode178
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.

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

[Brad Crittenden (bac)]

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#newcode550
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".

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

[lp:~evarlast/juju-quickstart/which-juju]

99. By Jay R. Wren on 2014-08-25

document env_type in juju_requires_sudo

[Jay R. Wren (evarlast)]

Please take a look.

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

[lp:~evarlast/juju-quickstart/which-juju]

100. By Jay R. Wren on 2014-08-25

merge upstream

[Jay R. Wren (evarlast)]

Please take a look.

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

[Jay R. Wren (evarlast)]

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

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

=== modified file 'quickstart/app.py'
--- quickstart/app.py	2014-08-21 16:24:05 +0000
+++ quickstart/app.py	2014-08-25 16:57:54 +0000
@@ -549,3 +549,21 @@
         env.deploy_bundle(bundle_yaml, name=bundle_name, bundle_id=bundle_id)
     except jujuclient.EnvError as err:
         raise ProgramExit('bad API server response: {}'.format(err.message))
+
+
+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.
+    "e.g. local, ec2, azure"
+
+    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 when it '
+                              'requires sudo')
+        return True
+    return False

=== modified file 'quickstart/manage.py'
--- quickstart/manage.py	2014-08-21 16:23:26 +0000
+++ quickstart/manage.py	2014-08-25 16:57:54 +0000
@@ -496,25 +496,26 @@
         print('contents loaded for bundle {} (services: {})'.format(
             options.bundle_name, len(options.bundle_services)))
 
-    juju_command = platform_support.get_juju_command(options.platform)
+    juju_command, custom_juju = 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, custom_juju)
+
     logging.debug('ensuring SSH keys are available')
     app.ensure_ssh_keys()
 
     print('bootstrapping the {} environment (type: {})'.format(
         options.env_name, options.env_type))
-    requires_sudo = False
     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')
-        # If the Juju version is less than 1.17.2 then use sudo for local envs.
-        requires_sudo = juju_version < (1, 17, 2)
 
     already_bootstrapped, bootstrap_node_series = app.bootstrap(
         options.env_name, juju_command,

=== modified file 'quickstart/platform_support.py'
--- quickstart/platform_support.py	2014-08-21 19:01:57 +0000
+++ quickstart/platform_support.py	2014-08-25 16:57:54 +0000
@@ -21,10 +21,14 @@
     unicode_literals,
 )
 
+import logging
 import os
 import platform
-from quickstart import utils
-from quickstart import settings
+
+from quickstart import (
+    settings,
+    utils,
+)
 
 
 def validate_platform(pf):
@@ -113,12 +117,23 @@
 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.
+
+    If the environment variable JUJU is set, then its value will be
+    returned.
     """
-    return settings.JUJU_CMD_PATHS.get(
+    juju_command = os.getenv('JUJU', '').strip()
+    platform_command = settings.JUJU_CMD_PATHS.get(
         platform,
         settings.JUJU_CMD_PATHS['default'])
+    if juju_command and juju_command != platform_command:
+        logging.warn("a customized juju is being used")
+        return juju_command, True
+    return platform_command, False
 
 
 def get_juju_installer(platform):

=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2014-08-01 15:35:56 +0000
+++ quickstart/tests/test_app.py	2014-08-25 16:57:54 +0000
@@ -1677,3 +1677,32 @@
         with self.assertRaises(ValueError) as context_manager:
             app.deploy_bundle(env, self.yaml, self.name, None)
         self.assertIs(error, context_manager.exception)
+
+
+class TestJujuRequiresSudo(ProgramExitTestsMixin, unittest.TestCase):
+    no_sudo_versions = [
+        (1, 17, 2), (1, 17, 10), (1, 18, 0), (1, 18, 2), (2, 16, 1)]
+    sudo_versions = [(0, 7, 9), (1, 0, 0), (1, 16, 42), (1, 17, 0), (1, 17, 1)]
+
+    def test_non_local_returns_false(self):
+        # A non-local provider does not require sudo.
+        actual = app.juju_requires_sudo('aws', None, None)
+        self.assertFalse(actual)
+
+    def test_local_old_juju_returns_true(self):
+        # A juju previous to 1.17.2 requires sudo.
+        for version in self.sudo_versions:
+            self.assertTrue(app.juju_requires_sudo('local', version, False),
+                            version)
+
+    def test_local_newer_juju_returns_false(self):
+        # A juju 1.17.2 and newer does not require sudo.
+        for version in self.no_sudo_versions:
+            self.assertFalse(app.juju_requires_sudo('local', version, False),
+                             version)
+
+    def test_raises_programexit(self):
+        for version in self.sudo_versions:
+            with self.assert_program_exit('cannot use a customized Juju when '
+                                          'it requires sudo'):
+                app.juju_requires_sudo('local', version, True)

=== modified file 'quickstart/tests/test_manage.py'
--- quickstart/tests/test_manage.py	2014-08-01 15:35:56 +0000
+++ quickstart/tests/test_manage.py	2014-08-25 16:57:54 +0000
@@ -795,9 +795,10 @@
         mock_app.watch.return_value = '1.2.3.4'
         # Make mock_app.create_auth_token return a fake authentication token.
         mock_app.create_auth_token.return_value = 'AUTHTOKEN'
+        mock_app.juju_requires_sudo.return_value = False
         options = self.make_options()
         with mock.patch('quickstart.manage.platform_support.get_juju_command',
-                        side_effect=[self.juju_command]):
+                        side_effect=[(self.juju_command, False)]):
             manage.run(options)
         mock_app.ensure_dependencies.assert_called()
         mock_app.ensure_ssh_keys.assert_called()
@@ -878,11 +879,12 @@
         # where to deploy the charm, the service and unit data.
         mock_app.check_environment.return_value = (
             'cs:precise/juju-gui-42', '0', None, None)
+        mock_app.juju_requires_sudo.return_value = False
         for version in versions:
             mock_app.ensure_dependencies.return_value = version
             with mock.patch(
                     'quickstart.manage.platform_support.get_juju_command',
-                    side_effect=[self.juju_command]):
+                    side_effect=[(self.juju_command, False)]):
                 manage.run(options)
             mock_app.bootstrap.assert_called_once_with(
                 options.env_name, self.juju_command, requires_sudo=False,
@@ -904,11 +906,12 @@
         # where to deploy the charm, the service and unit data.
         mock_app.check_environment.return_value = (
             'cs:trusty/juju-gui-42', '0', None, None)
+        mock_app.juju_requires_sudo.return_value = True
         for version in versions:
             mock_app.ensure_dependencies.return_value = version
             with mock.patch(
                     'quickstart.manage.platform_support.get_juju_command',
-                    side_effect=[self.juju_command]):
+                    side_effect=[(self.juju_command, False)]):
                 manage.run(options)
             mock_app.bootstrap.assert_called_once_with(
                 options.env_name, self.juju_command, requires_sudo=True,
@@ -928,8 +931,9 @@
         # where to deploy the charm, the service and unit data.
         mock_app.check_environment.return_value = (
             'cs:precise/juju-gui-42', '0', None, None)
+        mock_app.juju_requires_sudo.return_value = False
         with mock.patch('quickstart.manage.platform_support.get_juju_command',
-                        side_effect=[self.juju_command]):
+                        side_effect=[(self.juju_command, False)]):
             manage.run(options)
         mock_app.bootstrap.assert_called_once_with(
             options.env_name, self.juju_command,
@@ -1006,3 +1010,20 @@
             u'admin-secret not found in {}/environments/local.jenv '
             'or environments.yaml'.format(settings.JUJU_HOME))
         self.assertEqual(expected, context.exception.message)
+
+    def test_juju_environ_var_set(self, mock_app, mock_open):
+        mock_app.bootstrap.return_value = (True, 'precise')
+        mock_app.check_environment.return_value = (
+            'cs:precise/juju-gui-42', '0', None, None)
+        mock_app.juju_requires_sudo.return_value = False
+        options = self.make_options(env_type='aws')
+        juju_command = 'some/devel/path/juju'
+        with mock.patch('os.environ', {'JUJU': juju_command}):
+            manage.run(options)
+        mock_app.bootstrap.assert_called_once_with(
+            options.env_name, juju_command, requires_sudo=False,
+            debug=options.debug, upload_tools=options.upload_tools,
+            upload_series=options.upload_series,
+            constraints=options.constraints)
+        mock_app.get_api_url.assert_called_once_with(
+            options.env_name, juju_command)

=== modified file 'quickstart/tests/test_platform_support.py'
--- quickstart/tests/test_platform_support.py	2014-06-13 14:38:05 +0000
+++ quickstart/tests/test_platform_support.py	2014-08-25 16:57:54 +0000
@@ -165,3 +165,18 @@
     def test_linux_apt_passes(self):
         result = platform_support.validate_platform(settings.LINUX_APT)
         self.assertIsNone(result)
+
+
+class TestGetJujuCommand(unittest.TestCase):
+
+    def test_getenv_succeeds(self):
+        expected_command = '/custom/juju'
+        with mock.patch('os.environ', {'JUJU': expected_command}):
+            command, customized = platform_support.get_juju_command(None)
+        self.assertEqual(expected_command, command)
+        self.assertTrue(customized)
+
+    def test_without_env_var(self):
+        expected = settings.JUJU_CMD_PATHS['default'], False
+        actual = platform_support.get_juju_command('default')
+        self.assertEqual(expected, actual)

=== modified file 'quickstart/tests/test_utils.py'
--- quickstart/tests/test_utils.py	2014-06-02 12:56:24 +0000
+++ quickstart/tests/test_utils.py	2014-08-25 16:57:54 +0000
@@ -768,8 +768,14 @@
             with self.assert_value_error('bad wolf'):
                 utils.get_juju_version('juju')
 
+    def test_alpha_version_string(self):
+        # Patch level defaults to zero.
+        with self.patch_call(0, '1.17-alpha1-precise-amd64', ''):
+            version = utils.get_juju_version('juju')
+        self.assertEqual((1, 17, 0), version)
+
     def test_invalid_version_string(self):
         # A ValueError is raised if "juju version" outputs an invalid version.
-        with self.patch_call(0, '1.17-precise-amd64', ''):
-            with self.assert_value_error('invalid version string: 1.17'):
+        with self.patch_call(0, '1-precise-amd64', ''):
+            with self.assert_value_error('invalid version string: 1'):
                 utils.get_juju_version('juju')

=== modified file 'quickstart/utils.py'
--- quickstart/utils.py	2014-06-02 12:56:24 +0000
+++ quickstart/utils.py	2014-08-25 16:57:54 +0000
@@ -344,6 +344,8 @@
     Return a (major:int, minor:int, patch:int) tuple, including major, minor
     and patch version numbers.
 
+    Handle the special case of no patch level by defaulting to 0.
+
     Raise a ValueError if the "juju version" call exits with an error
     or the returned version is not well formed.
     """
@@ -352,8 +354,11 @@
         raise ValueError(error.encode('utf-8'))
     version_string = output.split('-')[0]
     try:
-        major, minor, patch = version_string.split('.', 2)
-        return int(major), int(minor), int(patch)
+        parts = version_string.split('.', 2)
+        if len(parts) == 2:
+            parts.append(0)
+        major, minor, patch = map(int, parts)
+        return major, minor, patch
     except ValueError:
         msg = 'invalid version string: {}'.format(version_string)
         raise ValueError(msg.encode('utf-8'))