Merge ~ines-almeida/launchpad:webhook-patterns/update-models into launchpad:master

[Ines Almeida]

Update Webhook and CIBuild models with the new fields for webhook pattern filtering

`git_ref_pattern` field and `check_webhook_git_ref_pattern` function added to Webhook model to enable filtering webhook triggers from git repository according to their git refs
`git_refs` added to CIBuild model will be used to store the git refs that originate the builds

This is the preparation work needed to filter git repo webhooks: https://warthogs.atlassian.net/browse/LP-1151

This MP adds 3 main things:
 - `git_ref_pattern` field to Webhook model - to store the pattern used to filter webhooks
 - `checkGitRefPattern` endpoint to Webhook model - to check if a git ref matches the ref_pattern of a given webhook
 - `git_refs` field to CIBuild model - to store the git refs that originate a given build, to later be used to check against the webhooks ref_pattern

The final functionality to actually filter webhooks according to the `ref_pattern` will be added in another MP.

[Ines Almeida (ines-almeida)]

Will update `ref_pattern` to `git_ref_pattern` as I also just changed it in the DB MP. I will wait for the approval of that MP before making that change.

This MP can still be reviewed with that in mind.

https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/446950

[Ines Almeida (ines-almeida)]

> Will update `ref_pattern` to `git_ref_pattern` as I also just changed it in
> the DB MP. I will wait for the approval of that MP before making that change.

Done

[Colin Watson (cjwatson)]

review: Approve

[Ines Almeida (ines-almeida)]

Addressed all comments and cleaned up commit tree. Will merge after checks pass

diff --git a/lib/lp/code/interfaces/cibuild.py b/lib/lp/code/interfaces/cibuild.py
index bac6181..472b67e 100644
--- a/lib/lp/code/interfaces/cibuild.py
+++ b/lib/lp/code/interfaces/cibuild.py
@@ -105,6 +105,15 @@ class ICIBuildView(IPackageBuildView, IPrivacy):
         )
     )
 
+    git_refs = exported(
+        List(
+            TextLine(),
+            title=_("The git references that originated this CI Build."),
+            required=False,
+            readonly=True,
+        )
+    )
+
     distro_arch_series = exported(
         Reference(
             IDistroArchSeries,
@@ -274,6 +283,7 @@ class ICIBuildSet(ISpecificBuildFarmJobSource):
         distro_arch_series,
         stages,
         date_created=DEFAULT,
+        git_refs=None,
     ):
         """Create an `ICIBuild`."""
 
@@ -285,7 +295,9 @@ class ICIBuildSet(ISpecificBuildFarmJobSource):
             these Git commit IDs.
         """
 
-    def requestBuild(git_repository, commit_sha1, distro_arch_series, stages):
+    def requestBuild(
+        git_repository, commit_sha1, distro_arch_series, stages, git_refs=None
+    ):
         """Request a CI build.
 
         This checks that the architecture is allowed and that there isn't
diff --git a/lib/lp/code/model/cibuild.py b/lib/lp/code/model/cibuild.py
index 1cf5a19..0634d50 100644
--- a/lib/lp/code/model/cibuild.py
+++ b/lib/lp/code/model/cibuild.py
@@ -14,7 +14,16 @@ from operator import itemgetter
 
 from lazr.lifecycle.event import ObjectCreatedEvent
 from storm.databases.postgres import JSON
-from storm.locals import Bool, DateTime, Desc, Int, Reference, Store, Unicode
+from storm.locals import (
+    Bool,
+    DateTime,
+    Desc,
+    Int,
+    List,
+    Reference,
+    Store,
+    Unicode,
+)
 from storm.store import EmptyResultSet
 from zope.component import getUtility
 from zope.event import notify
@@ -171,12 +180,14 @@ def determine_DASes_to_build(configuration, logger=None):
 
 
 def get_all_commits_for_paths(git_repository, paths):
-    return [
-        ref.commit_sha1
-        for ref in GitRef.findByReposAndPaths(
-            [(git_repository, ref_path) for ref_path in paths]
-        ).values()
-    ]
+    commits = {}
+    for ref in GitRef.findByReposAndPaths(
+        [(git_repository, ref_path) for ref_path in paths]
+    ).values():
+        if ref.commit_sha1 not in commits:
+            commits[ref.commit_sha1] = []
+        commits[ref.commit_sha1].append(ref.path)
+    return commits
 
 
 def parse_configuration(git_repository, blob):
@@ -204,6 +215,7 @@ class CIBuild(PackageBuildMixin, StormBase):
     git_repository = Reference(git_repository_id, "GitRepository.id")
 
     commit_sha1 = Unicode(name="commit_sha1", allow_none=False)
+    git_refs = List(name="git_refs", allow_none=True)
 
     distro_arch_series_id = Int(name="distro_arch_series", allow_none=False)
     distro_arch_series = Reference(
@@ -261,12 +273,14 @@ class CIBuild(PackageBuildMixin, StormBase):
         builder_constraints,
         stages,
         date_created=DEFAULT,
+        git_refs=None,
     ):
         """Construct a `CIBuild`."""
         super().__init__()
         self.build_farm_job = build_farm_job
         self.git_repository = git_repository
         self.commit_sha1 = commit_sha1
+        self.git_refs = git_refs
         self.distro_arch_series = distro_arch_series
         self.processor = processor
         self.virtualized = virtualized
@@ -655,6 +669,7 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
         commit_sha1,
         distro_arch_series,
         stages,
+        git_refs=None,
         date_created=DEFAULT,
     ):
         """See `ICIBuildSet`."""
@@ -674,6 +689,7 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
             ),
             stages=stages,
             date_created=date_created,
+            git_refs=git_refs,
         )
         store.add(cibuild)
         store.flush()
@@ -712,7 +728,12 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
         ) is not None and self._isBuildableArchitectureAllowed(das)
 
     def requestBuild(
-        self, git_repository, commit_sha1, distro_arch_series, stages
+        self,
+        git_repository,
+        commit_sha1,
+        distro_arch_series,
+        stages,
+        git_refs=None,
     ):
         """See `ICIBuildSet`."""
         pocket = PackagePublishingPocket.UPDATES
@@ -726,17 +747,32 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
             CIBuild.distro_arch_series == distro_arch_series,
         )
         if not result.is_empty():
+            # We append the new git_refs to existing builds here to keep the
+            # git_refs list up-to-date, and potentially filter git repository
+            # webhooks by their git refs if the status of the build changes
+            if git_refs:
+                for cibuild in result:
+                    if cibuild.git_refs is None:
+                        cibuild.git_refs = []
+                    cibuild.git_refs.extend(git_refs)
             raise CIBuildAlreadyRequested
 
         build = self.new(
-            git_repository, commit_sha1, distro_arch_series, stages
+            git_repository, commit_sha1, distro_arch_series, stages, git_refs
         )
         build.queueBuild()
         notify(ObjectCreatedEvent(build))
         return build
 
     def _tryToRequestBuild(
-        self, git_repository, commit_sha1, configuration, das, stages, logger
+        self,
+        git_repository,
+        commit_sha1,
+        configuration,
+        das,
+        stages,
+        logger,
+        git_refs=None,
     ):
         try:
             if logger is not None:
@@ -746,7 +782,9 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
                     das.distroseries.name,
                     das.architecturetag,
                 )
-            build = self.requestBuild(git_repository, commit_sha1, das, stages)
+            build = self.requestBuild(
+                git_repository, commit_sha1, das, stages, git_refs
+            )
             # Create reports for each individual job in this build so that
             # they show up as pending in the web UI.  The job names
             # generated here should match those generated by
@@ -778,7 +816,7 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
         # getCommits performs a web request!
         commits = getUtility(IGitHostingClient).getCommits(
             git_repository.getInternalPath(),
-            commit_sha1s,
+            list(commit_sha1s),
             # XXX cjwatson 2022-01-19: We should also fetch
             # debian/.launchpad.yaml (or perhaps make the path a property of
             # the repository) once lpci and launchpad-buildd support using
@@ -814,6 +852,7 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
                     das,
                     stages[das.architecturetag],
                     logger,
+                    git_refs=commit_sha1s.get(commit["sha1"]),
                 )
 
     def getByID(self, build_id):
diff --git a/lib/lp/code/model/tests/test_cibuild.py b/lib/lp/code/model/tests/test_cibuild.py
index c49024a..bc3fd83 100644
--- a/lib/lp/code/model/tests/test_cibuild.py
+++ b/lib/lp/code/model/tests/test_cibuild.py
@@ -94,7 +94,7 @@ class TestGetAllCommitsForPaths(TestCaseWithFactory):
 
         rv = get_all_commits_for_paths(repository, ref_paths)
 
-        self.assertEqual([], rv)
+        self.assertEqual({}, rv)
 
     def test_one_ref_one_path(self):
         repository = self.factory.makeGitRepository()
@@ -104,7 +104,7 @@ class TestGetAllCommitsForPaths(TestCaseWithFactory):
         rv = get_all_commits_for_paths(repository, ref_paths)
 
         self.assertEqual(1, len(rv))
-        self.assertEqual(ref.commit_sha1, rv[0])
+        self.assertIn(ref.commit_sha1, rv)
 
     def test_multiple_refs_and_paths(self):
         repository = self.factory.makeGitRepository()
@@ -781,6 +781,56 @@ class TestCIBuildSet(TestCaseWithFactory):
         self.assertIsNone(build_queue.builder_constraints)
         self.assertEqual(BuildQueueStatus.WAITING, build_queue.status)
 
+    def test_requestCIBuild_with_git_refs(self):
+        # requestBuild creates a new CIBuild with the given git_refs
+        repository = self.factory.makeGitRepository()
+        commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
+        das = self.factory.makeBuildableDistroArchSeries()
+        stages = [[("build", 0)]]
+
+        git_refs = ["ref/test", "ref/foo"]
+        build = getUtility(ICIBuildSet).requestBuild(
+            repository, commit_sha1, das, stages, git_refs
+        )
+
+        self.assertTrue(ICIBuild.providedBy(build))
+        self.assertThat(
+            build,
+            MatchesStructure.byEquality(
+                git_repository=repository,
+                commit_sha1=commit_sha1,
+                distro_arch_series=das,
+                stages=stages,
+                status=BuildStatus.NEEDSBUILD,
+            ),
+        )
+        store = Store.of(build)
+        store.flush()
+        build_queue = store.find(
+            BuildQueue,
+            BuildQueue._build_farm_job_id
+            == removeSecurityProxy(build).build_farm_job_id,
+        ).one()
+        self.assertProvides(build_queue, IBuildQueue)
+        self.assertTrue(build_queue.virtualized)
+        self.assertIsNone(build_queue.builder_constraints)
+        self.assertEqual(BuildQueueStatus.WAITING, build_queue.status)
+        self.assertEqual(git_refs, build.git_refs)
+
+        # Rescheduling a build for the same commit_sha1 raises an error, but
+        # git_refs of the build are updated
+        self.assertRaises(
+            CIBuildAlreadyRequested,
+            getUtility(ICIBuildSet).requestBuild,
+            repository,
+            commit_sha1,
+            das,
+            stages,
+            ["ref/bar"],
+        )
+        git_refs.append("ref/bar")
+        self.assertEqual(git_refs, build.git_refs)
+
     def test_requestBuild_score(self):
         # CI builds have an initial queue score of 2600.
         repository = self.factory.makeGitRepository()
@@ -1022,6 +1072,7 @@ class TestCIBuildSet(TestCaseWithFactory):
                 )
             ),
         )
+        self.assertEqual(build.git_refs, ref_paths)
 
     def test_requestBuildsForRefs_multiple_architectures(self):
         ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
diff --git a/lib/lp/services/webhooks/browser.py b/lib/lp/services/webhooks/browser.py
index bb53917..991d7ef 100644
--- a/lib/lp/services/webhooks/browser.py
+++ b/lib/lp/services/webhooks/browser.py
@@ -134,6 +134,7 @@ class WebhookAddView(LaunchpadFormView):
             event_types=data["event_types"],
             active=data["active"],
             secret=data["secret"],
+            git_ref_pattern=data.get("git_ref_pattern"),
         )
         self.next_url = canonical_url(webhook)
 
diff --git a/lib/lp/services/webhooks/interfaces.py b/lib/lp/services/webhooks/interfaces.py
index b00d45f..9b32ba1 100644
--- a/lib/lp/services/webhooks/interfaces.py
+++ b/lib/lp/services/webhooks/interfaces.py
@@ -176,6 +176,18 @@ class IWebhook(Interface):
         )
     )
 
+    git_ref_pattern = exported(
+        TextLine(
+            title=_("Git ref pattern"),
+            required=False,
+            description=_(
+                "Pattern to match against git-ref/branch name of an event, "
+                "to filter webhook triggers"
+            ),
+            max_length=200,
+        )
+    )
+
     def getDelivery(id):
         """Retrieve a delivery by ID, or None if it doesn't exist."""
 
@@ -197,7 +209,15 @@ class IWebhook(Interface):
 
 
 class IWebhookSet(Interface):
-    def new(target, registrant, delivery_url, event_types, active, secret):
+    def new(
+        target,
+        registrant,
+        delivery_url,
+        event_types,
+        active,
+        secret,
+        git_ref_pattern=None,
+    ):
         """Create a new webhook."""
 
     def delete(hooks):
@@ -250,7 +270,12 @@ class IWebhookTarget(Interface):
     )
     @operation_for_version("devel")
     def newWebhook(
-        registrant, delivery_url, event_types, active=True, secret=None
+        registrant,
+        delivery_url,
+        event_types,
+        active=True,
+        secret=None,
+        git_ref_pattern=None,
     ):
         """Create a new webhook."""
 
diff --git a/lib/lp/services/webhooks/model.py b/lib/lp/services/webhooks/model.py
index bafd604..d5e1267 100644
--- a/lib/lp/services/webhooks/model.py
+++ b/lib/lp/services/webhooks/model.py
@@ -12,6 +12,7 @@ import ipaddress
 import re
 import socket
 from datetime import datetime, timedelta, timezone
+from fnmatch import fnmatch
 from urllib.parse import urlsplit
 
 import iso8601
@@ -66,6 +67,15 @@ def webhook_modified(webhook, event):
         removeSecurityProxy(webhook).date_last_modified = UTC_NOW
 
 
+def check_webhook_git_ref_pattern(webhook: IWebhook, git_ref: str):
+    """Check if a given git ref matches against the webhook's
+    `git_ref_pattern` if it has one (only Git Repository webhooks can have
+    a `git_ref_pattern` value)"""
+    if not webhook.git_ref_pattern:
+        return True
+    return fnmatch(git_ref, webhook.git_ref_pattern)
+
+
 @implementer(IWebhook)
 class Webhook(StormBase):
     """See `IWebhook`."""
@@ -114,6 +124,8 @@ class Webhook(StormBase):
 
     json_data = JSON(name="json_data")
 
+    git_ref_pattern = Unicode(allow_none=True)
+
     @property
     def target(self):
         if self.git_repository is not None:
@@ -192,7 +204,14 @@ class WebhookSet:
     """See `IWebhookSet`."""
 
     def new(
-        self, target, registrant, delivery_url, event_types, active, secret
+        self,
+        target,
+        registrant,
+        delivery_url,
+        event_types,
+        active,
+        secret,
+        git_ref_pattern=None,
     ):
         from lp.charms.interfaces.charmrecipe import ICharmRecipe
         from lp.code.interfaces.branch import IBranch
@@ -233,6 +252,7 @@ class WebhookSet:
         hook.delivery_url = delivery_url
         hook.active = active
         hook.secret = secret
+        hook.git_ref_pattern = git_ref_pattern
         hook.event_types = event_types
         IStore(Webhook).add(hook)
         IStore(Webhook).flush()
@@ -346,10 +366,22 @@ class WebhookTargetMixin:
         return self.valid_webhook_event_types
 
     def newWebhook(
-        self, registrant, delivery_url, event_types, active=True, secret=None
+        self,
+        registrant,
+        delivery_url,
+        event_types,
+        active=True,
+        secret=None,
+        git_ref_pattern=None,
     ):
         return getUtility(IWebhookSet).new(
-            self, registrant, delivery_url, event_types, active, secret
+            self,
+            registrant,
+            delivery_url,
+            event_types,
+            active,
+            secret,
+            git_ref_pattern,
         )
 
 
diff --git a/lib/lp/services/webhooks/tests/test_model.py b/lib/lp/services/webhooks/tests/test_model.py
index ede094c..7053899 100644
--- a/lib/lp/services/webhooks/tests/test_model.py
+++ b/lib/lp/services/webhooks/tests/test_model.py
@@ -20,7 +20,11 @@ from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.snapshot import notify_modified
 from lp.services.webhooks.interfaces import IWebhookSet
-from lp.services.webhooks.model import WebhookJob, WebhookSet
+from lp.services.webhooks.model import (
+    WebhookJob,
+    WebhookSet,
+    check_webhook_git_ref_pattern,
+)
 from lp.soyuz.interfaces.livefs import (
     LIVEFS_FEATURE_FLAG,
     LIVEFS_WEBHOOKS_FEATURE_FLAG,
@@ -71,6 +75,40 @@ class TestWebhook(TestCaseWithFactory):
                 webhook.event_types = ["foo", [1]]
             self.assertContentEqual([], webhook.event_types)
 
+    def test_check_webhook_git_ref_pattern(self):
+        # See lib/lp/code/templates/gitrepository-permissions.pt for an
+        # explanation of the wildcards logic
+        git_ref = "refs/heads/foo-test"
+        expected_results = {
+            None: True,
+            "": True,
+            "*": True,
+            "*foo*": True,
+            "foo": False,
+            "foo-test": False,  # it needs to match the full git ref
+            "foo*": False,
+            "*foo": False,
+            "*foo?test": True,
+            "*foo[-_.]test": True,
+            "*foo![-]test": False,
+            "*bar*": False,
+            "refs/heads/*": True,  # this should match all branches (not tags)
+            "refs/heads/foo*": True,
+            "refs/heads/bar*": False,
+            "refs/heads/*-test": True,
+        }
+
+        results = dict()
+        webhook = self.factory.makeWebhook()
+        with admin_logged_in():
+            for pattern in expected_results:
+                webhook.git_ref_pattern = pattern
+                results[pattern] = check_webhook_git_ref_pattern(
+                    webhook, git_ref
+                )
+
+        self.assertEqual(expected_results, results)
+
 
 class TestWebhookPermissions(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
@@ -112,6 +150,7 @@ class TestWebhookPermissions(TestCaseWithFactory):
                 "secret",
                 "setSecret",
                 "target",
+                "git_ref_pattern",
             },
         }
         webhook = self.factory.makeWebhook()
@@ -128,6 +167,7 @@ class TestWebhookPermissions(TestCaseWithFactory):
                 "event_types",
                 "registrant_id",
                 "secret",
+                "git_ref_pattern",
             },
         }
         webhook = self.factory.makeWebhook()
diff --git a/lib/lp/services/webhooks/tests/test_webservice.py b/lib/lp/services/webhooks/tests/test_webservice.py
index 810a83d..f7d9d0b 100644
--- a/lib/lp/services/webhooks/tests/test_webservice.py
+++ b/lib/lp/services/webhooks/tests/test_webservice.py
@@ -78,6 +78,7 @@ class TestWebhook(TestCaseWithFactory):
                 "self_link",
                 "target_link",
                 "web_link",
+                "git_ref_pattern",
             ),
         )
 
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 11d337b..2ac51bc 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -6163,6 +6163,7 @@ class LaunchpadObjectFactory(ObjectFactory):
         secret=None,
         active=True,
         event_types=None,
+        git_ref_pattern=None,
     ):
         if target is None:
             target = self.makeGitRepository()
@@ -6175,6 +6176,7 @@ class LaunchpadObjectFactory(ObjectFactory):
             event_types or [],
             active,
             secret,
+            git_ref_pattern,
         )
 
     def makeSnap(