launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/show-failed-copies into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #812869 in Launchpad itself: "Failed PackageCopyJobs should show up on the PPA page somehow"
  https://bugs.launchpad.net/launchpad/+bug/812869

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/show-failed-copies/+merge/114589

== Summary ==

Bug 812869: PCJs can't be used to copy to PPAs, because the failure mode is uninformative.  See also bug 575450.

== Proposed fix ==

Raphaël Badin fixed most of this in r14665, by showing job notifications on Archive:+packages.  The unit tests for this manually construct failed jobs.  However, this doesn't quite work in practice because, as Julian points out: "This is in part due to the code in PCJ.run() that captures CannotCopy exceptions and swallows them (it assumes that all failures have a DistroSeriesDifference)."  So the final glue needed here just seems to be to turn copy failures into job failures in the case where we're copying to a PPA, much as there's already a separate error handling path in PlainPackageCopyJob.reportFailure.

While we're here, we might as well hook up the job runner facility to send an e-mail to the requester on failure.

== LOC Rationale ==

+38.  This is part of allowing us to remove synchronous copying from the PPA UI, and later to remove delayed copies, worth at least 1100 lines.

== Tests ==

bin/test -vvct test_packagecopyjob

== Demo and Q/A ==

Somewhat like https://code.launchpad.net/~cjwatson/launchpad/copy-packages-with-default-series/+merge/112557, but try copying something that fails, and make sure that an error message shows up on Archive:+packages after the PCJ has been processed.
-- 
https://code.launchpad.net/~cjwatson/launchpad/show-failed-copies/+merge/114589
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/show-failed-copies into lp:launchpad.

=== modified file 'lib/lp/soyuz/interfaces/packagecopyjob.py'
--- lib/lp/soyuz/interfaces/packagecopyjob.py	2012-06-19 23:49:20 +0000
+++ lib/lp/soyuz/interfaces/packagecopyjob.py	2012-07-12 09:12:22 +0000
@@ -228,3 +228,9 @@
     copy_policy = Choice(
         title=_("Applicable copy policy"),
         values=PackageCopyPolicy, required=True, readonly=True)
+
+    def getOperationDescription():
+        """Return a description of the copy operation."""
+
+    def getErrorRecipients():
+        """Return a list of email-ids to notify about copy errors."""

=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py	2012-06-29 08:40:05 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py	2012-07-12 09:12:22 +0000
@@ -55,6 +55,7 @@
     Job,
     )
 from lp.services.job.runner import BaseRunnableJob
+from lp.services.mail.sendmail import format_address_for_person
 from lp.soyuz.adapters.overrides import (
     FromExistingOverridePolicy,
     SourceOverride,
@@ -224,6 +225,14 @@
             ])
         return vars
 
+    def getOperationDescription(self):
+        """See `IPlainPackageCopyJob`."""
+        return "copying a package"
+
+    def getErrorRecipients(self):
+        """See `IPlainPackageCopyJob`."""
+        return [format_address_for_person(self.requester)]
+
     @property
     def copy_policy(self):
         """See `PlainPackageCopyJob`."""
@@ -243,6 +252,7 @@
     class_job_type = PackageCopyJobType.PLAIN
     classProvides(IPlainPackageCopyJobSource)
     config = config.IPlainPackageCopyJobSource
+    user_error_types = (CannotCopy,)
 
     @classmethod
     def _makeMetadata(cls, target_pocket, package_version,
@@ -494,6 +504,9 @@
         try:
             self.attemptCopy()
         except CannotCopy as e:
+            # Remember the target archive purpose, as otherwise aborting the
+            # transaction will forget it.
+            target_archive_purpose = self.target_archive.purpose
             logger = logging.getLogger()
             logger.info("Job:\n%s\nraised CannotCopy:\n%s" % (self, e))
             self.abort()  # Abort the txn.
@@ -503,9 +516,18 @@
             # else it will sit in ACCEPTED forever.
             self._rejectPackageUpload()
 
-            # Rely on the job runner to do the final commit.  Note that
-            # we're not raising any exceptions here, failure of a copy is
-            # not a failure of the job.
+            if target_archive_purpose == ArchivePurpose.PPA:
+                # If copying to a PPA, commit the failure and re-raise the
+                # exception.  We turn a copy failure into a job failure in
+                # order that it can show up in the UI.
+                transaction.commit()
+                raise
+            else:
+                # Otherwise, rely on the job runner to do the final commit,
+                # and do not consider a failure of a copy to be a failure of
+                # the job.  We will normally have a DistroSeriesDifference
+                # in this case.
+                pass
         except SuspendJobException:
             raise
         except:

=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py	2012-07-01 17:19:29 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py	2012-07-12 09:12:22 +0000
@@ -29,6 +29,7 @@
     block_on_job,
     pop_remote_notifications,
     )
+from lp.services.mail.sendmail import format_address_for_person
 from lp.services.webapp.testing import verifyObject
 from lp.soyuz.adapters.overrides import SourceOverride
 from lp.soyuz.enums import (
@@ -199,6 +200,12 @@
         job_source = getUtility(IPlainPackageCopyJobSource)
         self.assertTrue(verifyObject(IPlainPackageCopyJobSource, job_source))
 
+    def test_getErrorRecipients_requester(self):
+        # The job requester is the recipient.
+        job = self.makeJob()
+        email = format_address_for_person(job.requester)
+        self.assertEqual([email], job.getErrorRecipients())
+
     def test_create(self):
         # A PackageCopyJob can be created and stores its arguments.
         distroseries = self.factory.makeDistroSeries()
@@ -325,7 +332,9 @@
         # exception and posts a DistroSeriesDifferenceComment with the
         # failure message.
         dsd = self.factory.makeDistroSeriesDifference()
-        self.factory.makeArchive(distribution=dsd.derived_series.distribution)
+        self.factory.makeArchive(
+            distribution=dsd.derived_series.distribution,
+            purpose=ArchivePurpose.PRIMARY)
         job = self.makeJob(dsd)
         removeSecurityProxy(job).attemptCopy = FakeMethod(
             failure=CannotCopy("Server meltdown"))
@@ -357,7 +366,7 @@
         naked_job = removeSecurityProxy(job)
         naked_job.reportFailure = FakeMethod()
 
-        job.run()
+        self.assertRaises(CannotCopy, job.run)
 
         self.assertEqual(1, naked_job.reportFailure.call_count)
 
@@ -397,13 +406,13 @@
         naked_job = removeSecurityProxy(job)
         naked_job.reportFailure = FakeMethod()
 
-        job.run()
+        self.assertRaises(CannotCopy, job.run)
 
         self.assertEqual(1, naked_job.reportFailure.call_count)
 
     def test_target_ppa_message(self):
         # When copying to a PPA archive the error message is stored in the
-        # job's metadatas.
+        # job's metadata and the job fails.
         distroseries = self.factory.makeDistroSeries()
         package = self.factory.makeSourcePackageName()
         archive1 = self.factory.makeArchive(distroseries.distribution)
@@ -415,7 +424,8 @@
             include_binaries=False, package_version='1.0',
             requester=self.factory.makePerson())
         transaction.commit()
-        job.run()
+        self.runJob(job)
+        self.assertEqual(JobStatus.FAILED, job.status)
 
         self.assertEqual(
             "Destination pocket must be 'release' for a PPA.",
@@ -1033,7 +1043,7 @@
 
         # Now put the same named package in the target archive at the
         # oldest version in the changelog.
-        target_source_pub = self.publisher.getPubSource(
+        self.publisher.getPubSource(
             distroseries=self.distroseries, sourcename="libc",
             version="2.8-0", status=PackagePublishingStatus.PUBLISHED,
             archive=target_archive)