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

Proposed by Jay R. Wren
Status: Merged
Merged at revision: 96
Proposed branch: lp:~evarlast/juju-quickstart/which-juju
Merge into: lp:juju-quickstart
Diff against target: 290 lines (+125/-15)
8 files modified
quickstart/app.py (+18/-0)
quickstart/manage.py (+5/-4)
quickstart/platform_support.py (+18/-3)
quickstart/tests/test_app.py (+29/-0)
quickstart/tests/test_manage.py (+25/-4)
quickstart/tests/test_platform_support.py (+15/-0)
quickstart/tests/test_utils.py (+8/-2)
quickstart/utils.py (+7/-2)
To merge this branch: bzr merge lp:~evarlast/juju-quickstart/which-juju
Reviewer Review Type Date Requested Status
Juju GUI Hackers Pending
Review via email: mp+227238@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Francesco Banconi (frankban) wrote :

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?

Revision history for this message
Francesco Banconi (frankban) wrote :

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.

91. By Jay R. Wren

addressing review concerns

Revision history for this message
Francesco Banconi (frankban) wrote :

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.

Revision history for this message
Jay R. Wren (evarlast) wrote :

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

Revision history for this message
Francesco Banconi (frankban) wrote :

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.

92. By Jay R. Wren

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.

Revision history for this message
Jay R. Wren (evarlast) wrote :
Download full text (5.3 KiB)

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

Read more...

Revision history for this message
Francesco Banconi (frankban) wrote :
Download full text (3.6 KiB)

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

Read more...

93. By Jay R. Wren

unnecessary if block

Revision history for this message
Jay R. Wren (evarlast) wrote :
Download full text (4.3 KiB)

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

Read more...

Revision history for this message
Jay R. Wren (evarlast) wrote :
94. By Jay R. Wren

use logging.warn instead of print

95. By Jay R. Wren

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.

Revision history for this message
Jay R. Wren (evarlast) wrote :
Revision history for this message
Francesco Banconi (frankban) wrote :
Download full text (5.7 KiB)

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

Read more...

96. By Jay R. Wren

refactor manage.run

extract requires_sudo

Revision history for this message
Jay R. Wren (evarlast) wrote :
Revision history for this message
Francesco Banconi (frankban) wrote :
Download full text (4.0 KiB)

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

Read more...

97. By Jay R. Wren

pep8; convert once

Revision history for this message
Jay R. Wren (evarlast) wrote :
Revision history for this message
Francesco Banconi (frankban) wrote :

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/

98. By Jay R. Wren

commas; periods; names

Revision history for this message
Jay R. Wren (evarlast) wrote :

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/

Revision history for this message
Brad Crittenden (bac) wrote :

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/

99. By Jay R. Wren

document env_type in juju_requires_sudo

Revision history for this message
Jay R. Wren (evarlast) wrote :
100. By Jay R. Wren

merge upstream

Revision history for this message
Jay R. Wren (evarlast) wrote :
Revision history for this message
Jay R. Wren (evarlast) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
549 env.deploy_bundle(bundle_yaml, name=bundle_name, bundle_id=bundle_id)549 env.deploy_bundle(bundle_yaml, name=bundle_name, bundle_id=bundle_id)
550 except jujuclient.EnvError as err:550 except jujuclient.EnvError as err:
551 raise ProgramExit('bad API server response: {}'.format(err.message))551 raise ProgramExit('bad API server response: {}'.format(err.message))
552
553
554def juju_requires_sudo(env_type, juju_version, customized):
555 """Report whether the given Juju version requires "sudo".
556
557 The env_type argument is the current environment type.
558 "e.g. local, ec2, azure"
559
560 Raise a ProgramExit if a customized version of Juju is being used and Juju
561 itself requires "sudo"
562 """
563 # If the Juju version is less than 1.17.2 then use sudo for local envs.
564 if env_type == 'local' and juju_version < (1, 17, 2):
565 if customized:
566 raise ProgramExit(b'cannot use a customized Juju when it '
567 'requires sudo')
568 return True
569 return False
552570
=== 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 @@
496 print('contents loaded for bundle {} (services: {})'.format(496 print('contents loaded for bundle {} (services: {})'.format(
497 options.bundle_name, len(options.bundle_services)))497 options.bundle_name, len(options.bundle_services)))
498498
499 juju_command = platform_support.get_juju_command(options.platform)499 juju_command, custom_juju = platform_support.get_juju_command(
500 options.platform)
500501
501 logging.debug('ensuring juju and dependencies are installed')502 logging.debug('ensuring juju and dependencies are installed')
502 juju_version = app.ensure_dependencies(503 juju_version = app.ensure_dependencies(
503 options.distro_only, options.platform, juju_command)504 options.distro_only, options.platform, juju_command)
504505
506 requires_sudo = app.juju_requires_sudo(
507 options.env_type, juju_version, custom_juju)
508
505 logging.debug('ensuring SSH keys are available')509 logging.debug('ensuring SSH keys are available')
506 app.ensure_ssh_keys()510 app.ensure_ssh_keys()
507511
508 print('bootstrapping the {} environment (type: {})'.format(512 print('bootstrapping the {} environment (type: {})'.format(
509 options.env_name, options.env_type))513 options.env_name, options.env_type))
510 requires_sudo = False
511 if options.env_type == 'local':514 if options.env_type == 'local':
512 # If this is a local environment, notify the user that "sudo" will be515 # If this is a local environment, notify the user that "sudo" will be
513 # required to bootstrap the application, even in newer Juju versions516 # required to bootstrap the application, even in newer Juju versions
514 # where "sudo" is invoked by juju-core itself.517 # where "sudo" is invoked by juju-core itself.
515 print('sudo privileges will be required to bootstrap the environment')518 print('sudo privileges will be required to bootstrap the environment')
516 # If the Juju version is less than 1.17.2 then use sudo for local envs.
517 requires_sudo = juju_version < (1, 17, 2)
518519
519 already_bootstrapped, bootstrap_node_series = app.bootstrap(520 already_bootstrapped, bootstrap_node_series = app.bootstrap(
520 options.env_name, juju_command,521 options.env_name, juju_command,
521522
=== 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 @@
21 unicode_literals,21 unicode_literals,
22)22)
2323
24import logging
24import os25import os
25import platform26import platform
26from quickstart import utils27
27from quickstart import settings28from quickstart import (
29 settings,
30 utils,
31)
2832
2933
30def validate_platform(pf):34def validate_platform(pf):
@@ -113,12 +117,23 @@
113def get_juju_command(platform):117def get_juju_command(platform):
114 """Return the path to the Juju command on the given platform.118 """Return the path to the Juju command on the given platform.
115119
120 Also return a flag indicating whether the user requested to customize
121 the Juju command by providing a JUJU environment variable.
122
116 If the platform does not have a novel location, the default will be123 If the platform does not have a novel location, the default will be
117 returned.124 returned.
125
126 If the environment variable JUJU is set, then its value will be
127 returned.
118 """128 """
119 return settings.JUJU_CMD_PATHS.get(129 juju_command = os.getenv('JUJU', '').strip()
130 platform_command = settings.JUJU_CMD_PATHS.get(
120 platform,131 platform,
121 settings.JUJU_CMD_PATHS['default'])132 settings.JUJU_CMD_PATHS['default'])
133 if juju_command and juju_command != platform_command:
134 logging.warn("a customized juju is being used")
135 return juju_command, True
136 return platform_command, False
122137
123138
124def get_juju_installer(platform):139def get_juju_installer(platform):
125140
=== 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 @@
1677 with self.assertRaises(ValueError) as context_manager:1677 with self.assertRaises(ValueError) as context_manager:
1678 app.deploy_bundle(env, self.yaml, self.name, None)1678 app.deploy_bundle(env, self.yaml, self.name, None)
1679 self.assertIs(error, context_manager.exception)1679 self.assertIs(error, context_manager.exception)
1680
1681
1682class TestJujuRequiresSudo(ProgramExitTestsMixin, unittest.TestCase):
1683 no_sudo_versions = [
1684 (1, 17, 2), (1, 17, 10), (1, 18, 0), (1, 18, 2), (2, 16, 1)]
1685 sudo_versions = [(0, 7, 9), (1, 0, 0), (1, 16, 42), (1, 17, 0), (1, 17, 1)]
1686
1687 def test_non_local_returns_false(self):
1688 # A non-local provider does not require sudo.
1689 actual = app.juju_requires_sudo('aws', None, None)
1690 self.assertFalse(actual)
1691
1692 def test_local_old_juju_returns_true(self):
1693 # A juju previous to 1.17.2 requires sudo.
1694 for version in self.sudo_versions:
1695 self.assertTrue(app.juju_requires_sudo('local', version, False),
1696 version)
1697
1698 def test_local_newer_juju_returns_false(self):
1699 # A juju 1.17.2 and newer does not require sudo.
1700 for version in self.no_sudo_versions:
1701 self.assertFalse(app.juju_requires_sudo('local', version, False),
1702 version)
1703
1704 def test_raises_programexit(self):
1705 for version in self.sudo_versions:
1706 with self.assert_program_exit('cannot use a customized Juju when '
1707 'it requires sudo'):
1708 app.juju_requires_sudo('local', version, True)
16801709
=== 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 @@
795 mock_app.watch.return_value = '1.2.3.4'795 mock_app.watch.return_value = '1.2.3.4'
796 # Make mock_app.create_auth_token return a fake authentication token.796 # Make mock_app.create_auth_token return a fake authentication token.
797 mock_app.create_auth_token.return_value = 'AUTHTOKEN'797 mock_app.create_auth_token.return_value = 'AUTHTOKEN'
798 mock_app.juju_requires_sudo.return_value = False
798 options = self.make_options()799 options = self.make_options()
799 with mock.patch('quickstart.manage.platform_support.get_juju_command',800 with mock.patch('quickstart.manage.platform_support.get_juju_command',
800 side_effect=[self.juju_command]):801 side_effect=[(self.juju_command, False)]):
801 manage.run(options)802 manage.run(options)
802 mock_app.ensure_dependencies.assert_called()803 mock_app.ensure_dependencies.assert_called()
803 mock_app.ensure_ssh_keys.assert_called()804 mock_app.ensure_ssh_keys.assert_called()
@@ -878,11 +879,12 @@
878 # where to deploy the charm, the service and unit data.879 # where to deploy the charm, the service and unit data.
879 mock_app.check_environment.return_value = (880 mock_app.check_environment.return_value = (
880 'cs:precise/juju-gui-42', '0', None, None)881 'cs:precise/juju-gui-42', '0', None, None)
882 mock_app.juju_requires_sudo.return_value = False
881 for version in versions:883 for version in versions:
882 mock_app.ensure_dependencies.return_value = version884 mock_app.ensure_dependencies.return_value = version
883 with mock.patch(885 with mock.patch(
884 'quickstart.manage.platform_support.get_juju_command',886 'quickstart.manage.platform_support.get_juju_command',
885 side_effect=[self.juju_command]):887 side_effect=[(self.juju_command, False)]):
886 manage.run(options)888 manage.run(options)
887 mock_app.bootstrap.assert_called_once_with(889 mock_app.bootstrap.assert_called_once_with(
888 options.env_name, self.juju_command, requires_sudo=False,890 options.env_name, self.juju_command, requires_sudo=False,
@@ -904,11 +906,12 @@
904 # where to deploy the charm, the service and unit data.906 # where to deploy the charm, the service and unit data.
905 mock_app.check_environment.return_value = (907 mock_app.check_environment.return_value = (
906 'cs:trusty/juju-gui-42', '0', None, None)908 'cs:trusty/juju-gui-42', '0', None, None)
909 mock_app.juju_requires_sudo.return_value = True
907 for version in versions:910 for version in versions:
908 mock_app.ensure_dependencies.return_value = version911 mock_app.ensure_dependencies.return_value = version
909 with mock.patch(912 with mock.patch(
910 'quickstart.manage.platform_support.get_juju_command',913 'quickstart.manage.platform_support.get_juju_command',
911 side_effect=[self.juju_command]):914 side_effect=[(self.juju_command, False)]):
912 manage.run(options)915 manage.run(options)
913 mock_app.bootstrap.assert_called_once_with(916 mock_app.bootstrap.assert_called_once_with(
914 options.env_name, self.juju_command, requires_sudo=True,917 options.env_name, self.juju_command, requires_sudo=True,
@@ -928,8 +931,9 @@
928 # where to deploy the charm, the service and unit data.931 # where to deploy the charm, the service and unit data.
929 mock_app.check_environment.return_value = (932 mock_app.check_environment.return_value = (
930 'cs:precise/juju-gui-42', '0', None, None)933 'cs:precise/juju-gui-42', '0', None, None)
934 mock_app.juju_requires_sudo.return_value = False
931 with mock.patch('quickstart.manage.platform_support.get_juju_command',935 with mock.patch('quickstart.manage.platform_support.get_juju_command',
932 side_effect=[self.juju_command]):936 side_effect=[(self.juju_command, False)]):
933 manage.run(options)937 manage.run(options)
934 mock_app.bootstrap.assert_called_once_with(938 mock_app.bootstrap.assert_called_once_with(
935 options.env_name, self.juju_command,939 options.env_name, self.juju_command,
@@ -1006,3 +1010,20 @@
1006 u'admin-secret not found in {}/environments/local.jenv '1010 u'admin-secret not found in {}/environments/local.jenv '
1007 'or environments.yaml'.format(settings.JUJU_HOME))1011 'or environments.yaml'.format(settings.JUJU_HOME))
1008 self.assertEqual(expected, context.exception.message)1012 self.assertEqual(expected, context.exception.message)
1013
1014 def test_juju_environ_var_set(self, mock_app, mock_open):
1015 mock_app.bootstrap.return_value = (True, 'precise')
1016 mock_app.check_environment.return_value = (
1017 'cs:precise/juju-gui-42', '0', None, None)
1018 mock_app.juju_requires_sudo.return_value = False
1019 options = self.make_options(env_type='aws')
1020 juju_command = 'some/devel/path/juju'
1021 with mock.patch('os.environ', {'JUJU': juju_command}):
1022 manage.run(options)
1023 mock_app.bootstrap.assert_called_once_with(
1024 options.env_name, juju_command, requires_sudo=False,
1025 debug=options.debug, upload_tools=options.upload_tools,
1026 upload_series=options.upload_series,
1027 constraints=options.constraints)
1028 mock_app.get_api_url.assert_called_once_with(
1029 options.env_name, juju_command)
10091030
=== 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 @@
165 def test_linux_apt_passes(self):165 def test_linux_apt_passes(self):
166 result = platform_support.validate_platform(settings.LINUX_APT)166 result = platform_support.validate_platform(settings.LINUX_APT)
167 self.assertIsNone(result)167 self.assertIsNone(result)
168
169
170class TestGetJujuCommand(unittest.TestCase):
171
172 def test_getenv_succeeds(self):
173 expected_command = '/custom/juju'
174 with mock.patch('os.environ', {'JUJU': expected_command}):
175 command, customized = platform_support.get_juju_command(None)
176 self.assertEqual(expected_command, command)
177 self.assertTrue(customized)
178
179 def test_without_env_var(self):
180 expected = settings.JUJU_CMD_PATHS['default'], False
181 actual = platform_support.get_juju_command('default')
182 self.assertEqual(expected, actual)
168183
=== 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 @@
768 with self.assert_value_error('bad wolf'):768 with self.assert_value_error('bad wolf'):
769 utils.get_juju_version('juju')769 utils.get_juju_version('juju')
770770
771 def test_alpha_version_string(self):
772 # Patch level defaults to zero.
773 with self.patch_call(0, '1.17-alpha1-precise-amd64', ''):
774 version = utils.get_juju_version('juju')
775 self.assertEqual((1, 17, 0), version)
776
771 def test_invalid_version_string(self):777 def test_invalid_version_string(self):
772 # A ValueError is raised if "juju version" outputs an invalid version.778 # A ValueError is raised if "juju version" outputs an invalid version.
773 with self.patch_call(0, '1.17-precise-amd64', ''):779 with self.patch_call(0, '1-precise-amd64', ''):
774 with self.assert_value_error('invalid version string: 1.17'):780 with self.assert_value_error('invalid version string: 1'):
775 utils.get_juju_version('juju')781 utils.get_juju_version('juju')
776782
=== 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 @@
344 Return a (major:int, minor:int, patch:int) tuple, including major, minor344 Return a (major:int, minor:int, patch:int) tuple, including major, minor
345 and patch version numbers.345 and patch version numbers.
346346
347 Handle the special case of no patch level by defaulting to 0.
348
347 Raise a ValueError if the "juju version" call exits with an error349 Raise a ValueError if the "juju version" call exits with an error
348 or the returned version is not well formed.350 or the returned version is not well formed.
349 """351 """
@@ -352,8 +354,11 @@
352 raise ValueError(error.encode('utf-8'))354 raise ValueError(error.encode('utf-8'))
353 version_string = output.split('-')[0]355 version_string = output.split('-')[0]
354 try:356 try:
355 major, minor, patch = version_string.split('.', 2)357 parts = version_string.split('.', 2)
356 return int(major), int(minor), int(patch)358 if len(parts) == 2:
359 parts.append(0)
360 major, minor, patch = map(int, parts)
361 return major, minor, patch
357 except ValueError:362 except ValueError:
358 msg = 'invalid version string: {}'.format(version_string)363 msg = 'invalid version string: {}'.format(version_string)
359 raise ValueError(msg.encode('utf-8'))364 raise ValueError(msg.encode('utf-8'))

Subscribers

People subscribed via source and target branches

to all changes: