Merge ~xnox/launchpad:copy-signing-bool into launchpad:master

[Dimitri John Ledkov]

custom_uploads_copier.py: Support excluding signing

Add support in custom_uploads_copier to skip copying signing (uefi & signing).

Hook into initialise_distroseries with packagecopier to exclude copying signing. Not sure if this codepath is actually ever used.

Hook into publish_ftpmaster to exclude signing upon preparing to publish fresh series for the first time.

This patch by-itself doesn't expose similar functionality to the `copy-package` exported API, but it will help to implement that later on as well.

Note I only did checks with pre-commit, so unittests were not run, and I didn't do a deploy of development launchpad to test new series init to see if this does what I am intending for it to do.

[Dimitri John Ledkov (xnox)]

I was expecting for some archive publishing tests to fail, upon asserting that singing tarballs did not get copied up upon distro series init. But out of the sets of tests that I run, none failed. Maybe this is tested somewhere else, or hasn't been asserted before.

[Colin Watson (cjwatson)]

My biggest concern here is that the selection of custom upload types to exclude seems quite arbitrary just from looking at the code here, and it's not going to be obvious to future developers what overall properties of the system they need to maintain. At the moment I'm not even sure I understand the rationale well enough to be clear on whether this is the best approach.

Could you please add some commentary somewhere (in the actual code, not just in supporting material such as commit messages or merge proposal conversations) that explains the "why" of this change, as well as the "what"?

review: Needs Information

[Dimitri John Ledkov (xnox)]

I agree reading the diff it is not obvious, and it is very subtle as to what & why is being changed. In the code-paths that rarely anyone uses or changes. As this is literary only twice a year operation.

[Colin Watson (cjwatson)]

Yep, that's exactly why we need it to be clearly spelled out, if even I don't quite understand it :-)

Unmerged commits

ef88fc0... by Dimitri John Ledkov on 2023-01-20

custom_uploads_copier.py: Support excluding signing

Add support in custom_uploads_copier to skip copying signing (uefi &
signing).

Hook into initialise_distroseries with packagecopier to exclude
copying signing. Not sure if this codepath is actually ever used.

Hook into publish_ftpmaster to exclude signing upon preparing to
publish fresh series for the first time.

Signed-off-by: Dimitri John Ledkov <email address hidden>

	docs:0 (build)
	lint:0 (build)
	mypy:0 (build)

1 → 3 of 3 results

First • Previous • Next • Last

diff --git a/lib/lp/archivepublisher/scripts/publish_ftpmaster.py b/lib/lp/archivepublisher/scripts/publish_ftpmaster.py
index ab301a6..0714693 100644
--- a/lib/lp/archivepublisher/scripts/publish_ftpmaster.py
+++ b/lib/lp/archivepublisher/scripts/publish_ftpmaster.py
@@ -616,7 +616,9 @@ class PublishFTPMaster(LaunchpadCronScript):
                 have_fresh_series = True
                 if series.previous_series is not None:
                     copier = CustomUploadsCopier(
-                        series, PackagePublishingPocket.RELEASE
+                        series,
+                        PackagePublishingPocket.RELEASE,
+                        include_signing=False,
                     )
                     copier.copy(
                         series.previous_series, PackagePublishingPocket.RELEASE
diff --git a/lib/lp/soyuz/scripts/custom_uploads_copier.py b/lib/lp/soyuz/scripts/custom_uploads_copier.py
index 8eae9be..283e933 100644
--- a/lib/lp/soyuz/scripts/custom_uploads_copier.py
+++ b/lib/lp/soyuz/scripts/custom_uploads_copier.py
@@ -49,10 +49,14 @@ class CustomUploadsCopier:
         target_series,
         target_pocket=PackagePublishingPocket.RELEASE,
         target_archive=None,
+        include_signing=True,
     ):
         self.target_series = target_series
         self.target_pocket = target_pocket
         self.target_archive = target_archive
+        if not include_signing:
+            del self.copyable_types[PackageUploadCustomFormat.UEFI]
+            del self.copyable_types[PackageUploadCustomFormat.SIGNING]
 
     def isCopyable(self, upload):
         """Is `upload` the kind of `PackageUploadCustom` that we can copy?"""
diff --git a/lib/lp/soyuz/scripts/initialize_distroseries.py b/lib/lp/soyuz/scripts/initialize_distroseries.py
index 5ffe47d..ffb41cf 100644
--- a/lib/lp/soyuz/scripts/initialize_distroseries.py
+++ b/lib/lp/soyuz/scripts/initialize_distroseries.py
@@ -676,6 +676,7 @@ class InitializeDistroSeries:
                                     close_bugs=False,
                                     create_dsd_job=False,
                                     person=None,
+                                    include_signing=False,
                                 )
                             )
                         if self.rebuild:
diff --git a/lib/lp/soyuz/scripts/packagecopier.py b/lib/lp/soyuz/scripts/packagecopier.py
index 372c52f..dfd5f28 100644
--- a/lib/lp/soyuz/scripts/packagecopier.py
+++ b/lib/lp/soyuz/scripts/packagecopier.py
@@ -581,6 +581,7 @@ def do_copy(
     phased_update_percentage=None,
     move=False,
     logger=None,
+    include_signing=True,
 ):
     """Perform the complete copy of the given sources incrementally.
 
@@ -633,6 +634,8 @@ def do_copy(
     :param move: If True, delete the source publication after copying to the
         destination.
     :param logger: An optional logger.
+    :param include_signing: A boolean indicating whether or not to copy
+        signing custom upload types.
 
     :raise CannotCopy when one or more copies were not allowed. The error
         will contain the reason why each copy was denied.
@@ -741,6 +744,7 @@ def do_copy(
             phased_update_percentage=phased_update_percentage,
             move=move,
             logger=logger,
+            include_signing=include_signing,
         )
         if send_email and sub_copies:
             mailer = PackageUploadMailer.forAction(
@@ -790,6 +794,7 @@ def _do_direct_copy(
     phased_update_percentage=None,
     move=False,
     logger=None,
+    include_signing=True,
 ):
     """Copy publishing records to another location.
 
@@ -825,6 +830,8 @@ def _do_direct_copy(
     :param move: If True, delete the source publication after copying to the
         destination.
     :param logger: An optional logger.
+    :param include_signing: A boolean indicating whether or not to copy
+        signing custom upload types.
 
     :return: a list of `ISourcePackagePublishingHistory` and
         `BinaryPackagePublishingHistory` corresponding to the copied
@@ -903,7 +910,10 @@ def _do_direct_copy(
         # Custom uploads aren't modelled as publication history records, so
         # we have to send these through the upload queue.
         custom_copier = CustomUploadsCopier(
-            series, target_pocket=pocket, target_archive=archive
+            series,
+            target_pocket=pocket,
+            target_archive=archive,
+            include_signing=include_signing,
         )
         for custom in custom_files:
             if custom_copier.isCopyable(custom):
diff --git a/lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py b/lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py
index af1b325..de21254 100644
--- a/lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py
+++ b/lib/lp/soyuz/scripts/tests/test_custom_uploads_copier.py
@@ -71,6 +71,28 @@ class TestCustomUploadsCopierLite(TestCaseWithFactory, CommonTestHelpers):
             [upload.customformat for upload in copied_uploads],
         )
 
+    def test_isCopyable_exclude_signing_types(self):
+        # Same as above, but check that signing types are excluded,
+        # when copyer is requested to ignore them
+        uploads = [
+            FakeUpload(custom_type, None)
+            for custom_type in PackageUploadCustomFormat.items
+        ]
+        copier = CustomUploadsCopier(FakeDistroSeries(), include_signing=False)
+        copied_uploads = filter(copier.isCopyable, uploads)
+        self.assertFalse(
+            PackageUploadCustomFormat.UEFI
+            in CustomUploadsCopier.copyable_types
+        )
+        self.assertFalse(
+            PackageUploadCustomFormat.SIGNING
+            in CustomUploadsCopier.copyable_types
+        )
+        self.assertContentEqual(
+            CustomUploadsCopier.copyable_types,
+            [upload.customformat for upload in copied_uploads],
+        )
+
     def test_getKey_calls_correct_custom_upload_method(self):
         # getKey calls the getSeriesKey method on the correct custom upload.
         class FakeCustomUpload: