Merge lp:~frankban/juju-quickstart/apt-update-fixes into lp:juju-quickstart

[Francesco Banconi]

Always retrieve bundle from new cs endpoint.

This is a follow up from previous branch,
and also includes the remaining bits before
2.0.1 release.

Even if a legacy bundle is specified, and even
if the legacy bundle data is retrieved for name
validation, always use the new bundle.yaml
charm store endpoint when returning the bundle
contents.

This branch also include a drive by fix to always
run `apt-get update` before installing packages,
even in the case --distro-only is enabled.

Tests: `make check`.

QA:
install bundles with quickstart:
`devenv/bin/juju-quickstart {bundle}`
Try the following bundles:
- devenv/bin/juju-quickstart mediawiki-single
- devenv/bin/juju-quickstart u/bigdata-dev/apache-analytics-sql
- devenv/bin/juju-quickstart bundle:mediawiki/scalable
- devenv/bin/juju-quickstart bundle:~landscape/landscape-dense-maas/landscape-dense-maas
- devenv/bin/juju-quickstart bundle:django/example-single

Those instead should return errors:
- devenv/bin/juju-quickstart mediawiki/trusty
- devenv/bin/juju-quickstart mediawiki-nosuch
- devenv/bin/juju-quickstart no-such
- devenv/bin/juju-quickstart bundle:no/such
- devenv/bin/juju-quickstart bundle:invalid
- devenv/bin/juju-quickstart bundle:~landscape/landscape-dense-maas/landscape

Thank you!

https://codereview.appspot.com/215750043/

[Francesco Banconi (frankban)]

Reviewers: mp+252430_code.launchpad.net,

Message:
Please take a look.

Description:
Always retrieve bundle from new cs endpoint.

This is a follow up from previous branch,
and also includes the remaining bits before
2.0.1 release.

Even if a legacy bundle is specified, and even
if the legacy bundle data is retrieved for name
validation, always use the new bundle.yaml
charm store endpoint when returning the bundle
contents.

This branch also include a drive by fix to always
run `apt-get update` before installing packages,
even in the case --distro-only is enabled.

Tests: `make check`.

QA:
install bundles with quickstart:
`devenv/bin/juju-quickstart {bundle}`
Try the following bundles:
- devenv/bin/juju-quickstart mediawiki-single
- devenv/bin/juju-quickstart u/bigdata-dev/apache-analytics-sql
- devenv/bin/juju-quickstart bundle:mediawiki/scalable
- devenv/bin/juju-quickstart
bundle:~landscape/landscape-dense-maas/landscape-dense-maas
- devenv/bin/juju-quickstart bundle:django/example-single

Those instead should return errors:
- devenv/bin/juju-quickstart mediawiki/trusty
- devenv/bin/juju-quickstart mediawiki-nosuch
- devenv/bin/juju-quickstart no-such
- devenv/bin/juju-quickstart bundle:no/such
- devenv/bin/juju-quickstart bundle:invalid
- devenv/bin/juju-quickstart
bundle:~landscape/landscape-dense-maas/landscape

Thank you!

https://code.launchpad.net/~frankban/juju-quickstart/apt-update-fixes/+merge/252430

(do not edit description out of merge proposal)

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

Affected files (+104, -23 lines):
   A [revision details]
   M quickstart/models/bundles.py
   M quickstart/platform_support.py
   M quickstart/tests/models/test_bundles.py
   M quickstart/tests/test_app.py
   M quickstart/tests/test_utils.py
   M quickstart/utils.py

[Brad Crittenden (bac)]

Code LGTM. No QA yet.

https://codereview.appspot.com/215750043/

[Richard Harding (rharding)]

LGTM no qa atm

https://codereview.appspot.com/215750043/

[Richard Harding (rharding)]

On 2015/03/10 13:55:46, rharding wrote:
> LGTM no qa atm

QA OK thanks!

https://codereview.appspot.com/215750043/

[Francesco Banconi (frankban)]

*** Submitted:

Always retrieve bundle from new cs endpoint.

This is a follow up from previous branch,
and also includes the remaining bits before
2.0.1 release.

Even if a legacy bundle is specified, and even
if the legacy bundle data is retrieved for name
validation, always use the new bundle.yaml
charm store endpoint when returning the bundle
contents.

This branch also include a drive by fix to always
run `apt-get update` before installing packages,
even in the case --distro-only is enabled.

Tests: `make check`.

QA:
install bundles with quickstart:
`devenv/bin/juju-quickstart {bundle}`
Try the following bundles:
- devenv/bin/juju-quickstart mediawiki-single
- devenv/bin/juju-quickstart u/bigdata-dev/apache-analytics-sql
- devenv/bin/juju-quickstart bundle:mediawiki/scalable
- devenv/bin/juju-quickstart
bundle:~landscape/landscape-dense-maas/landscape-dense-maas
- devenv/bin/juju-quickstart bundle:django/example-single

Those instead should return errors:
- devenv/bin/juju-quickstart mediawiki/trusty
- devenv/bin/juju-quickstart mediawiki-nosuch
- devenv/bin/juju-quickstart no-such
- devenv/bin/juju-quickstart bundle:no/such
- devenv/bin/juju-quickstart bundle:invalid
- devenv/bin/juju-quickstart
bundle:~landscape/landscape-dense-maas/landscape

Thank you!

R=bac, rharding
CC=
https://codereview.appspot.com/215750043

https://codereview.appspot.com/215750043/

[Francesco Banconi (frankban)]

Thanks for the quick reviews and QA Brad and Rick!

https://codereview.appspot.com/215750043/

=== modified file 'quickstart/models/bundles.py'
--- quickstart/models/bundles.py	2015-03-10 10:18:58 +0000
+++ quickstart/models/bundles.py	2015-03-10 13:22:58 +0000
@@ -142,7 +142,10 @@
     """
     if source.startswith('bundle:'):
         # The source refers to an old style bundle URL.
-        return _bundle_from_charmworld_url(source)
+        try:
+            return _bundle_from_charmworld_url(source)
+        except charmstore.NotFoundError as err:
+            raise IOError(bytes(err))
 
     has_extension = source.endswith('.yaml') or source.endswith('.json')
     is_remote = source.startswith('http://') or source.startswith('https://')
@@ -200,6 +203,7 @@
     content is not valid.
     Raise a IOError if a problem is encountered while fetching the YAML
     content from the charm store.
+    Raise a charmstore.NotFoundError if the bundle is not found.
     """
     match = _charmworld_url_expression.match(url)
     if match is None:
@@ -224,11 +228,10 @@
         # in the original YAML is also required.
         reference = references.Reference(
             'cs', user, 'bundle', basket, revision)
-        try:
-            data = charmstore.get_legacy_bundle_data(reference)
-        except charmstore.NotFoundError as err:
-            raise IOError(bytes(err))
-        data = _flatten_data(data, name)
+        # Validate the bundle name is included in the legacy data.
+        _flatten_data(charmstore.get_legacy_bundle_data(reference), name)
+        # Retrieve the new bundle data corresponding to the shorter reference.
+        data = charmstore.get_bundle_data(reference)
 
     validate(data)
     # XXX frankban 2015-02-26: remove this when switching to the new bundle

=== modified file 'quickstart/platform_support.py'
--- quickstart/platform_support.py	2015-01-30 15:27:07 +0000
+++ quickstart/platform_support.py	2015-03-10 13:22:58 +0000
@@ -83,10 +83,14 @@
     print('sudo privileges will be used for the installation of \n'
           'the following packages: {}\n'
           'this can take a while...'.format(', '.join(required_packages)))
+    if distro_only:
+        retcode, _, error = utils.call('sudo', '/usr/bin/apt-get', 'update')
+        if retcode:
+            raise OSError(error.encode('utf-8'))
     retcode, _, error = utils.call(
         'sudo', '/usr/bin/apt-get', 'install', '-y', *required_packages)
     if retcode:
-        raise OSError(bytes(error))
+        raise OSError(error.encode('utf-8'))
 
 
 def _installer_osx(distro_only, required_packages):
@@ -102,7 +106,7 @@
     retcode, _, error = utils.call(
         '/usr/local/bin/brew', 'install', *required_packages)
     if retcode:
-        raise OSError(bytes(error))
+        raise OSError(error.encode('utf-8'))
 
 
 INSTALLERS = {

=== modified file 'quickstart/tests/models/test_bundles.py'
--- quickstart/tests/models/test_bundles.py	2015-03-09 17:50:28 +0000
+++ quickstart/tests/models/test_bundles.py	2015-03-10 13:22:58 +0000
@@ -121,8 +121,10 @@
         side_effect = [
             # A first call urlread returns a not found error.
             netutils.NotFoundError('boo!'),
-            # A second call succeeds.
+            # A second call to retrieve the legacy data succeeds.
             self.legacy_bundle_content,
+            # The third call to retrieve the new data succeeds.
+            self.bundle_content,
         ]
         with self.patch_urlread(error=side_effect) as mock_urlread:
             bundle = bundles.from_source('bundle:mediawiki/bundle1')
@@ -132,10 +134,11 @@
         # XXX frankban 2015-03-09: remove this check once we get rid of the
         # charmworld id concept.
         self.assertEqual('mediawiki/bundle1', bundle.reference.charmworld_id)
-        # The urlread function has been called two times: the first time
+        # The urlread function has been called three times: the first time
         # including both the basket and the bundle name, the second time
-        # to retrieve the legacy bundle yaml, only including the basket.
-        self.assertEqual(mock_urlread.call_count, 2)
+        # to retrieve the legacy bundle YAML, the third time to retrieve the
+        # new bundle YAML (this time without including the basket name).
+        self.assertEqual(len(side_effect), mock_urlread.call_count)
         mock_urlread.assert_has_calls([
             mock.call(
                 settings.CHARMSTORE_API +
@@ -143,6 +146,9 @@
             mock.call(
                 settings.CHARMSTORE_API +
                 'bundle/mediawiki/archive/bundles.yaml.orig'),
+            mock.call(
+                settings.CHARMSTORE_API +
+                'bundle/mediawiki/archive/bundle.yaml'),
         ])
 
     def test_charmworld_bundle_deprecation_warning(self):
@@ -193,7 +199,7 @@
         # The urlread function has been called two times: the first time
         # including both the basket and the bundle name, the second time
         # to retrieve the legacy bundle yaml, only including the basket.
-        self.assertEqual(mock_urlread.call_count, 2)
+        self.assertEqual(2, mock_urlread.call_count)
         mock_urlread.assert_has_calls([
             mock.call(
                 settings.CHARMSTORE_API +
@@ -220,7 +226,7 @@
         # The urlread function has been called two times: the first time
         # including both the basket and the bundle name, the second time
         # to retrieve the legacy bundle yaml, only including the basket.
-        self.assertEqual(mock_urlread.call_count, 2)
+        self.assertEqual(len(side_effect), mock_urlread.call_count)
         mock_urlread.assert_has_calls([
             mock.call(
                 settings.CHARMSTORE_API +
@@ -230,6 +236,42 @@
                 'bundle/django/archive/bundles.yaml.orig'),
         ])
 
+    def test_charmworld_bundle_from_legacy_not_found_error(self):
+        # An IOError is raised if the legacy bundle cannot be found.
+        side_effect = [
+            # A first call urlread returns a not found error.
+            netutils.NotFoundError('boo!'),
+            # A second call to retrieve the legacy bundle data succeeds.
+            self.legacy_bundle_content,
+            # The third call to get the bundle from the new API endpoint fails.
+            # Note that this is unlikely to happen.
+            netutils.NotFoundError('what!'),
+        ]
+        expected_error = (
+            'charm store resource not found at '
+            '{}bundle/django/archive/bundle.yaml: '
+            'what!'.format(settings.CHARMSTORE_API))
+        with self.patch_urlread(error=side_effect) as mock_urlread:
+            with self.assertRaises(IOError) as ctx:
+                bundles.from_source('bundle:django/bundle1')
+        self.assertEqual(expected_error, bytes(ctx.exception))
+        # The urlread function has been called three times: the first time
+        # including both the basket and the bundle name, the second time
+        # to retrieve the legacy bundle YAML, the third time to retrieve the
+        # new bundle YAML (this time without including the basket name).
+        self.assertEqual(len(side_effect), mock_urlread.call_count)
+        mock_urlread.assert_has_calls([
+            mock.call(
+                settings.CHARMSTORE_API +
+                'bundle/django-bundle1/archive/bundle.yaml'),
+            mock.call(
+                settings.CHARMSTORE_API +
+                'bundle/django/archive/bundles.yaml.orig'),
+            mock.call(
+                settings.CHARMSTORE_API +
+                'bundle/django/archive/bundle.yaml'),
+        ])
+
     def test_jujucharms_bundle(self):
         # A bundle instance is properly returned from a jujucharms.com id.
         with self.patch_urlread(contents=self.bundle_content) as mock_urlread:

=== modified file 'quickstart/tests/test_app.py'
--- quickstart/tests/test_app.py	2015-03-09 17:50:28 +0000
+++ quickstart/tests/test_app.py	2015-03-10 13:22:58 +0000
@@ -98,9 +98,10 @@
         side_effects = (
             (127, '', 'no juju'),  # Retrieve the Juju version.
             (0, 'saucy', ''),  # Retrieve the Ubuntu release codename.
+            (0, 'update 1', ''),  # Update the repository with new sources.
             (0, 'install add repo', ''),  # Install add-apt-repository.
             (0, 'add repo', ''),  # Add the juju stable repository.
-            (0, 'update', ''),  # Update the repository with new sources.
+            (0, 'update 2', ''),  # Update the repository with new sources.
             (0, 'install', ''),  # Install missing packages.
             (0, '1.18.0', ''),  # Retrieve the version again.
         )
@@ -109,6 +110,7 @@
         mock_call.assert_has_calls([
             mock.call(self.juju_command, 'version'),
             mock.call('lsb_release', '-cs'),
+            mock.call('sudo', self.apt_get, 'update'),
             mock.call('sudo', self.apt_get, 'install', '-y',
                       'software-properties-common'),
             mock.call('sudo', self.add_repository, '-y', 'ppa:juju/stable'),
@@ -161,6 +163,7 @@
         # All the missing packages are installed from the distro repository.
         side_effects = (
             (127, '', 'no juju'),  # Retrieve the Juju version.
+            (0, 'update', ''),  # Update the repository with new sources.
             (0, 'install', ''),  # Install missing packages.
             (0, '1.17.42', ''),  # Retrieve the version again.
         )
@@ -169,6 +172,7 @@
         self.assertEqual(len(side_effects), mock_call.call_count)
         mock_call.assert_has_calls([
             mock.call(self.juju_command, 'version'),
+            mock.call('sudo', self.apt_get, 'update'),
             mock.call('sudo', self.apt_get, 'install', '-y',
                       'juju-core', 'juju-local'),
             mock.call(self.juju_command, 'version'),
@@ -199,9 +203,10 @@
             (0, '1.16.42', ''),  # Check the juju command.
             (127, '', 'no lxc'),  # Check the lxc-ls command.
             (0, 'saucy', ''),  # Retrieve the Ubuntu release codename.
+            (0, 'update 1', ''),  # Update the repository with new sources.
             (0, 'install add repo', ''),  # Install add-apt-repository.
             (0, 'add repo', ''),  # Add the juju stable repository.
-            (0, 'update', ''),  # Update the repository with new sources.
+            (0, 'update 2', ''),  # Update the repository with new sources.
             (0, 'install', ''),  # Install missing packages.
         )
         mock_call, juju_version = self.call_ensure_dependencies(side_effects)
@@ -210,6 +215,7 @@
             mock.call(self.juju_command, 'version'),
             mock.call('/usr/bin/lxc-ls'),
             mock.call('lsb_release', '-cs'),
+            mock.call('sudo', self.apt_get, 'update'),
             mock.call('sudo', self.apt_get, 'install', '-y',
                       'software-properties-common'),
             mock.call('sudo', self.add_repository, '-y', 'ppa:juju/stable'),
@@ -231,6 +237,7 @@
         side_effects = (
             (0, '1.16.42', ''),  # Check the juju command.
             (127, '', 'no lxc'),  # Check the lxc-ls command.
+            (0, 'update', ''),  # Update the repository with new sources.
             (0, 'install', ''),  # Install missing packages.
         )
         mock_call, juju_version = self.call_ensure_dependencies(
@@ -239,6 +246,7 @@
         mock_call.assert_has_calls([
             mock.call(self.juju_command, 'version'),
             mock.call('/usr/bin/lxc-ls'),
+            mock.call('sudo', self.apt_get, 'update'),
             mock.call('sudo', self.apt_get, 'install', '-y', 'juju-local'),
         ])
         mock_print.assert_called_once_with(
@@ -252,6 +260,7 @@
         side_effects = (
             (127, '', 'no juju'),  # Check the juju command.
             (0, 'saucy', ''),  # Retrieve the Ubuntu release codename.
+            (0, 'update', ''),  # Update the repository with new sources.
             (0, 'install add repo', ''),  # Install add-apt-repository.
             (1, '', 'add repo error'),  # Add the juju stable repository.
         )
@@ -264,15 +273,27 @@
         side_effects = (
             (127, '', 'no juju'),  # Check the juju command.
             (0, 'saucy', ''),  # Retrieve the Ubuntu release codename.
+            (0, 'update 1', ''),  # Update the repository with new sources.
             (0, 'install add repo', ''),  # Install add-apt-repository.
             (0, 'add repo', ''),  # Add the juju stable repository.
-            (0, 'update', ''),  # Update the repository with new sources.
+            (0, 'update 2', ''),  # Update the repository with new sources.
             (1, '', 'install error'),  # Install missing packages.
         )
         with self.assert_program_exit('install error'):
             mock_call = self.call_ensure_dependencies(side_effects)[0]
             self.assertEqual(3, mock_call.call_count)
 
+    def test_distro_only_update_failure_apt(self, mock_print):
+        # A ProgramExit is raised if updating APT sources fails.
+        side_effects = (
+            (127, '', 'no juju'),  # Retrieve the Juju version.
+            (1, '', 'update error'),  # Update the repository with new sources.
+        )
+        with self.assert_program_exit('update error'):
+            mock_call = self.call_ensure_dependencies(
+                side_effects, distro_only=True)[0]
+            self.assertEqual(2, mock_call.call_count)
+
     def test_failed_install_osx(self, mock_print):
         # A ProgramExit is raised if the packages installation fails.
         side_effects = (
@@ -292,9 +313,10 @@
         side_effects = (
             (127, '', 'no juju'),  # Check the juju command.
             (0, 'saucy', ''),  # Retrieve the Ubuntu release codename.
+            (0, 'update 1', ''),  # Update the repository with new sources.
             (0, 'install add repo', ''),  # Install add-apt-repository.
             (0, 'add repo', ''),  # Add the juju stable repository.
-            (0, 'update', ''),  # Update the repository with new sources.
+            (0, 'update 2', ''),  # Update the repository with new sources.
             (0, 'install', ''),  # Install missing packages.
             (127, '', 'no juju (again)'),  # Retrieve the Juju version.
         )

=== modified file 'quickstart/tests/test_utils.py'
--- quickstart/tests/test_utils.py	2015-02-09 12:34:33 +0000
+++ quickstart/tests/test_utils.py	2015-03-10 13:22:58 +0000
@@ -40,9 +40,10 @@
     apt_get = '/usr/bin/apt-get'
     repository = 'ppa:good/stuff'
     side_effects = (
+        (0, 'first update', ''),  # Update the global repository.
         (0, 'apt-get install', ''),  # Install add-apt-repository.
         (0, 'add-apt-repository', ''),  # Add the repository.
-        (0, 'update', ''),  # Update the global repository
+        (0, 'second update', ''),  # Update the global repository.
     )
 
     def patch_codename(self, codename):
@@ -59,6 +60,7 @@
         mock_get_ubuntu_codename.assert_called_once_with()
         self.assertEqual(len(self.side_effects), mock_call.call_count)
         mock_call.assert_has_calls([
+            mock.call('sudo', self.apt_get, 'update'),
             mock.call('sudo', self.apt_get, 'install', '-y',
                       'python-software-properties'),
             mock.call('sudo', self.apt_add_repository, '-y', self.repository),
@@ -73,6 +75,7 @@
         mock_get_ubuntu_codename.assert_called_once_with()
         self.assertEqual(len(self.side_effects), mock_call.call_count)
         mock_call.assert_has_calls([
+            mock.call('sudo', self.apt_get, 'update'),
             mock.call('sudo', self.apt_get, 'install', '-y',
                       'software-properties-common'),
             mock.call('sudo', self.apt_add_repository, '-y', self.repository),
@@ -92,15 +95,19 @@
 
     def test_command_error(self, mock_print):
         # An OSError is raised if a command error occurs.
-        side_effects = [(1, '', 'apt-get install error')]
+        side_effects = [(0, 'update', ''), (1, '', 'apt-get install error')]
         with self.patch_codename('quantal') as mock_get_ubuntu_codename:
             with self.patch_multiple_calls(side_effects) as mock_call:
                 with self.assertRaises(OSError) as context_manager:
                     utils.add_apt_repository(self.repository)
         mock_get_ubuntu_codename.assert_called_once_with()
-        mock_call.assert_called_once_with(
-            'sudo', self.apt_get, 'install', '-y',
-            'software-properties-common')
+        self.assertEqual(len(side_effects), mock_call.call_count)
+        mock_call.assert_has_calls([
+            mock.call('sudo', self.apt_get, 'update'),
+            mock.call(
+                'sudo', self.apt_get, 'install', '-y',
+                'software-properties-common'),
+        ])
         self.assertEqual(
             'apt-get install error', bytes(context_manager.exception))
 

=== modified file 'quickstart/utils.py'
--- quickstart/utils.py	2015-03-10 10:18:58 +0000
+++ quickstart/utils.py	2015-03-10 13:22:58 +0000
@@ -48,6 +48,7 @@
     if get_ubuntu_codename() == 'precise':
         add_repository_package = 'python-software-properties'
     commands = (
+        ('/usr/bin/apt-get', 'update'),
         ('/usr/bin/apt-get', 'install', '-y', add_repository_package),
         ('/usr/bin/add-apt-repository', '-y', repository),
         ('/usr/bin/apt-get', 'update'),