launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/ugly-add-node-errors into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/ugly-add-node-errors/+merge/99271

We now have user-friendly Fault exceptions for provisioning errors.  But since Fault is not in the list of exception types that our exception middleware knows how to represent in a friendly manner, the UI will just show an ugly repr() of the exception.

One way of solving this would have been to extend the exceptions middleware to deal specially with Faults.  But then what of unexpected, unknown instances of Fault?  We might want the full information (fault code, traceback) for debugging.

So instead, I changed the code that represents Faults during provisioning in a user-friendly way.  It now turns them into MAASAPIExceptions, which get rendered as user-friendly text.

Note that unfortunately, I can now no longer use regex matching in ExpectedExceptions for these cases.  This is because the exception string may span multiple lines (the ugly repr() would turn newlines into even uglier “\n”) and even in multi-line mode, I can't get ExpectedException to accept that I matched the string.  I suppose it wants every line to match or something.  But luckily the same test case has more detailed text matching checks for each of these exceptions.  The two tests that lose string matching are there to test integration of exception catching with exception conversion, not so much to test the user-friendly exception representation per se.
-- 
https://code.launchpad.net/~jtv/maas/ugly-add-node-errors/+merge/99271
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/ugly-add-node-errors into lp:maas.

=== modified file 'src/maasserver/provisioning.py'
--- src/maasserver/provisioning.py	2012-03-23 08:19:11 +0000
+++ src/maasserver/provisioning.py	2012-03-26 09:08:36 +0000
@@ -26,6 +26,7 @@
     post_save,
     )
 from django.dispatch import receiver
+from maasserver.exceptions import MAASAPIException
 from maasserver.models import (
     Config,
     MACAddress,
@@ -85,7 +86,7 @@
         Otherwise, this returns None and the original exception should be
         re-raised.  (This is left to the caller in order to minimize
         erosion of the backtrace).
-    :rtype: :class:`Exception`, or None.
+    :rtype: :class:`MAASAPIException`, or None.
     """
     params = {
         'fault_code': fault.faultCode,
@@ -95,8 +96,8 @@
     if user_friendly_text is None:
         return None
     else:
-        user_friendly_text = dedent(user_friendly_text.lstrip('\n') % params)
-        return xmlrpclib.Fault(fault.faultCode, user_friendly_text)
+        return MAASAPIException(dedent(
+            user_friendly_text.lstrip('\n') % params))
 
 
 class ProvisioningCaller:

=== modified file 'src/maasserver/tests/test_provisioning.py'
--- src/maasserver/tests/test_provisioning.py	2012-03-26 05:07:12 +0000
+++ src/maasserver/tests/test_provisioning.py	2012-03-26 09:08:36 +0000
@@ -15,6 +15,7 @@
 from xmlrpclib import Fault
 
 from maasserver import provisioning
+from maasserver.exceptions import MAASAPIException
 from maasserver.models import (
     ARCHITECTURE,
     Config,
@@ -127,21 +128,18 @@
             raise Fault(PSERV_FAULT.NO_SUCH_PROFILE, "Unknown profile.")
 
         self.papi.patch('add_node', raise_missing_profile)
-        expectation = ExpectedException(
-            Fault, value_re='.*maas-import-isos.*')
-        with expectation:
+        with ExpectedException(MAASAPIException):
             node = factory.make_node(architecture='amd32k')
             provisioning.provision_post_save_Node(
                 sender=Node, instance=node, created=True)
 
     def test_provision_post_save_Node_returns_other_pserv_faults(self):
-        error_text = factory.getRandomString()
 
         def raise_fault(*args, **kwargs):
-            raise Fault(PSERV_FAULT.NO_COBBLER, error_text)
+            raise Fault(PSERV_FAULT.NO_COBBLER, factory.getRandomString())
 
         self.papi.patch('add_node', raise_fault)
-        with ExpectedException(Fault, ".*%s.*" % error_text):
+        with ExpectedException(MAASAPIException):
             node = factory.make_node(architecture='amd32k')
             provisioning.provision_post_save_Node(
                 sender=Node, instance=node, created=True)
@@ -243,7 +241,7 @@
 
         self.papi.patch('add_node', raise_fault)
 
-        with ExpectedException(Fault, ".*provisioning server.*"):
+        with ExpectedException(MAASAPIException, ".*provisioning server.*"):
             self.papi.add_node('node', 'profile', 'power', {})
 
     def test_provisioning_errors_are_reported_helpfully(self):
@@ -253,13 +251,13 @@
 
         self.papi.patch('add_node', raise_provisioning_error)
 
-        with ExpectedException(Fault, ".*Cobbler.*"):
+        with ExpectedException(MAASAPIException, ".*Cobbler.*"):
             self.papi.add_node('node', 'profile', 'power', {})
 
     def test_present_user_friendly_fault_describes_pserv_fault(self):
         self.assertIn(
             "provisioning server",
-            present_user_friendly_fault(Fault(8002, 'error')).faultString)
+            present_user_friendly_fault(Fault(8002, 'error')).message)
 
     def test_present_user_friendly_fault_covers_all_pserv_faults(self):
         all_pserv_faults = set(map_enum(PSERV_FAULT).values())
@@ -271,26 +269,26 @@
         for fault_code in map_enum(PSERV_FAULT).values():
             original_fault = Fault(fault_code, fault_string)
             new_fault = present_user_friendly_fault(original_fault)
-            self.assertNotEqual(fault_string, new_fault.faultString)
+            self.assertNotEqual(fault_string, new_fault.message)
 
     def test_present_user_friendly_fault_describes_cobbler_fault(self):
         friendly_fault = present_user_friendly_fault(
             Fault(PSERV_FAULT.NO_COBBLER, factory.getRandomString()))
-        friendly_text = friendly_fault.faultString
+        friendly_text = friendly_fault.message
         self.assertIn("unable to reach", friendly_text)
         self.assertIn("Cobbler", friendly_text)
 
     def test_present_user_friendly_fault_describes_cobbler_auth_fail(self):
         friendly_fault = present_user_friendly_fault(
             Fault(PSERV_FAULT.COBBLER_AUTH_FAILED, factory.getRandomString()))
-        friendly_text = friendly_fault.faultString
+        friendly_text = friendly_fault.message
         self.assertIn("failed to authenticate", friendly_text)
         self.assertIn("Cobbler", friendly_text)
 
     def test_present_user_friendly_fault_describes_cobbler_auth_error(self):
         friendly_fault = present_user_friendly_fault(
             Fault(PSERV_FAULT.COBBLER_AUTH_ERROR, factory.getRandomString()))
-        friendly_text = friendly_fault.faultString
+        friendly_text = friendly_fault.message
         self.assertIn("authentication token", friendly_text)
         self.assertIn("Cobbler", friendly_text)
 
@@ -300,7 +298,7 @@
             Fault(
                 PSERV_FAULT.NO_SUCH_PROFILE,
                 "invalid profile name: %s" % profile))
-        friendly_text = friendly_fault.faultString
+        friendly_text = friendly_fault.message
         self.assertIn(profile, friendly_text)
         self.assertIn("maas-import-isos", friendly_text)
 
@@ -308,7 +306,7 @@
         error_text = factory.getRandomString()
         friendly_fault = present_user_friendly_fault(
             Fault(PSERV_FAULT.GENERIC_COBBLER_ERROR, error_text))
-        friendly_text = friendly_fault.faultString
+        friendly_text = friendly_fault.message
         self.assertIn("Cobbler", friendly_text)
         self.assertIn(error_text, friendly_text)