launchpad-reviewers team mailing list archive

[Colin Watson <mp+427776@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:fix-dominate-sources-with-channel into launchpad:master.

Commit message:
Fix domination of source publications with channels

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

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

We need to explicitly pass a `jsonb` operand to `IsDistinctFrom`, as otherwise it miscompiles to the nonsensical `channel IS DISTINCT FROM track IS DISTINCT FROM risk IS DISTINCT FROM branch` rather than `channel IS DISTINCT FROM CAST('[track, risk, branch]' AS jsonb)`.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:fix-dominate-sources-with-channel into launchpad:master.

diff --git a/lib/lp/archivepublisher/domination.py b/lib/lp/archivepublisher/domination.py
index 7ee9b5e..d705f81 100644
--- a/lib/lp/archivepublisher/domination.py
+++ b/lib/lp/archivepublisher/domination.py
@@ -50,6 +50,7 @@ it is performed for each suite using:
 
 __all__ = ["Dominator"]
 
+import json
 from collections import defaultdict
 from datetime import timedelta
 from functools import cmp_to_key
@@ -57,7 +58,7 @@ from itertools import filterfalse
 from operator import attrgetter, itemgetter
 
 import apt_pkg
-from storm.expr import And, Count, Desc, Not, Select
+from storm.expr import And, Cast, Count, Desc, Not, Select
 from zope.component import getUtility
 
 from lp.registry.model.sourcepackagename import SourcePackageName
@@ -594,7 +595,9 @@ class Dominator:
                 Not(
                     IsDistinctFrom(
                         BinaryPackagePublishingHistory._channel,
-                        pub_record._channel,
+                        Cast(json.dumps(pub_record._channel), "jsonb")
+                        if pub_record._channel is not None
+                        else None,
                     )
                 ),
             )
diff --git a/lib/lp/archivepublisher/tests/test_dominator.py b/lib/lp/archivepublisher/tests/test_dominator.py
index acff7b4..e689f3c 100644
--- a/lib/lp/archivepublisher/tests/test_dominator.py
+++ b/lib/lp/archivepublisher/tests/test_dominator.py
@@ -517,9 +517,76 @@ class TestDominator(TestNativePublishingBase):
         for pub in overrides_2:
             self.assertEqual(PackagePublishingStatus.PUBLISHED, pub.status)
 
-    def test_dominate_by_channel(self):
-        # Publications only dominate other publications in the same channel.
-        # (Currently only tested for binary publications.)
+    def test_dominate_sources_by_channel(self):
+        # Source publications only dominate other publications in the same
+        # channel.
+        with lp_dbuser():
+            archive = self.factory.makeArchive()
+            distroseries = self.factory.makeDistroSeries(
+                distribution=archive.distribution
+            )
+            das = self.factory.makeDistroArchSeries(distroseries=distroseries)
+            repository = self.factory.makeGitRepository(
+                target=self.factory.makeDistributionSourcePackage(
+                    distribution=archive.distribution
+                )
+            )
+            ci_builds = [
+                self.factory.makeCIBuild(
+                    git_repository=repository, distro_arch_series=das
+                )
+                for _ in range(3)
+            ]
+        spn = self.factory.makeSourcePackageName()
+        sprs = [
+            ci_build.createSourcePackageRelease(
+                distroseries=distroseries,
+                sourcepackagename=spn,
+                version=version,
+                creator=ci_build.git_repository.owner,
+                archive=archive,
+            )
+            for version, ci_build in zip(("1.0", "1.1", "1.2"), ci_builds)
+        ]
+        stable_spphs = [
+            self.factory.makeSourcePackagePublishingHistory(
+                distroseries=distroseries,
+                archive=archive,
+                sourcepackagerelease=spr,
+                pocket=PackagePublishingPocket.RELEASE,
+                status=PackagePublishingStatus.PUBLISHED,
+                channel="stable",
+            )
+            for spr in sprs[:2]
+        ]
+        candidate_spph = self.factory.makeSourcePackagePublishingHistory(
+            distroseries=distroseries,
+            archive=archive,
+            sourcepackagerelease=sprs[2],
+            pocket=PackagePublishingPocket.RELEASE,
+            status=PackagePublishingStatus.PUBLISHED,
+            channel="candidate",
+        )
+
+        dominator = Dominator(self.logger, archive)
+        dominator.judgeAndDominate(
+            distroseries, PackagePublishingPocket.RELEASE
+        )
+
+        # The older of the two stable publications is superseded, while the
+        # current stable publication and the candidate publication are left
+        # alone.
+        self.checkPublication(
+            stable_spphs[0], PackagePublishingStatus.SUPERSEDED
+        )
+        self.checkPublications(
+            (stable_spphs[1], candidate_spph),
+            PackagePublishingStatus.PUBLISHED,
+        )
+
+    def test_dominate_binaries_by_channel(self):
+        # Binary publications only dominate other publications in the same
+        # channel.
         with lp_dbuser():
             archive = self.factory.makeArchive()
             distroseries = self.factory.makeDistroSeries(