actions: Allow not printing error when an action fails

Reviewed-by: Joseph Nuthalapati <njoseph@thoughtworks.com>

plinth/actions.py

@@ -109,12 +109,12 @@ def run(action, options=None, input=None, async=False):
     return _run(action, options, input, async, False)
 
 
-def superuser_run(action, options=None, input=None, async=False):
+def superuser_run(action, options=None, input=None, async=False, log_error=True):
     """Safely run a specific action as root.
 
     See actions._run for more information.
     """
-    return _run(action, options, input, async, True)
+    return _run(action, options, input, async, True, log_error=log_error)
 
 
 def run_as_user(action, options=None, input=None, async=False,
@@ -127,7 +127,7 @@ def run_as_user(action, options=None, input=None, async=False,
 
 
 def _run(action, options=None, input=None, async=False, run_as_root=False,
-         become_user=None):
+         become_user=None, log_error=True):
     """Safely run a specific action as a normal user or root.
 
     Actions are pulled from the actions directory.
@@ -186,8 +186,9 @@ def _run(action, options=None, input=None, async=False, run_as_root=False,
         output, error = proc.communicate(input=input)
         output, error = output.decode(), error.decode()
         if proc.returncode != 0:
-            LOGGER.error('Error executing command - %s, %s, %s', cmd, output,
+            if log_error:
-                         error)
+                LOGGER.error('Error executing command - %s, %s, %s', cmd, output,
+                             error)
             raise ActionError(action, output, error)
 
         return output

plinth/package.py

@@ -92,9 +92,9 @@ class Transaction(object):
         process.stdin.close()
 
         stdout_thread = threading.Thread(target=self._read_stdout,
-                                         args=(process,))
+                                         args=(process, ))
         stderr_thread = threading.Thread(target=self._read_stderr,
-                                         args=(process,))
+                                         args=(process, ))
         stdout_thread.start()
         stderr_thread.start()
         stdout_thread.join()
@@ -123,11 +123,14 @@ class Transaction(object):
             return
 
         status_map = {
-            'pmstatus': _('installing'),
+            'pmstatus':
-            'dlstatus': _('downloading'),
+                _('installing'),
-            'media-change': _('media change'),
+            'dlstatus':
-            'pmconffile': _('configuration file: {file}').format(
+                _('downloading'),
-                file=parts[1]),
+            'media-change':
+                _('media change'),
+            'pmconffile':
+                _('configuration file: {file}').format(file=parts[1]),
         }
         self.status_string = status_map.get(parts[0], '')
         self.percentage = int(float(parts[2]))
@@ -136,7 +139,8 @@ class Transaction(object):
 def is_package_manager_busy():
     """Return whether a package manager is running."""
     try:
-        actions.superuser_run('packages', ['is-package-manager-busy'])
+        actions.superuser_run('packages', ['is-package-manager-busy'],
+                              log_error=False)
         return True
     except actions.ActionError:
         return False

plinth/tests/test_actions.py

@@ -19,10 +19,13 @@
 Test module for actions utilities that modify configuration.
 """
 
+import apt_pkg
+import os
 import shutil
 import tempfile
 import unittest
 
+from plinth.errors import ActionError
 from plinth.actions import superuser_run, run
 from plinth import cfg
 
@@ -40,6 +43,7 @@ class TestPrivileged(unittest.TestCase):
         cls.action_directory = tempfile.TemporaryDirectory()
         cfg.actions_dir = cls.action_directory.name
 
+        shutil.copy(os.path.join(os.path.dirname(__file__), '..', '..', 'actions', 'packages'), cfg.actions_dir)
         shutil.copy('/bin/echo', cfg.actions_dir)
         shutil.copy('/usr/bin/id', cfg.actions_dir)
 
@@ -143,3 +147,15 @@ class TestPrivileged(unittest.TestCase):
         output = run('echo', tuple(options.split()))
         output = output.rstrip('\n')
         self.assertEqual(options, output)
+
+    def test_error_handling_for_superuser(self):
+        """Test that errors are raised only when expected."""
+        with apt_pkg.SystemLock():
+            with self.assertRaises(ActionError):
+                superuser_run('packages', ['is-package-manager-busy'],
+                              log_error=True)
+            with self.assertRaises(ActionError):
+                superuser_run('packages', ['is-package-manager-busy'],
+                              log_error=False)
+            with self.assertRaises(ActionError):
+                superuser_run('packages', ['is-package-manager-busy'])