Code review comment for lp:~evarlast/juju-quickstart/which-juju

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(
                      'quickstart.manage.platform_support.get_juju_command',
- side_effect=[self.juju_command]):
+ side_effect=[self.juju_command, self.juju_command]):
                  manage.run(options)
              mock_app.bootstrap.assert_called_once_with(
                  options.env_name, self.juju_command, requires_sudo=True,
@@ -991,3 +991,17 @@
              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)
+ 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)
+ mock_app.get_api_url.assert_called_once_with(
+ options.env_name, juju_command)

Index: quickstart/tests/test_platform_support.py
=== 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-21 15:20:55 +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 = '/custom/juju'
+ with mock.patch('os.environ', {'JUJU': expected}):
+ actual = platform_support.get_juju_command(None)
+ self.assertEquals(expected, actual)
+
+ def test_getenv_fails_when_ignore_env_var(self):
+ unexpected = '/custom/juju'
+ with mock.patch('os.environ', {'JUJU': unexpected}):
+ actual = platform_support.get_juju_command(None, True)
+ self.assertNotEquals(unexpected, actual)