launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/custom-same-archive into lp:launchpad.

Commit message:
Automatically approve copies of UEFI custom uploads within the same archive.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1068558 in Launchpad itself: "UEFI copies within same archive shouldn't require re-approval"
  https://bugs.launchpad.net/launchpad/+bug/1068558

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/custom-same-archive/+merge/130531

== Summary ==

Bug 1068558: UEFI copies within the same archive shouldn't require re-approval.  This is a minor inconvenience, but it would be nice to fix it.

== Proposed fix ==

If the source and target archives match, automatically approve the copy.

== Implementation details ==

I re-expressed some tests slightly to save LoC, mostly just by removing repetition of the form "foo_upload.packageupload" which tended to wrap a lot.

== Tests ==

bin/test -vvct test_custom_uploads_copier

== Demo and Q/A ==

Copy efilinux from quantal to quantal-proposed on dogfood.  It should not require manual approval.
-- 
https://code.launchpad.net/~cjwatson/launchpad/custom-same-archive/+merge/130531
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/custom-same-archive into lp:launchpad.

=== modified file 'lib/lp/soyuz/scripts/custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/custom_uploads_copier.py	2012-09-24 23:52:41 +0000
+++ lib/lp/soyuz/scripts/custom_uploads_copier.py	2012-10-19 11:36:26 +0000
@@ -51,11 +51,12 @@
         """Is `upload` the kind of `PackageUploadCustom` that we can copy?"""
         return upload.customformat in self.copyable_types
 
-    def autoApprove(self, custom):
-        """Can `custom` be automatically approved?"""
+    def autoApprove(self, custom, source_archive):
+        """Can `custom` be automatically approved from `source_archive`?"""
         # XXX cjwatson 2012-08-16: This more or less duplicates
         # BuildDaemonUploadPolicy.autoApprove/CustomUploadFile.autoApprove.
-        if custom.packageupload.archive.is_ppa:
+        if (custom.packageupload.archive.is_ppa or
+            custom.packageupload.archive == source_archive):
             return True
         # UEFI uploads are signed, and must therefore be approved by a human.
         if custom.customformat == PackageUploadCustomFormat.UEFI:
@@ -127,7 +128,7 @@
             changes_file_alias=original_upload.packageupload.changesfile)
         custom = package_upload.addCustom(
             original_upload.libraryfilealias, original_upload.customformat)
-        if self.autoApprove(custom):
+        if self.autoApprove(custom, original_upload.packageupload.archive):
             package_upload.setAccepted()
         else:
             package_upload.setUnapproved()

=== modified file 'lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py'
--- lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2012-09-24 23:22:23 +0000
+++ lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py	2012-10-19 11:36:26 +0000
@@ -334,15 +334,14 @@
             target_series, target_archive=target_series.main_archive)
         copied_upload = copier.copyUpload(original_upload)
         self.assertEqual([copied_upload], list_custom_uploads(target_series))
-        self.assertNotEqual(
-            original_upload.packageupload, copied_upload.packageupload)
+        original_pu = original_upload.packageupload
+        copied_pu = copied_upload.packageupload
+        self.assertNotEqual(original_pu, copied_pu)
         self.assertEqual(
             original_upload.customformat, copied_upload.customformat)
         self.assertEqual(
             original_upload.libraryfilealias, copied_upload.libraryfilealias)
-        self.assertEqual(
-            original_upload.packageupload.changesfile,
-            copied_upload.packageupload.changesfile)
+        self.assertEqual(original_pu.changesfile, copied_pu.changesfile)
 
     def test_copyUpload_copies_into_release_pocket(self):
         # copyUpload copies the original upload into the release pocket,
@@ -352,45 +351,50 @@
             pocket=PackagePublishingPocket.UPDATES)
         target_series = self.factory.makeDistroSeries()
         copier = CustomUploadsCopier(target_series)
-        copied_upload = copier.copyUpload(original_upload)
-        self.assertEqual(
-            PackagePublishingPocket.RELEASE,
-            copied_upload.packageupload.pocket)
+        copied_pu = copier.copyUpload(original_upload).packageupload
+        self.assertEqual(PackagePublishingPocket.RELEASE, copied_pu.pocket)
 
     def test_copyUpload_to_updates_pocket(self):
         # copyUpload copies an upload between pockets in the same series if
         # requested.
-        source_series = self.factory.makeDistroSeries(
-            status=SeriesStatus.CURRENT)
+        series = self.factory.makeDistroSeries(status=SeriesStatus.CURRENT)
         original_upload = self.makeUpload(
-            distroseries=source_series,
-            pocket=PackagePublishingPocket.PROPOSED)
+            distroseries=series, pocket=PackagePublishingPocket.PROPOSED)
         copier = CustomUploadsCopier(
-            source_series, target_pocket=PackagePublishingPocket.UPDATES)
-        copied_upload = copier.copyUpload(original_upload)
-        self.assertEqual(
-            PackagePublishingPocket.UPDATES,
-            copied_upload.packageupload.pocket)
+            series, target_pocket=PackagePublishingPocket.UPDATES)
+        copied_pu = copier.copyUpload(original_upload).packageupload
+        self.assertEqual(PackagePublishingPocket.UPDATES, copied_pu.pocket)
 
     def test_copyUpload_accepts_upload(self):
         # Uploads created by copyUpload are automatically accepted.
         original_upload = self.makeUpload()
         target_series = self.factory.makeDistroSeries()
         copier = CustomUploadsCopier(target_series)
-        copied_upload = copier.copyUpload(original_upload)
-        self.assertEqual(
-            PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
+        copied_pu = copier.copyUpload(original_upload).packageupload
+        self.assertEqual(PackageUploadStatus.ACCEPTED, copied_pu.status)
 
-    def test_copyUpload_unapproves_uefi(self):
+    def test_copyUpload_unapproves_uefi_from_different_archive(self):
         # Copies of UEFI custom uploads to a primary archive are set to
         # UNAPPROVED, since they will normally end up being signed.
+        target_series = self.factory.makeDistroSeries()
+        archive = self.factory.makeArchive(
+            distribution=target_series.distribution)
+        original_upload = self.makeUpload(
+            archive=archive, custom_type=PackageUploadCustomFormat.UEFI)
+        copier = CustomUploadsCopier(
+            target_series, target_archive=target_series.main_archive)
+        copied_pu = copier.copyUpload(original_upload).packageupload
+        self.assertEqual(PackageUploadStatus.UNAPPROVED, copied_pu.status)
+
+    def test_copyUpload_approves_uefi_from_same_archive(self):
+        # Copies of UEFI custom uploads within the same archive are
+        # automatically accepted, since they have already been signed.
         original_upload = self.makeUpload(
             custom_type=PackageUploadCustomFormat.UEFI)
         target_series = self.factory.makeDistroSeries()
         copier = CustomUploadsCopier(target_series)
-        copied_upload = copier.copyUpload(original_upload)
-        self.assertEqual(
-            PackageUploadStatus.UNAPPROVED, copied_upload.packageupload.status)
+        copied_pu = copier.copyUpload(original_upload).packageupload
+        self.assertEqual(PackageUploadStatus.ACCEPTED, copied_pu.status)
 
     def test_copyUpload_approves_uefi_to_ppa(self):
         # Copies of UEFI custom uploads to a PPA are automatically accepted,
@@ -404,23 +408,20 @@
             distribution=target_series.distribution)
         copier = CustomUploadsCopier(
             target_series, target_archive=target_archive)
-        copied_upload = copier.copyUpload(original_upload)
-        self.assertEqual(
-            PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
+        copied_pu = copier.copyUpload(original_upload).packageupload
+        self.assertEqual(PackageUploadStatus.ACCEPTED, copied_pu.status)
 
     def test_copyUpload_archive_None_copies_within_archive(self):
         # If CustomUploadsCopier was created with no target archive,
         # copyUpload copies an upload to the same archive as the original
         # upload.
         original_upload = self.makeUpload()
+        original_pu = original_upload.packageupload
         target_series = self.factory.makeDistroSeries()
         copier = CustomUploadsCopier(target_series)
-        copied_upload = copier.copyUpload(original_upload)
-        self.assertEqual(
-            PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
-        self.assertEqual(
-            original_upload.packageupload.archive,
-            copied_upload.packageupload.archive)
+        copied_pu = copier.copyUpload(original_upload).packageupload
+        self.assertEqual(PackageUploadStatus.ACCEPTED, copied_pu.status)
+        self.assertEqual(original_pu.archive, copied_pu.archive)
 
     def test_copyUpload_to_specified_archive(self):
         # If CustomUploadsCopier was created with a target archive,
@@ -429,10 +430,9 @@
         original_upload = self.makeUpload(distroseries=series)
         archive = self.factory.makeArchive(distribution=series.distribution)
         copier = CustomUploadsCopier(series, target_archive=archive)
-        copied_upload = copier.copyUpload(original_upload)
-        self.assertEqual(
-            PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
-        self.assertEqual(archive, copied_upload.packageupload.archive)
+        copied_pu = copier.copyUpload(original_upload).packageupload
+        self.assertEqual(PackageUploadStatus.ACCEPTED, copied_pu.status)
+        self.assertEqual(archive, copied_pu.archive)
 
     def test_copyUpload_from_ppa_to_main_archive(self):
         # copyUpload can copy uploads from a PPA to the main archive.
@@ -441,8 +441,6 @@
         original_upload = self.makeUpload(distroseries=series, archive=archive)
         copier = CustomUploadsCopier(
             series, target_archive=series.main_archive)
-        copied_upload = copier.copyUpload(original_upload)
-        self.assertEqual(
-            PackageUploadStatus.ACCEPTED, copied_upload.packageupload.status)
-        self.assertEqual(
-            series.main_archive, copied_upload.packageupload.archive)
+        copied_pu = copier.copyUpload(original_upload).packageupload
+        self.assertEqual(PackageUploadStatus.ACCEPTED, copied_pu.status)
+        self.assertEqual(series.main_archive, copied_pu.archive)