Merge lp:~smoser/charm-helpers/apt-install-noninteractive into lp:charm-helpers

[Scott Moser]

Changes to 'install' to ensure non-interactive mode

This makes 2 changes to invocations of 'apt-get install'
 a.) adds DEBIAN_FRONTEND=noninteractive to the environment
 b.) uses options --option=Dpkg::Options::=--force-confold

Also changed:
 * change '-y' to '--assume-yes' for readability.
 * changes to tests to accommodate

Changes to 'install' to ensure non-interactive mode

This makes 2 changes to invocations of 'apt-get install'
 a.) adds DEBIAN_FRONTEND=noninteractive to the environment
 b.) uses options --option=Dpkg::Options::=--force-confold

Also changed:
 * change '-y' to '--assume-yes' for readability.
 * changes to tests to accommodate

[Michael Nelson (michael.nelson)]

Excellent - thanks Scott. Out of interest, without the two extra options, in what situations were you seeing interactive mode?

Also, can we swap reviews? :-)

https://code.launchpad.net/~michael.nelson/charm-helpers/ansible-tags/+merge/194324

Most of the diff is moving a chunk of code to a more shareable location.

review: Approve

[James Page (james-page)]

review: Approve

[Scott Moser (smoser)]

The one thing that hit me was 'apt-get install maas' has a priority 'high' dialog that just says "You can see _SOMEURL_/MAAS/".

This kills that.

=== modified file 'charmhelpers/fetch/__init__.py'
--- charmhelpers/fetch/__init__.py	2013-11-06 03:25:32 +0000
+++ charmhelpers/fetch/__init__.py	2013-11-11 20:33:28 +0000
@@ -13,6 +13,7 @@
     log,
 )
 import apt_pkg
+import os
 
 CLOUD_ARCHIVE = """# Ubuntu Cloud Archive
 deb http://ubuntu-cloud.archive.canonical.com/ubuntu {} main
@@ -66,8 +67,10 @@
 
 def apt_install(packages, options=None, fatal=False):
     """Install one or more packages"""
-    options = options or []
-    cmd = ['apt-get', '-y']
+    if options is None:
+        options = ['--option=Dpkg::Options::=--force-confold']
+
+    cmd = ['apt-get', '--assume-yes']
     cmd.extend(options)
     cmd.append('install')
     if isinstance(packages, basestring):
@@ -76,10 +79,14 @@
         cmd.extend(packages)
     log("Installing {} with options: {}".format(packages,
                                                 options))
+    env = os.environ.copy()
+    if 'DEBIAN_FRONTEND' not in env:
+        env['DEBIAN_FRONTEND'] = 'noninteractive'
+
     if fatal:
-        subprocess.check_call(cmd)
+        subprocess.check_call(cmd, env=env)
     else:
-        subprocess.call(cmd)
+        subprocess.call(cmd, env=env)
 
 
 def apt_update(fatal=False):
@@ -93,7 +100,7 @@
 
 def apt_purge(packages, fatal=False):
     """Purge one or more packages"""
-    cmd = ['apt-get', '-y', 'purge']
+    cmd = ['apt-get', '--assume-yes', 'purge']
     if isinstance(packages, basestring):
         cmd.append(packages)
     else:

=== modified file 'tests/fetch/test_fetch.py'
--- tests/fetch/test_fetch.py	2013-11-06 03:30:37 +0000
+++ tests/fetch/test_fetch.py	2013-11-11 20:33:28 +0000
@@ -7,6 +7,7 @@
 )
 from urlparse import urlparse
 from charmhelpers import fetch
+import os
 import yaml
 
 FAKE_APT_CACHE = {
@@ -36,6 +37,15 @@
     return cache
 
 
+def getenv(update=None):
+    # return a copy of os.environ with update applied.
+    # this was necessary because some modules modify os.environment directly
+    copy = os.environ.copy()
+    if update is not None:
+        copy.update(update)
+    return copy
+
+
 class FetchTest(TestCase):
     @patch('apt_pkg.Cache')
     def test_filter_packages_missing(self, cache):
@@ -359,8 +369,10 @@
 
         fetch.apt_install(packages, options)
 
-        mock_call.assert_called_with(['apt-get', '-y', '--foo', '--bar',
-                                      'install', 'foo', 'bar'])
+        mock_call.assert_called_with(['apt-get', '--assume-yes',
+            '--foo', '--bar', 'install', 'foo', 'bar'],
+            env=getenv({'DEBIAN_FRONTEND': 'noninteractive'}))
+
 
     @patch('subprocess.call')
     @patch.object(fetch, 'log')
@@ -369,8 +381,12 @@
 
         fetch.apt_install(packages)
 
-        mock_call.assert_called_with(['apt-get', '-y', 'install', 'foo',
-                                      'bar'])
+        expected = ['apt-get', '--assume-yes',
+                    '--option=Dpkg::Options::=--force-confold',
+                    'install', 'foo', 'bar']
+
+        mock_call.assert_called_with(expected,
+            env=getenv({'DEBIAN_FRONTEND': 'noninteractive'}))
 
     @patch('subprocess.call')
     @patch.object(fetch, 'log')
@@ -380,8 +396,11 @@
 
         fetch.apt_install(packages, options)
 
-        mock_call.assert_called_with(['apt-get', '-y', '--foo', '--bar',
-                                      'install', 'foo bar'])
+        expected = ['apt-get', '--assume-yes',
+                    '--foo', '--bar', 'install', 'foo bar']
+
+        mock_call.assert_called_with(expected,
+            env=getenv({'DEBIAN_FRONTEND': 'noninteractive'}))
 
     @patch('subprocess.check_call')
     @patch.object(fetch, 'log')
@@ -391,8 +410,9 @@
 
         fetch.apt_install(packages, options, fatal=True)
 
-        check_call.assert_called_with(['apt-get', '-y', '--foo', '--bar',
-                                       'install', 'foo', 'bar'])
+        check_call.assert_called_with(['apt-get', '--assume-yes',
+            '--foo', '--bar', 'install', 'foo', 'bar'],
+            env=getenv({'DEBIAN_FRONTEND': 'noninteractive'}))
 
     @patch('subprocess.check_call')
     @patch.object(fetch, 'log')
@@ -420,7 +440,8 @@
         fetch.apt_purge(packages)
 
         log.assert_called()
-        mock_call.assert_called_with(['apt-get', '-y', 'purge', 'foo bar'])
+        mock_call.assert_called_with(['apt-get', '--assume-yes',
+                                      'purge', 'foo bar'])
 
     @patch('subprocess.call')
     @patch.object(fetch, 'log')
@@ -430,8 +451,8 @@
         fetch.apt_purge(packages)
 
         log.assert_called()
-        mock_call.assert_called_with(['apt-get', '-y', 'purge', 'foo',
-                                      'bar'])
+        mock_call.assert_called_with(['apt-get', '--assume-yes',
+                                      'purge', 'foo', 'bar'])
 
     @patch('subprocess.check_call')
     @patch.object(fetch, 'log')