Merge lp:~gandelman-a/charm-helpers/filter_req_packages into lp:charm-helpers

[Adam Gandelman]

We run into instances where we've deployed a charm and a week later trigger some relation event that calls 'apt-get install' In that weeks time, a newer package has been published to the Ubuntu archive, causing apt-get to block on local config changes. To help keep these types of hooks idempotent, it would be helpful to only attempt installation of packages that require it, eg:

CORE_PACKAGES = ['mysql', 'apache2']
apt_update()
apt_install(filter_required_packages(CORE_PACKAGES))

Thought about adding a call to filter_required_packages() to apt_install(), but decided against it--with the correct options passed, apt_install() should be able to be used to upgrade packages as well as install

[Matthew Wedgwood (mew)]

I think this is a good addition, but I have some (perhaps nitpicky) problems with it as-is:

1. The function name is a little opaque. "Required" is a little vague. Also, you're actually filtering the packages that aren't required. Perhaps filter_installed_packages would be ok?

2. The function assumes (by throwing an exception) that the package cache is up-to-date. It makes sense to me simply to filter the packages that are already installed, returning everything else. That way you could run 'apt-get update' later if an un-installed packaged required a new repo. This should still fit your use case, as an exception would be thrown by apt_install when the package wasn't found.

review: Needs Fixing

[Adam Gandelman (gandelman-a)]

Matt- Agree with both points. I had proposed this at one point but we never actually started using it, so there I'm open to modifying the exception handling here. Thanks for the feedback.

[lp:~gandelman-a/charm-helpers/filter_req_packages]

23. By Adam Gandelman on 2013-06-11

Do not raise if package unavailable.

24. By Adam Gandelman on 2013-06-11

Rename filter_required_packages -> filter_installed_packages.

[Adam Gandelman (gandelman-a)]

review: Needs Resubmitting

[James Page (james-page)]

LGTM

review: Approve

[Matthew Wedgwood (mew)]

Awesome, +1

review: Approve

=== modified file 'charmhelpers/core/host.py'
--- charmhelpers/core/host.py	2013-06-11 14:09:19 +0000
+++ charmhelpers/core/host.py	2013-06-11 18:13:28 +0000
@@ -5,6 +5,7 @@
 #  Nick Moffitt <[e-mail]>
 #  Matthew Wedgwood <[e-mail]>
 
+import apt_pkg
 import os
 import pwd
 import grp
@@ -132,6 +133,22 @@
                    **kwargs)
 
 
+def filter_installed_packages(packages):
+    """Returns a list of packages that require installation"""
+    apt_pkg.init()
+    cache = apt_pkg.Cache()
+    _pkgs = []
+    for package in packages:
+        try:
+            p = cache[package]
+            p.current_ver or _pkgs.append(package)
+        except KeyError:
+            log('Package {} has no installation candidate.'.format(package),
+                level='WARNING')
+            _pkgs.append(package)
+    return _pkgs
+
+
 def apt_install(packages, options=None, fatal=False):
     """Install one or more packages"""
     options = options or []
@@ -150,6 +167,15 @@
         subprocess.call(cmd)
 
 
+def apt_update(fatal=False):
+    """Update local apt cache"""
+    cmd = ['apt-get', 'update']
+    if fatal:
+        subprocess.check_call(cmd)
+    else:
+        subprocess.call(cmd)
+
+
 def mount(device, mountpoint, options=None, persist=False):
     '''Mount a filesystem'''
     cmd_args = ['mount']

=== modified file 'tests/core/test_host.py'
--- tests/core/test_host.py	2013-06-10 15:59:37 +0000
+++ tests/core/test_host.py	2013-06-11 18:13:28 +0000
@@ -17,6 +17,30 @@
                """rw,nosuid,noexec,relatime,gid=5,mode=620,ptmxmode=000 0 0
 """).strip().split('\n')
 
+FAKE_APT_CACHE = {
+    # an installed package
+    'vim': {
+        'current_ver': '2:7.3.547-6ubuntu5'
+    },
+    # a uninstalled installation candidate
+    'emacs': {
+    }
+}
+
+def fake_apt_cache():
+    def _get(package):
+        pkg = MagicMock()
+        if package not in FAKE_APT_CACHE:
+            raise KeyError
+        pkg.name = package
+        if 'current_ver' in FAKE_APT_CACHE[package]:
+            pkg.current_ver = FAKE_APT_CACHE[package]['current_ver']
+        else:
+            pkg.current_ver = None
+        return pkg
+    cache = MagicMock()
+    cache.__getitem__.side_effect = _get
+    return cache
 
 @contextmanager
 def patch_open():
@@ -485,6 +509,37 @@
         check_call.assert_called_with(['apt-get', '-y', '--foo', '--bar',
                                        'install', 'foo', 'bar'])
 
+    @patch('subprocess.check_call')
+    def test_apt_update_fatal(self, check_call):
+        host.apt_update(fatal=True)
+        check_call.assert_called_with(['apt-get', 'update'])
+
+    @patch('subprocess.call')
+    def test_apt_update_nonfatal(self, call):
+        host.apt_update()
+        call.assert_called_with(['apt-get', 'update'])
+
+    @patch('apt_pkg.Cache')
+    def test_filter_packages_missing(self, cache):
+        cache.side_effect = fake_apt_cache
+        result = host.filter_installed_packages(['vim', 'emacs'])
+        self.assertEquals(result, ['emacs'])
+
+    @patch('apt_pkg.Cache')
+    def test_filter_packages_none_missing(self, cache):
+        cache.side_effect = fake_apt_cache
+        result = host.filter_installed_packages(['vim'])
+        self.assertEquals(result, [])
+
+    @patch.object(host, 'log')
+    @patch('apt_pkg.Cache')
+    def test_filter_packages_not_available(self, cache, log):
+        cache.side_effect = fake_apt_cache
+        result = host.filter_installed_packages(['vim', 'joe'])
+        self.assertEquals(result, ['joe'])
+        log.assert_called_with('Package joe has no installation candidate.',
+                               level='WARNING')
+
     @patch('subprocess.check_output')
     @patch.object(host, 'log')
     def test_mounts_a_device(self, log, check_output):