launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:livefs-request-build-consider-metadata into launchpad:master.

Commit message:
Consider metadata_override in LiveFS.requestBuild

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1864860 in Launchpad itself: "LiveFS.requestBuild disregards metadata_override for already-pending checks"
  https://bugs.launchpad.net/launchpad/+bug/1864860

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

While I'm here, also fix the comparison of unique_key; it was previously using a plain equality test, which in SQL returns NULL if one side or the other is NULL.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:livefs-request-build-consider-metadata into launchpad:master.

diff --git a/lib/lp/soyuz/model/livefs.py b/lib/lp/soyuz/model/livefs.py
index 0ec68b4..921d3b4 100644
--- a/lib/lp/soyuz/model/livefs.py
+++ b/lib/lp/soyuz/model/livefs.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -7,10 +7,13 @@ __all__ = [
     ]
 
 from datetime import timedelta
+import json
 import math
 
 from lazr.lifecycle.event import ObjectCreatedEvent
 import pytz
+import six
+from storm.expr import Cast
 from storm.locals import (
     Bool,
     DateTime,
@@ -55,6 +58,7 @@ from lp.services.database.interfaces import (
     )
 from lp.services.database.stormexpr import (
     Greatest,
+    IsDistinctFrom,
     NullsLast,
     )
 from lp.services.features import getFeatureFlag
@@ -183,14 +187,25 @@ class LiveFS(Storm, WebhookTargetMixin):
             # See rationale in `LiveFSBuildArchiveOwnerMismatch` docstring.
             raise LiveFSBuildArchiveOwnerMismatch()
 
-        pending = IStore(self).find(
-            LiveFSBuild,
+        clauses = [
             LiveFSBuild.livefs_id == self.id,
             LiveFSBuild.archive_id == archive.id,
             LiveFSBuild.distro_arch_series_id == distro_arch_series.id,
             LiveFSBuild.pocket == pocket,
-            LiveFSBuild.unique_key == unique_key,
-            LiveFSBuild.status == BuildStatus.NEEDSBUILD)
+            Not(IsDistinctFrom(LiveFSBuild.unique_key, unique_key)),
+            # Cast to jsonb in order to compare the JSON structures rather
+            # than their encoding, since the latter might differ in
+            # insignificant ways.
+            Not(IsDistinctFrom(
+                Cast(LiveFSBuild.metadata_override, "jsonb"),
+                Cast(
+                    None if metadata_override is None
+                    else six.ensure_text(
+                        json.dumps(metadata_override, ensure_ascii=False)),
+                    "jsonb"))),
+            LiveFSBuild.status == BuildStatus.NEEDSBUILD,
+            ]
+        pending = IStore(self).find(LiveFSBuild, *clauses)
         if pending.any() is not None:
             raise LiveFSBuildAlreadyPending
 
diff --git a/lib/lp/soyuz/tests/test_livefs.py b/lib/lp/soyuz/tests/test_livefs.py
index 315b96a..70e276d 100644
--- a/lib/lp/soyuz/tests/test_livefs.py
+++ b/lib/lp/soyuz/tests/test_livefs.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test live filesystems."""
@@ -246,6 +246,35 @@ class TestLiveFS(TestCaseWithFactory):
             self.factory.makeDistroArchSeries(
                 distroseries=livefs.distro_series),
             PackagePublishingPocket.RELEASE)
+        # We can specify a unique key.
+        livefs.requestBuild(
+            livefs.owner, livefs.distro_series.main_archive, distroarchseries,
+            PackagePublishingPocket.RELEASE, unique_key="foo")
+        livefs.requestBuild(
+            livefs.owner, livefs.distro_series.main_archive, distroarchseries,
+            PackagePublishingPocket.RELEASE, unique_key="bar")
+        self.assertRaises(
+            LiveFSBuildAlreadyPending, livefs.requestBuild,
+            livefs.owner, livefs.distro_series.main_archive, distroarchseries,
+            PackagePublishingPocket.RELEASE, unique_key="bar")
+        # We can apply different metadata overrides.
+        livefs.requestBuild(
+            livefs.owner, livefs.distro_series.main_archive, distroarchseries,
+            PackagePublishingPocket.RELEASE,
+            metadata_override={"proposed": True})
+        livefs.requestBuild(
+            livefs.owner, livefs.distro_series.main_archive, distroarchseries,
+            PackagePublishingPocket.RELEASE,
+            metadata_override={"project": "foo", "proposed": True})
+        livefs.requestBuild(
+            livefs.owner, livefs.distro_series.main_archive, distroarchseries,
+            PackagePublishingPocket.RELEASE,
+            metadata_override={"project": "foo"})
+        self.assertRaises(
+            LiveFSBuildAlreadyPending, livefs.requestBuild,
+            livefs.owner, livefs.distro_series.main_archive, distroarchseries,
+            PackagePublishingPocket.RELEASE,
+            metadata_override={"project": "foo", "proposed": True})
         # Changing the status of the old build allows a new build.
         old_build.updateStatus(BuildStatus.BUILDING)
         old_build.updateStatus(BuildStatus.FULLYBUILT)