launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/job-user-error-types into lp:launchpad.

Commit message:
Switch to lazr.jobrunner 0.11 and define BaseJobRunner.userErrorTypes so that user errors are handled correctly.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1070804 in Launchpad itself: "Error notification by mails doesn't describe the error"
  https://bugs.launchpad.net/launchpad/+bug/1070804

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/job-user-error-types/+merge/131507

== Summary ==

''Summarise the problem that you're solving.''

Bug 1070804: The mails sent on PCJ copy failures are uninformative, and amount to OOPS notifications.  This turns out to be because they are in fact OOPS notifications due to a bug in lazr.jobrunner.

== Proposed fix ==

lazr.jobrunner 0.11 fixed most of this.  We need a further tweak in Launchpad to make use of this by removing the security proxy from job.user_error_types before trying to match exceptions against it.

== Tests ==

bin/test -vvct lp.services.job.tests.test_runner -t lp.soyuz.tests.test_packagecopyjob

== Demo and Q/A ==

Try a copy which raises CannotCopy for some reason, and check that it sends a useful mail rather than an "internal error" one.
-- 
https://code.launchpad.net/~cjwatson/launchpad/job-user-error-types/+merge/131507
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/job-user-error-types into lp:launchpad.

=== modified file 'lib/lp/services/job/runner.py'
--- lib/lp/services/job/runner.py	2012-07-26 14:31:45 +0000
+++ lib/lp/services/job/runner.py	2012-10-26 01:03:20 +0000
@@ -177,9 +177,8 @@
     def notifyOops(self, oops):
         """Report this oops."""
         ctrl = self.getOopsMailController(oops['id'])
-        if ctrl is None:
-            return
-        ctrl.send()
+        if ctrl is not None:
+            ctrl.send()
 
     def getOopsVars(self):
         """See `IRunnableJob`."""
@@ -188,9 +187,8 @@
     def notifyUserError(self, e):
         """See `IRunnableJob`."""
         ctrl = self.getUserErrorMailController(e)
-        if ctrl is None:
-            return
-        ctrl.send()
+        if ctrl is not None:
+            ctrl.send()
 
     def makeOopsReport(self, oops_config, info):
         """Generate an OOPS report using the given OOPS configuration."""
@@ -294,6 +292,9 @@
     def runJob(self, job, fallback):
         super(BaseJobRunner, self).runJob(IRunnableJob(job), fallback)
 
+    def userErrorTypes(self, job):
+        return removeSecurityProxy(job).user_error_types
+
     def retryErrorTypes(self, job):
         return removeSecurityProxy(job).retry_error_types
 

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-10-25 11:02:37 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-10-26 01:03:20 +0000
@@ -400,7 +400,7 @@
 
     def test_target_ppa_message(self):
         # When copying to a PPA archive the error message is stored in the
-        # job's metadata and the job fails.
+        # job's metadata and the job fails, but no OOPS is recorded.
         distroseries = self.factory.makeDistroSeries()
         package = self.factory.makeSourcePackageName()
         archive1 = self.factory.makeArchive(distroseries.distribution)
@@ -412,11 +412,14 @@
             include_binaries=False, package_version='1.0',
             requester=self.factory.makePerson())
         transaction.commit()
-        self.runJob(job)
+        switch_dbuser(self.dbuser)
+        runner = JobRunner([job])
+        runner.runAll()
         self.assertEqual(JobStatus.FAILED, job.status)
 
         self.assertEqual(
             "PPA uploads must be for the RELEASE pocket.", job.error_message)
+        self.assertEqual([], runner.oops_ids)
 
     def assertOopsRecorded(self, job):
         self.assertEqual(JobStatus.FAILED, job.status)
@@ -922,8 +925,7 @@
         self.assertEqual(JobStatus.SUSPENDED, job.status)
         if return_job:
             return job
-        pcj = removeSecurityProxy(job).context
-        return pcj
+        return removeSecurityProxy(job).context
 
     def test_copying_to_main_archive_debian_override_contrib(self):
         # The job uses the overrides to map debian components to
@@ -1440,9 +1442,7 @@
         switch_dbuser('copy_packages')
 
         override = SourceOverride(
-            source_package_name=name,
-            component=component,
-            section=section)
+            source_package_name=name, component=component, section=section)
         pcj.addSourceOverride(override)
 
         metadata_component = getUtility(
@@ -1499,9 +1499,7 @@
         switch_dbuser('copy_packages')
 
         override = SourceOverride(
-            source_package_name=name,
-            component=component,
-            section=section)
+            source_package_name=name, component=component, section=section)
         pcj.addSourceOverride(override)
 
         self.assertEqual(override, pcj.getSourceOverride())

=== modified file 'versions.cfg'
--- versions.cfg	2012-10-23 05:16:23 +0000
+++ versions.cfg	2012-10-26 01:03:20 +0000
@@ -47,7 +47,7 @@
 lazr.config = 1.1.3
 lazr.delegates = 1.2.0
 lazr.enum = 1.1.3
-lazr.jobrunner = 0.10
+lazr.jobrunner = 0.11
 lazr.lifecycle = 1.1
 lazr.restful = 0.19.9
 lazr.restfulclient = 0.13.1