launchpad-reviewers team mailing list archive

[Colin Watson <mp+456336@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:rework-archive-job-series-check into launchpad:master.

Commit message:
Rework CI build upload series check to use DistroSeriesParent

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/456336

In https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/448906, I had to resort to doing series name comparison because I thought that there was no proper database-level link between the build and target series in the cases where we're using this at present.  I've since realized that was wrong, and the two series are linked via the `DistroSeriesParent` table, so we can do this more neatly.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:rework-archive-job-series-check into launchpad:master.

diff --git a/lib/lp/soyuz/model/archivejob.py b/lib/lp/soyuz/model/archivejob.py
index 8c9ff5a..b3beedf 100644
--- a/lib/lp/soyuz/model/archivejob.py
+++ b/lib/lp/soyuz/model/archivejob.py
@@ -797,28 +797,28 @@ class CIBuildUploadJob(ArchiveJobDerived):
                 continue
             artifact = scanned_artifact.artifact
             library_file = artifact.library_file
-            # XXX cjwatson 2023-08-09: Comparing distroseries names here is
-            # a pretty unpleasant hack, but it's necessary because in
-            # practice what we're doing is taking the output of build jobs
-            # that were run on (e.g.) Ubuntu focal and uploading them to
-            # (e.g.) <private distribution> focal.  Unfortunately the
-            # private series doesn't have its parent series set to the
-            # corresponding Ubuntu series, so we have no way to make the
-            # connection other than comparing names.  We need to figure out
-            # something better here, but at least this hack isn't too deep
-            # in the core of the system.
-            if (
-                artifact.report.distro_arch_series is not None
-                and artifact.report.distro_arch_series.distroseries.name
-                != self.target_distroseries.name
-            ):
-                logger.info(
-                    "Skipping %s (built for %s, not %s)",
-                    library_file.filename,
-                    artifact.report.distro_arch_series.distroseries.name,
-                    self.target_distroseries.name,
+            if artifact.report.distro_arch_series is not None:
+                build_distroseries = (
+                    artifact.report.distro_arch_series.distroseries
                 )
-                continue
+                # It might seem that we ought to check that the build job's
+                # series is equal to the target series, but in practice what
+                # we're doing is taking the output of build jobs that were
+                # run on (e.g.) Ubuntu focal and uploading them to (e.g.)
+                # <private distribution> focal, so we need to check parent
+                # series links as well.
+                if (
+                    build_distroseries != self.target_distroseries
+                    and build_distroseries
+                    not in self.target_distroseries.getParentSeries()
+                ):
+                    logger.info(
+                        "Skipping %s (built for %s, not %s)",
+                        library_file.filename,
+                        build_distroseries.name,
+                        self.target_distroseries.name,
+                    )
+                    continue
             logger.info(
                 "Uploading %s to %s %s (%s)",
                 library_file.filename,
diff --git a/lib/lp/soyuz/tests/test_archivejob.py b/lib/lp/soyuz/tests/test_archivejob.py
index 4592b7a..3244e4d 100644
--- a/lib/lp/soyuz/tests/test_archivejob.py
+++ b/lib/lp/soyuz/tests/test_archivejob.py
@@ -1660,8 +1660,7 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
         # series are selected.
         #
         # The build distribution is always Ubuntu for now, but the target
-        # distribution may differ.  Unfortunately, this currently requires
-        # matching on series names.
+        # distribution may differ.
         logger = self.useFixture(FakeLogger())
         target_distribution = self.factory.makeDistribution()
         archive = self.factory.makeArchive(
@@ -1672,13 +1671,16 @@ class TestCIBuildUploadJob(TestCaseWithFactory):
             self.factory.makeUbuntuDistroSeries() for _ in range(2)
         ]
         target_distroserieses = [
-            self.factory.makeDistroSeries(
-                distribution=target_distribution,
-                name=ubuntu_distroseries.name,
-                version=ubuntu_distroseries.version,
-            )
+            self.factory.makeDistroSeries(distribution=target_distribution)
             for ubuntu_distroseries in ubuntu_distroserieses
         ]
+        for ubuntu_distroseries, target_distroseries in zip(
+            ubuntu_distroserieses, target_distroserieses
+        ):
+            self.factory.makeDistroSeriesParent(
+                parent_series=ubuntu_distroseries,
+                derived_series=target_distroseries,
+            )
         processor = self.factory.makeProcessor()
         ubuntu_dases = [
             self.factory.makeDistroArchSeries(