refactored actions to use exceptions instead of return-values

actions.py

@@ -77,6 +77,7 @@ import pipes
 import subprocess
 
 import cfg
+from errors import ActionError
 
 
 def run(action, options=None, async=False):
@@ -106,6 +107,7 @@ def _run(action, options=None, async=False, run_as_root=False):
 
     # contract 3A and 3B: don't call anything outside of the actions directory.
     if os.sep in action:
+        # TODO: perhaps we should raise an ActionError instead of ValueError
         raise ValueError("Action can't contain:" + os.sep)
 
     cmd = cfg.actions_dir + os.sep + action
@@ -139,4 +141,6 @@ def _run(action, options=None, async=False, run_as_root=False):
 
     if not async:
         output, error = proc.communicate()
-        return output, error
+        if proc.returncode != 0:
+            raise ActionError('Running action %s failed: %s' % (action, error))
+        return output

errors.py

@@ -0,0 +1,10 @@
+""" Plinth project specific errors """
+
+
+class PlinthError(Exception):
+    pass
+
+
+class ActionError(PlinthError):
+    """ Use this error for exceptions when executing an action """
+    pass

modules/config/config.py

@@ -32,6 +32,7 @@ import socket
 import actions
 import cfg
 import util
+from errors import ActionError
 
 
 LOGGER = logging.getLogger(__name__)
@@ -133,20 +134,22 @@ def get_status():
 def _apply_changes(request, old_status, new_status):
     """Apply the form changes"""
     if old_status['hostname'] != new_status['hostname']:
-        if not set_hostname(new_status['hostname']):
+        try:
-            messages.error(request, _('Setting hostname failed'))
+            set_hostname(new_status['hostname'])
+        except (ActionError, ValueError) as err:
+            messages.error(request, _('Error setting hostname: %s') %
+                           err.message)
         else:
             messages.success(request, _('Hostname set'))
     else:
         messages.info(request, _('Hostname is unchanged'))
 
     if old_status['time_zone'] != new_status['time_zone']:
-        output, error = actions.superuser_run('timezone-change',
+        try:
-                                              [new_status['time_zone']])
+            actions.superuser_run('timezone-change', [new_status['time_zone']])
-        del output  # Unused
+        except (ActionError, ValueError) as err:
-        if error:
+            messages.error(request, _('Error setting time zone: %s') %
-            messages.error(request,
+                           err.message)
-                           _('Error setting time zone - %s') % error)
         else:
             messages.success(request, _('Time zone set'))
     else:
@@ -164,7 +167,5 @@ def set_hostname(hostname):
         actions.superuser_run('xmpp-pre-hostname-change')
         actions.superuser_run('hostname-change', hostname)
         actions.superuser_run('xmpp-hostname-change', hostname, async=True)
-    except OSError:
+    except OSError as err:
-        return False
+        raise ActionError(err.message)
-
-    return True

modules/diagnostics/diagnostics.py

@@ -25,6 +25,7 @@ from gettext import gettext as _
 
 import actions
 import cfg
+from errors import ActionError
 
 
 def init():
@@ -43,7 +44,12 @@ def index(request):
 @login_required
 def test(request):
     """Run diagnostics and the output page"""
-    output, error = actions.superuser_run("diagnostic-test")
+    output = ""
+    error = ""
+    try:
+        output = actions.superuser_run("diagnostic-test")
+    except ActionError as err:
+        error = err.message
     return TemplateResponse(request, 'diagnostics_test.html',
                             {'title': _('Diagnostic Test'),
                              'diagnostics_output': output,

modules/firewall/firewall.py

@@ -140,16 +140,11 @@ def on_service_enabled(sender, service_id, enabled, **kwargs):
 def _run(arguments, superuser=False):
     """Run an given command and raise exception if there was an error"""
     command = 'firewall'
-
     LOGGER.info('Running command - %s, %s, %s', command, arguments, superuser)
 
     if superuser:
-        output, error = actions.superuser_run(command, arguments)
+        output = actions.superuser_run(command, arguments)
     else:
-        output, error = actions.run(command, arguments)
+        output = actions.run(command, arguments)
-
-    if error:
-        raise Exception('Error setting/getting firewalld confguration - %s'
-                        % error)
 
     return output

modules/owncloud/owncloud.py

@@ -53,10 +53,7 @@ def index(request):
 
 def get_status():
     """Return the current status"""
-    output, error = actions.run('owncloud-setup', 'status')
+    output = actions.run('owncloud-setup', 'status')
-    if error:
-        raise Exception('Error getting ownCloud status: %s' % error)
-
     return {'enabled': 'enable' in output.split()}
 
 

modules/packages/packages.py

@@ -6,23 +6,18 @@ from gettext import gettext as _
 
 import actions
 import cfg
+from errors import ActionError
 
 
 def get_modules_available():
     """Return list of all modules"""
-    output, error = actions.run('module-manager', ['list-available'])
+    output = actions.run('module-manager', ['list-available'])
-    if error:
-        raise Exception('Error getting modules: %s' % error)
-
     return output.split()
 
 
 def get_modules_enabled():
     """Return list of all modules"""
-    output, error = actions.run('module-manager', ['list-enabled'])
+    output = actions.run('module-manager', ['list-enabled'])
-    if error:
-        raise Exception('Error getting enabled modules - %s' % error)
-
     return output.split()
 
 
@@ -88,13 +83,12 @@ def _apply_changes(request, old_status, new_status):
 
         module = field.split('_enabled')[0]
         if enabled:
-            output, error = actions.superuser_run(
+            try:
-                'module-manager', ['enable', cfg.python_root, module])
+                actions.superuser_run(
-            del output  # Unused
+                    'module-manager', ['enable', cfg.python_root, module])
-
+            except ActionError:
-            # TODO: need to get plinth to load the module we just
+                # TODO: need to get plinth to load the module we just
-            # enabled
+                # enabled
-            if error:
                 messages.error(
                     request, _('Error enabling module - {module}').format(
                         module=module))
@@ -103,13 +97,12 @@ def _apply_changes(request, old_status, new_status):
                     request, _('Module enabled - {module}').format(
                         module=module))
         else:
-            output, error = actions.superuser_run(
+            try:
-                'module-manager', ['disable', cfg.python_root, module])
+                actions.superuser_run(
-            del output  # Unused
+                    'module-manager', ['disable', cfg.python_root, module])
-
+            except ActionError:
-            # TODO: need a smoother way for plinth to unload the
+                # TODO: need a smoother way for plinth to unload the
-            # module
+                # module
-            if error:
                 messages.error(
                     request, _('Error disabling module - {module}').format(
                         module=module))

modules/pagekite/pagekite.py

@@ -196,16 +196,10 @@ def _apply_changes(request, old_status, new_status):
 def _run(arguments, superuser=True):
     """Run an given command and raise exception if there was an error"""
     command = 'pagekite-configure'
-
     LOGGER.info('Running command - %s, %s, %s', command, arguments, superuser)
 
     if superuser:
-        output, error = actions.superuser_run(command, arguments)
+        output = actions.superuser_run(command, arguments)
     else:
-        output, error = actions.run(command, arguments)
+        output = actions.run(command, arguments)
-
-    if error:
-        raise Exception('Error setting/getting PageKite confguration - %s'
-                        % error)
-
     return output

modules/tor/tor.py

@@ -36,8 +36,7 @@ def init():
 @login_required
 def index(request):
     """Service the index page"""
-    output, error = actions.superuser_run("tor-get-ports")
+    output = actions.superuser_run("tor-get-ports")
-    del error  # Unused
 
     port_info = output.split("\n")
     tor_ports = {}

modules/xmpp/xmpp.py

@@ -88,10 +88,7 @@ def configure(request):
 
 def get_status():
     """Return the current status"""
-    output, error = actions.run('xmpp-setup', 'status')
+    output = actions.run('xmpp-setup', 'status')
-    if error:
-        raise Exception('Error getting status: %s' % error)
-
     return {'inband_enabled': 'inband_enable' in output.split()}
 
 
@@ -111,11 +108,7 @@ def _apply_changes(request, old_status, new_status):
         option = 'noinband_enable'
 
     LOGGER.info('Option - %s', option)
-
+    actions.superuser_run('xmpp-setup', [option])
-    _output, error = actions.superuser_run('xmpp-setup', [option])
-    del _output  # Unused
-    if error:
-        raise Exception('Error running command - %s' % error)
 
 
 class RegisterForm(forms.Form):  # pylint: disable-msg=W0232
@@ -151,10 +144,8 @@ def register(request):
 
 def _register_user(request, data):
     """Register a new XMPP user"""
-    output, error = actions.superuser_run(
+    output = actions.superuser_run(
         'xmpp-register', [data['username'], data['password']])
-    if error:
-        raise Exception('Error registering user - %s' % error)
 
     if 'successfully registered' in output:
         messages.success(request, _('Registered account for %s') %

tests/actions_test.py

@@ -90,7 +90,7 @@ class TestPrivileged(unittest.TestCase):
         # counting is safer than actual badness.
         options = "good; echo $((1+1))"
 
-        output, error = run(action, options)
+        output = run(action, options)
 
         self.assertFalse("2" in output)
 
@@ -105,7 +105,7 @@ class TestPrivileged(unittest.TestCase):
         # counting is safer than actual badness.
         options = ["good", ";", "echo $((1+1))"]
 
-        output, error = run(action, options)
+        output = run(action, options)
 
         # we'd better not evaluate the data.
         self.assertFalse("2" in output)

views.py

@@ -48,4 +48,4 @@ def index(request):
     if request.user.is_authenticated():
         return HttpResponseRedirect(reverse('apps:index'))
 
-    return HttpResponseRedirect(reverse('apps:about'))
+    return HttpResponseRedirect(reverse('help:about'))