Merge lp:~frankban/juju-quickstart/prepare-new-release into lp:juju-quickstart

[Francesco Banconi]

Minor changes in preparation for release.

Bump version up to 1.1.0.
Re-organize the "requires sudo" code.
Restore 100% unit test coverage (compulsive mode on).
Update license headers of modified files.

Tests: `make check`
QA: use quickstart as usual, bootstrapping
both ec2 and local environments.

https://codereview.appspot.com/65970043/

[Francesco Banconi (frankban)]

Reviewers: mp+207145_code.launchpad.net,

Message:
Please take a look.

Description:
Minor changes in preparation for release.

Bump version up to 1.1.0.
Re-organize the "requires sudo" code.
Restore 100% unit test coverage (compulsive mode on).
Update license headers of modified files.

Tests: `make check`
QA: use quickstart as usual, bootstrapping
both ec2 and local environments.

https://code.launchpad.net/~frankban/juju-quickstart/prepare-new-release/+merge/207145

(do not edit description out of merge proposal)

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

Affected files (+93, -71 lines):
   A [revision details]
   M quickstart/__init__.py
   M quickstart/app.py
   M quickstart/manage.py
   M quickstart/settings.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_manage.py
   M quickstart/tests/test_utils.py
   M quickstart/utils.py

[Richard Harding (rharding)]

LGTM with the one note on potential room for a docs update.

Will QA.

https://codereview.appspot.com/65970043/diff/1/quickstart/manage.py
File quickstart/manage.py (right):

https://codereview.appspot.com/65970043/diff/1/quickstart/manage.py#newcode392
quickstart/manage.py:392: requires_sudo =
app.bootstrap_requires_sudo(is_local)
yay for functions that can be tested in isolation.

https://codereview.appspot.com/65970043/diff/1/quickstart/settings.py
File quickstart/settings.py (right):

https://codereview.appspot.com/65970043/diff/1/quickstart/settings.py#newcode35
quickstart/settings.py:35: JUJU_DEFAULT_SERIES = ('precise', 'quantal',
'raring', 'saucy', 'trusty')
good call, are these tweaks needed to keep it up to date noted? Maybe a
releases section in the hacking doc can note to check the latest charm
version and such?

https://codereview.appspot.com/65970043/

[Richard Harding (rharding)]

QA ok

https://codereview.appspot.com/65970043/

[lp:~frankban/juju-quickstart/prepare-new-release]

55. By Francesco Banconi on 2014-02-19

Changes as per review.

[Francesco Banconi (frankban)]

*** Submitted:

Minor changes in preparation for release.

Bump version up to 1.1.0.
Re-organize the "requires sudo" code.
Restore 100% unit test coverage (compulsive mode on).
Update license headers of modified files.

Tests: `make check`
QA: use quickstart as usual, bootstrapping
both ec2 and local environments.

R=rharding
CC=
https://codereview.appspot.com/65970043

https://codereview.appspot.com/65970043/diff/1/quickstart/settings.py
File quickstart/settings.py (right):

https://codereview.appspot.com/65970043/diff/1/quickstart/settings.py#newcode35
quickstart/settings.py:35: JUJU_DEFAULT_SERIES = ('precise', 'quantal',
'raring', 'saucy', 'trusty')
On 2014/02/19 13:29:09, rharding wrote:
> good call, are these tweaks needed to keep it up to date noted? Maybe
a releases
> section in the hacking doc can note to check the latest charm version
and such?

Done.

https://codereview.appspot.com/65970043/

[Francesco Banconi (frankban)]

Thank you!

https://codereview.appspot.com/65970043/

=== modified file 'HACKING.rst'
--- HACKING.rst	2014-01-30 12:26:02 +0000
+++ HACKING.rst	2014-02-19 13:55:43 +0000
@@ -134,6 +134,11 @@
 precise to saucy), or in the Juju Quickstart Beta PPA: see
 <https://launchpad.net/~juju-gui/+archive/quickstart-beta/+packages>.
 
+Please also keep up to date the possible values for the environments.yaml
+default-series field (see ``quickstart.settings.JUJU_DEFAULT_SERIES``) and the
+set of series supported by the Juju GUI charm
+(see ``quickstart.settings.JUJU_GUI_SUPPORTED_SERIES``).
+
 Debugging bundle support
 ~~~~~~~~~~~~~~~~~~~~~~~~
 
 
=== modified file 'quickstart/__init__.py'
--- quickstart/__init__.py	2014-01-14 09:28:17 +0000
+++ quickstart/__init__.py	2014-02-19 13:55:43 +0000
@@ -1,6 +1,6 @@
 # This file is part of the Juju Quickstart Plugin, which lets users set up a
 # Juju environment in very few steps (https://launchpad.net/juju-quickstart).
-# Copyright (C) 2013 Canonical Ltd.
+# Copyright (C) 2013-2014 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it under
 # the terms of the GNU Affero General Public License version 3, as published by
@@ -22,7 +22,7 @@
 from __future__ import unicode_literals
 
 
-VERSION = (1, 0, 0)
+VERSION = (1, 1, 0)
 
 
 def get_version():
 
=== modified file 'quickstart/app.py'
--- quickstart/app.py	2014-01-31 21:00:06 +0000
+++ quickstart/app.py	2014-02-19 13:55:43 +0000
@@ -1,6 +1,6 @@
 # This file is part of the Juju Quickstart Plugin, which lets users set up a
 # Juju environment in very few steps (https://launchpad.net/juju-quickstart).
-# Copyright (C) 2013 Canonical Ltd.
+# Copyright (C) 2013-2014 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it under
 # the terms of the GNU Affero General Public License version 3, as published by
@@ -216,6 +216,27 @@
     print('a new ssh key was generated in {}'.format(key_file))
 
 
+def bootstrap_requires_sudo(is_local):
+    """Return True if "sudo" is required to bootstrap the Juju environment.
+
+    Return False otherwise.
+    Raise a ProgramExit if any error occurs retrieving the Juju version.
+    """
+    if not is_local:
+        return False
+    # 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 required to bootstrap the environment')
+    # If the Juju core version is less than 1.17.2 then use sudo for local
+    # deployments.
+    try:
+        major, minor, patch = utils.get_juju_version()
+    except ValueError as err:
+        raise ProgramExit(bytes(err))
+    return (major, minor, patch) < (1, 17, 2)
+
+
 def bootstrap(env_name, requires_sudo=False, debug=False):
     """Bootstrap the Juju environment with the given name.
 
@@ -238,7 +259,6 @@
     already_bootstrapped = False
     cmd = ['/usr/bin/juju', 'bootstrap', '-e', env_name]
     if requires_sudo:
-        print('sudo privileges required to bootstrap the environment')
         cmd.insert(0, 'sudo')
     if debug:
         cmd.append('--debug')
 
=== modified file 'quickstart/manage.py'
--- quickstart/manage.py	2014-01-31 20:51:16 +0000
+++ quickstart/manage.py	2014-02-19 13:55:43 +0000
@@ -1,6 +1,6 @@
 # This file is part of the Juju Quickstart Plugin, which lets users set up a
 # Juju environment in very few steps (https://launchpad.net/juju-quickstart).
-# Copyright (C) 2013 Canonical Ltd.
+# Copyright (C) 2013-2014 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it under
 # the terms of the GNU Affero General Public License version 3, as published by
@@ -389,16 +389,7 @@
     print('bootstrapping the {} environment (type: {})'.format(
         options.env_name, options.env_type))
     is_local = options.env_type == 'local'
-    requires_sudo = False
-    if is_local:
-        # If the Juju core version is less than 1.17.2 then
-        # use sudo for local deployments.
-        try:
-            major, minor, patch = utils.get_juju_version()
-        except ValueError as err:
-            raise app.ProgramExit(bytes(err))
-        requires_sudo = utils.local_bootstrap_requires_sudo(
-            major, minor, patch)
+    requires_sudo = app.bootstrap_requires_sudo(is_local)
     already_bootstrapped, bsn_series = app.bootstrap(
         options.env_name, requires_sudo=requires_sudo, debug=options.debug)
 
 
=== modified file 'quickstart/settings.py'
--- quickstart/settings.py	2013-12-10 17:22:44 +0000
+++ quickstart/settings.py	2014-02-19 13:55:43 +0000
@@ -1,6 +1,6 @@
 # This file is part of the Juju Quickstart Plugin, which lets users set up a
 # Juju environment in very few steps (https://launchpad.net/juju-quickstart).
-# Copyright (C) 2013 Canonical Ltd.
+# Copyright (C) 2013-2014 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it under
 # the terms of the GNU Affero General Public License version 3, as published by
@@ -26,13 +26,13 @@
 
 # The default Juju GUI charm URL to use when it is not possible to retrieve it
 # from the charmworld API, e.g. due to temporary connection/charmworld errors.
-DEFAULT_CHARM_URL = 'cs:precise/juju-gui-80'
+DEFAULT_CHARM_URL = 'cs:precise/juju-gui-83'
 
 # The quickstart app short description.
 DESCRIPTION = 'set up a Juju environment (including the GUI) in very few steps'
 
 # The possible values for the environments.yaml default-series field.
-JUJU_DEFAULT_SERIES = ('precise', 'quantal', 'raring', 'saucy')
+JUJU_DEFAULT_SERIES = ('precise', 'quantal', 'raring', 'saucy', 'trusty')
 
 # Retrieve the current juju-core home.
 JUJU_HOME = os.getenv('JUJU_HOME', '~/.juju')
 
=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2014-01-31 21:38:59 +0000
+++ quickstart/tests/test_app.py	2014-02-19 13:55:43 +0000
@@ -1,6 +1,6 @@
 # This file is part of the Juju Quickstart Plugin, which lets users set up a
 # Juju environment in very few steps (https://launchpad.net/juju-quickstart).
-# Copyright (C) 2013 Canonical Ltd.
+# Copyright (C) 2013-2014 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it under
 # the terms of the GNU Affero General Public License version 3, as published by
@@ -400,6 +400,56 @@
 
 
 @helpers.mock_print
+class TestBootstrapRequiresSudo(ProgramExitTestsMixin, unittest.TestCase):
+
+    sudo_message = 'sudo privileges required to bootstrap the environment'
+
+    def patch_get_juju_version(self, return_value):
+        """Patch the quickstart.utils.get_juju_version function."""
+        if isinstance(return_value, Exception):
+            mock_get_juju_version = mock.Mock(side_effect=return_value)
+        else:
+            mock_get_juju_version = mock.Mock(return_value=return_value)
+        return mock.patch(
+            'quickstart.utils.get_juju_version', mock_get_juju_version)
+
+    def test_not_local(self, mock_print):
+        # Sudo privileges are never required for non-local environments.
+        self.assertFalse(app.bootstrap_requires_sudo(False))
+
+    def test_sudo_required(self, mock_print):
+        # Sudo privileges are required if the Juju version is < 1.17.2.
+        versions = [(0, 7, 9), (1, 0, 0), (1, 16, 42), (1, 17, 0), (1, 17, 1)]
+        for version in versions:
+            with self.patch_get_juju_version(version):
+                requires_sudo = app.bootstrap_requires_sudo(True)
+            self.assertTrue(requires_sudo, version)
+            # On local environments the "sudo privileges required" message is
+            # always printed.
+            mock_print.assert_called_once_with(self.sudo_message)
+            mock_print.reset_mock()
+
+    def test_sudo_not_required(self, mock_print):
+        # Sudo privileges are not required if the Juju version is >= 1.17.2.
+        versions = [
+            (1, 17, 2), (1, 17, 10), (1, 18, 0), (1, 18, 2), (2, 16, 1)]
+        for version in versions:
+            with self.patch_get_juju_version(version):
+                requires_sudo = app.bootstrap_requires_sudo(True)
+            self.assertFalse(requires_sudo, version)
+            # On local environments the "sudo privileges required" message is
+            # always printed.
+            mock_print.assert_called_once_with(self.sudo_message)
+            mock_print.reset_mock()
+
+    def test_invalid_version(self, mock_print):
+        # A ProgramExit is raised if the Juju version is not valid.
+        with self.patch_get_juju_version(ValueError(b'bad wolf')):
+            with self.assert_program_exit('bad wolf'):
+                app.bootstrap_requires_sudo(True)
+
+
[e-mail]_print
 class TestBootstrap(
         helpers.CallTestsMixin, ProgramExitTestsMixin, unittest.TestCase):
 
@@ -459,10 +509,7 @@
         mock_call.assert_has_calls([
             mock.call('sudo', self.juju, 'bootstrap', '-e', self.env_name),
         ] + self.make_status_calls(1))
-        mock_print.assert_has_calls([
-            mock.call('sudo privileges required to bootstrap the environment'),
-            mock.call(self.status_message),
-        ])
+        mock_print.assert_called_once_with(self.status_message)
 
     def test_success_debug(self, mock_print):
         # The environment is successfully bootstrapped in debug mode.
 
=== modified file 'quickstart/tests/test_manage.py'
--- quickstart/tests/test_manage.py	2014-01-31 18:09:48 +0000
+++ quickstart/tests/test_manage.py	2014-02-19 13:55:43 +0000
@@ -1,6 +1,6 @@
 # This file is part of the Juju Quickstart Plugin, which lets users set up a
 # Juju environment in very few steps (https://launchpad.net/juju-quickstart).
-# Copyright (C) 2013 Canonical Ltd.
+# Copyright (C) 2013-2014 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it under
 # the terms of the GNU Affero General Public License version 3, as published by
@@ -669,12 +669,6 @@
         msg = 'admin-secret not found in {}'.format(path)
         raise ValueError(msg.encode('utf-8'))
 
-    @staticmethod
-    def patch_get_juju_version(major, minor, patch):
-        """Patch the quickstart.utils.get_juju_version function."""
-        mock_call = mock.Mock(return_value=(major, minor, patch))
-        return mock.patch('quickstart.utils.get_juju_version', mock_call)
-
     def test_no_bundle(self, mock_app, mock_open):
         # The application runs correctly if no bundle is provided.
         token = 'AUTHTOKEN'
@@ -683,12 +677,12 @@
         mock_app.bootstrap.return_value = (True, 'precise')
         mock_app.get_admin_secret = self.mock_get_admin_secret_error
         options = self.make_options()
-        with self.patch_get_juju_version(1, 17, 1):
-            manage.run(options)
+        manage.run(options)
         mock_app.ensure_dependencies.assert_called()
         mock_app.ensure_ssh_keys.assert_called()
         mock_app.bootstrap.assert_called_once_with(
-            options.env_name, requires_sudo=False, debug=options.debug)
+            options.env_name, requires_sudo=mock_app.bootstrap_requires_sudo(),
+            debug=options.debug)
         mock_app.get_api_url.assert_called_once_with(options.env_name)
         mock_app.connect.assert_has_calls([
             mock.call(mock_app.get_api_url(), options.admin_secret),
@@ -731,9 +725,9 @@
         # The application correctly handles working with local providers with
         # new Juju versions not requiring "sudo" to bootstrap the environment.
         options = self.make_options(env_type='local')
+        mock_app.bootstrap_requires_sudo.return_value = False
         mock_app.bootstrap.return_value = (True, 'precise')
-        with self.patch_get_juju_version(1, 18, 0):
-            manage.run(options)
+        manage.run(options)
         mock_app.bootstrap.assert_called_once_with(
             options.env_name, requires_sudo=False, debug=options.debug)
 
@@ -741,9 +735,9 @@
         # The application correctly handles working with local providers when
         # Juju requires an external "sudo" call to bootstrap the environment.
         options = self.make_options(env_type='local')
+        mock_app.bootstrap_requires_sudo.return_value = True
         mock_app.bootstrap.return_value = (True, 'precise')
-        with self.patch_get_juju_version(1, 17, 1):
-            manage.run(options)
+        manage.run(options)
         mock_app.bootstrap.assert_called_once_with(
             options.env_name, requires_sudo=True, debug=options.debug)
 
 
=== modified file 'quickstart/tests/test_utils.py'
--- quickstart/tests/test_utils.py	2014-01-31 18:09:48 +0000
+++ quickstart/tests/test_utils.py	2014-02-19 13:55:43 +0000
@@ -1,6 +1,6 @@
 # This file is part of the Juju Quickstart Plugin, which lets users set up a
 # Juju environment in very few steps (https://launchpad.net/juju-quickstart).
-# Copyright (C) 2013 Canonical Ltd.
+# Copyright (C) 2013-2014 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it under
 # the terms of the GNU Affero General Public License version 3, as published by
@@ -710,26 +710,3 @@
                 utils.get_juju_version()
         msg = 'invalid version string: 1.17'
         self.assertEqual(msg, bytes(context_manager.exception))
-
-
-class TestLocalBootstrapRequiresSudo(unittest.TestCase):
-
-    def test_returns_true_on_lower_than_1_17_2(self):
-        # If Juju version is lower than 1.17.2.
-        value = utils.local_bootstrap_requires_sudo(1, 17, 0)
-        self.assertTrue(value)
-        value = utils.local_bootstrap_requires_sudo(1, 16, 5)
-        self.assertTrue(value)
-
-    def test_returns_false_higher_than_1_17_1(self):
-        # If juju version is higher than 1.17.1.
-        value = utils.local_bootstrap_requires_sudo(1, 17, 2)
-        self.assertFalse(value)
-        value = utils.local_bootstrap_requires_sudo(1, 17, 10)
-        self.assertFalse(value)
-        value = utils.local_bootstrap_requires_sudo(1, 18, 0)
-        self.assertFalse(value)
-        value = utils.local_bootstrap_requires_sudo(1, 18, 2)
-        self.assertFalse(value)
-        value = utils.local_bootstrap_requires_sudo(2, 16, 1)
-        self.assertFalse(value)
 
=== modified file 'quickstart/utils.py'
--- quickstart/utils.py	2014-01-31 18:09:48 +0000
+++ quickstart/utils.py	2014-02-19 13:55:43 +0000
@@ -1,6 +1,6 @@
 # This file is part of the Juju Quickstart Plugin, which lets users set up a
 # Juju environment in very few steps (https://launchpad.net/juju-quickstart).
-# Copyright (C) 2013 Canonical Ltd.
+# Copyright (C) 2013-2014 Canonical Ltd.
 #
 # This program is free software: you can redistribute it and/or modify it under
 # the terms of the GNU Affero General Public License version 3, as published by
@@ -348,15 +348,6 @@
         raise ValueError(msg.encode('utf-8'))
 
 
-def local_bootstrap_requires_sudo(major, minor, patch):
-    """Parse Juju version to check if sudo is required for local deployment.
-
-    Before Juju version 1.17.2 sudo is required for bootstrapping
-    local environments.
-    """
-    return (major, minor, patch) < (1, 17, 2)
-
-
 def run_once(function):
     """Return a decorated version of the given function which runs only once.