← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] ~ines-almeida/launchpad:webhook-patterns/add-functionality into launchpad:master

 

Ines Almeida has proposed merging ~ines-almeida/launchpad:webhook-patterns/add-functionality into launchpad:master with ~ines-almeida/launchpad:webhook-patterns/update-models as a prerequisite.

Commit message:
Filter git repository webhooks by git_ref_pattern

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/446982
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:webhook-patterns/add-functionality into launchpad:master.
diff --git a/lib/lp/code/model/branchjob.py b/lib/lp/code/model/branchjob.py
index e593b2b..8c9e639 100644
--- a/lib/lp/code/model/branchjob.py
+++ b/lib/lp/code/model/branchjob.py
@@ -723,7 +723,7 @@ class RevisionsAddedJob(BranchJobDerived):
 
         proposals = {}
         for proposal, source in result:
-            # Only show the must recent proposal for any given source.
+            # Only show the most recent proposal for any given source.
             date_created = proposal.date_created
             source_id = source.id
 
diff --git a/lib/lp/code/model/gitjob.py b/lib/lp/code/model/gitjob.py
index 566c728..1c51b94 100644
--- a/lib/lp/code/model/gitjob.py
+++ b/lib/lp/code/model/gitjob.py
@@ -253,8 +253,12 @@ class GitRefScanJob(GitJobDerived):
                     upserted_refs,
                     removed_refs,
                 )
+                git_refs = list(payload["ref_changes"].keys())
                 getUtility(IWebhookSet).trigger(
-                    self.repository, "git:push:0.1", payload
+                    self.repository,
+                    "git:push:0.1",
+                    payload,
+                    git_refs=git_refs,
                 )
         except LostObjectError:
             log.info(
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index 2a7007a..ecf4e0f 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -112,6 +112,7 @@ class WithVCSScenarios(WithScenarios):
         stacked_on=None,
         information_type=None,
         owner=None,
+        name=None,
     ):
         # Create the product pillar and its access policy if information
         # type is "PROPRIETARY".
@@ -128,10 +129,12 @@ class WithVCSScenarios(WithScenarios):
         kwargs = {"information_type": information_type, "owner": owner}
         if self.git:
             kwargs["target"] = product
-            return self.factory.makeGitRefs(**kwargs)[0]
+            paths = [name] if name else None
+            return self.factory.makeGitRefs(paths=paths, **kwargs)[0]
         else:
             kwargs["product"] = product
             kwargs["stacked_on"] = stacked_on
+            kwargs["name"] = name
             return self.factory.makeProductBranch(**kwargs)
 
     def makeBranchMergeProposal(
@@ -1297,16 +1300,17 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
         self.assertCorrectDelivery(expected_payload, hook, delivery)
         self.assertCorrectLogging(expected_redacted_payload, hook, logger)
 
-    def test_create_triggers_webhooks(self):
+    def test_create_triggers_webhooks(self, ref_pattern=None):
         # When a merge proposal is created, any relevant webhooks are
         # triggered.
         logger = self.useFixture(FakeLogger())
         source = self.makeBranch()
-        target = self.makeBranch(same_target_as=source)
+        target = self.makeBranch(same_target_as=source, name="foo-bar")
         registrant = self.factory.makePerson()
         hook = self.factory.makeWebhook(
             target=self.getWebhookTarget(target),
             event_types=["merge-proposal:0.1"],
+            ref_pattern=ref_pattern,
         )
         proposal = source.addLandingTarget(
             registrant, target, needs_review=True
@@ -1325,12 +1329,12 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
         self.assertCorrectDelivery(expected_payload, hook, delivery)
         self.assertCorrectLogging(expected_redacted_payload, hook, logger)
 
-    def test_modify_triggers_webhooks(self):
+    def test_modify_triggers_webhooks(self, ref_pattern=None):
         logger = self.useFixture(FakeLogger())
         # When an existing merge proposal is modified, any relevant webhooks
         # are triggered.
         source = self.makeBranch()
-        target = self.makeBranch(same_target_as=source)
+        target = self.makeBranch(same_target_as=source, name="foo-bar")
         registrant = self.factory.makePerson()
         proposal = source.addLandingTarget(
             registrant, target, needs_review=True
@@ -1338,6 +1342,7 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
         hook = self.factory.makeWebhook(
             target=self.getWebhookTarget(target),
             event_types=["merge-proposal:0.1"],
+            ref_pattern=ref_pattern,
         )
         login_person(target.owner)
         expected_payload = {
@@ -1364,12 +1369,12 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
         self.assertCorrectDelivery(expected_payload, hook, delivery)
         self.assertCorrectLogging(expected_redacted_payload, hook, logger)
 
-    def test_delete_triggers_webhooks(self):
+    def test_delete_triggers_webhooks(self, ref_pattern=None):
         # When an existing merge proposal is deleted, any relevant webhooks
         # are triggered.
         logger = self.useFixture(FakeLogger())
         source = self.makeBranch()
-        target = self.makeBranch(same_target_as=source)
+        target = self.makeBranch(same_target_as=source, name="foo-bar")
         registrant = self.factory.makePerson()
         proposal = source.addLandingTarget(
             registrant, target, needs_review=True
@@ -1377,6 +1382,7 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
         hook = self.factory.makeWebhook(
             target=self.getWebhookTarget(target),
             event_types=["merge-proposal:0.1"],
+            ref_pattern=ref_pattern,
         )
         login_person(target.owner)
         expected_payload = {
@@ -1394,6 +1400,77 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
         self.assertCorrectLogging(expected_redacted_payload, hook, logger)
 
 
+class TestGitMergeProposalWebhooks(TestMergeProposalWebhooks):
+    # Override scenarios to only have git related tests in this class
+    scenarios = [
+        ("git", {"git": True}),
+    ]
+
+    def test_create_triggers_webhooks_with_matching_ref_pattern(self):
+        self.test_create_triggers_webhooks("*foo*")
+
+    def test_modify_triggers_webhooks_with_matching_ref_pattern(self):
+        self.test_modify_triggers_webhooks("*foo*")
+
+    def test_delete_triggers_webhooks_with_matching_ref_pattern(self):
+        self.test_delete_triggers_webhooks("*foo*")
+
+    def test_create_doesnt_trigger_webhooks_without_matching_ref_pattern(self):
+        source = self.makeBranch()
+        target = self.makeBranch(same_target_as=source, name="foo-bar")
+        registrant = self.factory.makePerson()
+        hook = self.factory.makeWebhook(
+            target=self.getWebhookTarget(target),
+            event_types=["merge-proposal:0.1"],
+            ref_pattern="not-matching-test",
+        )
+        source.addLandingTarget(registrant, target, needs_review=True)
+        with admin_logged_in():
+            self.assertEqual(0, len(list(hook.deliveries)))
+
+    def test_modify_doesnt_trigger_webhooks_without_matching_ref_pattern(self):
+        source = self.makeBranch()
+        target = self.makeBranch(same_target_as=source, name="foo-bar")
+        registrant = self.factory.makePerson()
+        proposal = source.addLandingTarget(
+            registrant, target, needs_review=True
+        )
+        hook = self.factory.makeWebhook(
+            target=self.getWebhookTarget(target),
+            event_types=["merge-proposal:0.1"],
+            ref_pattern="not-matching-test",
+        )
+
+        with person_logged_in(
+            target.owner
+        ), BranchMergeProposalNoPreviewDiffDelta.monitor(proposal):
+            proposal.setStatus(
+                BranchMergeProposalStatus.CODE_APPROVED, user=target.owner
+            )
+            proposal.description = "An excellent proposal."
+
+        with admin_logged_in():
+            self.assertEqual(0, len(list(hook.deliveries)))
+
+    def test_delete_doesnt_trigger_webhooks_without_matching_ref_pattern(self):
+        source = self.makeBranch()
+        target = self.makeBranch(same_target_as=source, name="foo-bar")
+        registrant = self.factory.makePerson()
+        proposal = source.addLandingTarget(
+            registrant, target, needs_review=True
+        )
+        hook = self.factory.makeWebhook(
+            target=self.getWebhookTarget(target),
+            event_types=["merge-proposal:0.1"],
+            ref_pattern="not-matching-test",
+        )
+        with person_logged_in(target.owner):
+            proposal.deleteProposal()
+
+        with admin_logged_in():
+            self.assertEqual(0, len(list(hook.deliveries)))
+
+
 class TestGetAddress(TestCaseWithFactory):
     """Test that the address property gives expected results."""
 
diff --git a/lib/lp/code/model/tests/test_gitjob.py b/lib/lp/code/model/tests/test_gitjob.py
index 28f4a5f..58033d9 100644
--- a/lib/lp/code/model/tests/test_gitjob.py
+++ b/lib/lp/code/model/tests/test_gitjob.py
@@ -230,6 +230,69 @@ class TestGitRefScanJob(TestCaseWithFactory):
                 ),
             )
 
+    def test_ref_pattern_match_triggers_webhooks(self):
+        # Jobs trigger any relevant webhooks if their `ref_pattern` matches
+        # at least one of the git references
+        repository = self.factory.makeGitRepository()
+        self.factory.makeGitRefs(
+            repository, paths=["refs/heads/master", "refs/tags/1.0"]
+        )
+        hook = self.factory.makeWebhook(
+            target=repository,
+            event_types=["git:push:0.1"],
+            ref_pattern="*tags/1.0",
+        )
+        job = GitRefScanJob.create(repository)
+        paths = ("refs/heads/master", "refs/tags/2.0")
+        self.useFixture(GitHostingFixture(refs=self.makeFakeRefs(paths)))
+        with dbuser("branchscanner"):
+            JobRunner([job]).runAll()
+        delivery = hook.deliveries.one()
+        sha1 = lambda s: hashlib.sha1(s).hexdigest()
+        payload_matcher = MatchesDict(
+            {
+                "git_repository": Equals("/" + repository.unique_name),
+                "git_repository_path": Equals(repository.unique_name),
+                "ref_changes": Equals(
+                    {
+                        "refs/tags/1.0": {
+                            "old": {"commit_sha1": sha1(b"refs/tags/1.0")},
+                            "new": None,
+                        },
+                        "refs/tags/2.0": {
+                            "old": None,
+                            "new": {"commit_sha1": sha1(b"refs/tags/2.0")},
+                        },
+                    }
+                ),
+            }
+        )
+        self.assertThat(
+            delivery,
+            MatchesStructure(
+                event_type=Equals("git:push:0.1"), payload=payload_matcher
+            ),
+        )
+
+    def test_ref_pattern_fails_doesnt_trigger_webhooks(self):
+        # Jobs are not triggered if `ref_pattern` doesn't match any of the
+        # git references.
+        repository = self.factory.makeGitRepository()
+        self.factory.makeGitRefs(
+            repository, paths=["refs/heads/master", "refs/tags/1.0"]
+        )
+        hook = self.factory.makeWebhook(
+            target=repository, event_types=["git:push:0.1"], ref_pattern="*foo"
+        )
+        job = GitRefScanJob.create(repository)
+        paths = ("refs/heads/master", "refs/tags/2.0")
+        self.useFixture(GitHostingFixture(refs=self.makeFakeRefs(paths)))
+        with dbuser("branchscanner"):
+            JobRunner([job]).runAll()
+
+        with person_logged_in(repository.owner):
+            self.assertEqual(0, len(list(hook.deliveries)))
+
     def test_triggers_webhooks_with_oci_project_as_repository_target(self):
         # Jobs trigger any relevant webhooks when they're enabled.
         logger = self.useFixture(FakeLogger())
diff --git a/lib/lp/code/subscribers/branchmergeproposal.py b/lib/lp/code/subscribers/branchmergeproposal.py
index 4eb94dc..32da5a5 100644
--- a/lib/lp/code/subscribers/branchmergeproposal.py
+++ b/lib/lp/code/subscribers/branchmergeproposal.py
@@ -54,8 +54,19 @@ def _trigger_webhook(merge_proposal, payload):
         target = merge_proposal.target_branch
     else:
         target = merge_proposal.target_git_repository
+
+    git_refs = []
+    if "new" in payload:
+        git_refs.append(payload["new"]["target_git_path"])
+    if "old" in payload:
+        git_refs.append(payload["old"]["target_git_path"])
+
     getUtility(IWebhookSet).trigger(
-        target, "merge-proposal:0.1", payload, context=merge_proposal
+        target,
+        "merge-proposal:0.1",
+        payload,
+        context=merge_proposal,
+        git_refs=git_refs,
     )
 
 
diff --git a/lib/lp/services/webhooks/browser.py b/lib/lp/services/webhooks/browser.py
index 991d7ef..fdacce0 100644
--- a/lib/lp/services/webhooks/browser.py
+++ b/lib/lp/services/webhooks/browser.py
@@ -19,6 +19,7 @@ from lp.app.browser.launchpadform import (
     action,
 )
 from lp.app.widgets.itemswidgets import LabeledMultiCheckBoxWidget
+from lp.code.interfaces.gitrepository import IGitRepository
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
     LaunchpadView,
@@ -100,7 +101,14 @@ class WebhookBreadcrumb(Breadcrumb):
 
 class WebhookEditSchema(Interface):
     use_template(
-        IWebhook, include=["delivery_url", "event_types", "active", "secret"]
+        IWebhook,
+        include=[
+            "delivery_url",
+            "event_types",
+            "active",
+            "secret",
+            "ref_pattern",
+        ],
     )
 
 
@@ -111,6 +119,13 @@ class WebhookAddView(LaunchpadFormView):
     custom_widget_event_types = LabeledMultiCheckBoxWidget
     next_url = None
 
+    def initialize(self):
+        # Don't show `ref_pattern` in webhook creation page
+        self.field_names = ["delivery_url", "event_types", "active", "secret"]
+        if IGitRepository.providedBy(self.context):
+            self.field_names.append("ref_pattern")
+        super().initialize()
+
     @property
     def inside_breadcrumb(self):
         return WebhooksBreadcrumb(self.context)
@@ -143,11 +158,15 @@ class WebhookView(LaunchpadEditFormView):
     label = "Manage webhook"
 
     schema = WebhookEditSchema
-    # XXX wgrant 2015-08-04: Need custom widget for secret.
-    field_names = ["delivery_url", "event_types", "active"]
     custom_widget_event_types = LabeledMultiCheckBoxWidget
 
     def initialize(self):
+        # Don't show `ref_pattern` in webhook creation page
+        # # XXX wgrant 2015-08-04: Need custom widget for secret.
+        self.field_names = ["delivery_url", "event_types", "active"]
+        if IGitRepository.providedBy(self.context.target):
+            self.field_names.append("ref_pattern")
+
         super().initialize()
         cache = IJSONRequestCache(self.request)
         cache.objects["deliveries"] = list(self.deliveries.batch)
diff --git a/lib/lp/services/webhooks/interfaces.py b/lib/lp/services/webhooks/interfaces.py
index 72c2e12..f3309e0 100644
--- a/lib/lp/services/webhooks/interfaces.py
+++ b/lib/lp/services/webhooks/interfaces.py
@@ -240,7 +240,7 @@ class IWebhookSet(Interface):
     def findByTarget(target):
         """Find all webhooks for the given target."""
 
-    def trigger(target, event_type, payload, context=None):
+    def trigger(target, event_type, payload, context=None, git_refs=None):
         """Trigger subscribed webhooks to deliver a payload."""
 
 
diff --git a/lib/lp/services/webhooks/model.py b/lib/lp/services/webhooks/model.py
index 605d5c9..b2c2441 100644
--- a/lib/lp/services/webhooks/model.py
+++ b/lib/lp/services/webhooks/model.py
@@ -13,6 +13,7 @@ import re
 import socket
 from datetime import datetime, timedelta, timezone
 from fnmatch import fnmatch
+from typing import List
 from urllib.parse import urlsplit
 
 import iso8601
@@ -328,17 +329,35 @@ class WebhookSet:
             )
         )
 
-    def trigger(self, target, event_type, payload, context=None):
+    def _checkGitRefs(self, webhook: Webhook, git_refs: List[str]):
+        """Check if any of the `git_refs` matches the webhook's `ref_pattern`
+        if a `ref_pattern` and`git_refs` exist."""
+        if not webhook.ref_pattern or git_refs is None:
+            return True
+
+        for git_ref in git_refs:
+            if webhook.checkGitRefPattern(git_ref):
+                return True
+        return False
+
+    def trigger(
+        self, target, event_type, payload, context=None, git_refs=None
+    ):
         if context is None:
             context = target
         user = removeSecurityProxy(target).owner
         if not self._checkVisibility(context, user):
             return
+
         # XXX wgrant 2015-08-10: Two INSERTs and one celery submission for
         # each webhook, but the set should be small and we'd have to defer
         # the triggering itself to a job to fix it.
         for webhook in self.findByTarget(target):
-            if webhook.active and event_type in webhook.event_types:
+            if (
+                webhook.active
+                and event_type in webhook.event_types
+                and self._checkGitRefs(webhook, git_refs)
+            ):
                 WebhookDeliveryJob.create(webhook, event_type, payload)
 
 
diff --git a/lib/lp/services/webhooks/tests/test_browser.py b/lib/lp/services/webhooks/tests/test_browser.py
index f8a76a7..f054b38 100644
--- a/lib/lp/services/webhooks/tests/test_browser.py
+++ b/lib/lp/services/webhooks/tests/test_browser.py
@@ -14,6 +14,7 @@ from lp.charms.interfaces.charmrecipe import (
     CHARM_RECIPE_ALLOW_CREATE,
     CHARM_RECIPE_WEBHOOKS_FEATURE_FLAG,
 )
+from lp.code.interfaces.gitrepository import IGitRepository
 from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.interfaces import IPlacelessAuthUtility
@@ -418,19 +419,37 @@ class TestWebhookAddViewBase(WebhookTargetViewTestHelpers):
             ),
         )
 
-    def test_creates(self):
-        view = self.makeView(
-            "+new-webhook",
-            method="POST",
-            form={
-                "field.delivery_url": "http://example.com/test";,
-                "field.active": "on",
-                "field.event_types-empty-marker": "1",
-                "field.event_types": self.event_type,
-                "field.secret": "secret code",
-                "field.actions.new": "Add webhook",
-            },
-        )
+    def test_rendering_check_ref_pattern_field(self):
+        # Verify that `ref_pattern` field exists in webhooks form for git
+        # repositories, but not for other webhook pages
+        if IGitRepository.providedBy(self.target):
+            self.assertThat(
+                self.makeView("+new-webhook")(),
+                soupmatchers.HTMLContains(
+                    soupmatchers.TagWithId("field.ref_pattern")
+                ),
+            )
+        else:
+            self.assertThat(
+                self.makeView("+new-webhook")(),
+                soupmatchers.HTMLContains(
+                    soupmatchers.TagWithId("field.ref_pattern", count=0)
+                ),
+            )
+
+    def test_creates(self, ref_pattern_input=None, ref_pattern=None):
+        form_data = {
+            "field.delivery_url": "http://example.com/test";,
+            "field.active": "on",
+            "field.event_types-empty-marker": "1",
+            "field.event_types": self.event_type,
+            "field.secret": "secret code",
+            "field.actions.new": "Add webhook",
+        }
+        if ref_pattern_input:
+            form_data.update({"field.ref_pattern": ref_pattern_input})
+
+        view = self.makeView("+new-webhook", method="POST", form=form_data)
         self.assertEqual([], view.errors)
         hook = self.target.webhooks.one()
         self.assertThat(
@@ -442,9 +461,19 @@ class TestWebhookAddViewBase(WebhookTargetViewTestHelpers):
                 active=True,
                 event_types=[self.event_type],
                 secret="secret code",
+                ref_pattern=ref_pattern,
             ),
         )
 
+    def test_creates_with_ref_pattern_field(self):
+        # Adding webhook with `ref_pattern` field works for git repository
+        # webhooks, but not for other webhooks (creation is successful, but
+        # `ref_pattern` is not present)
+        if IGitRepository.providedBy(self.target):
+            self.test_creates(ref_pattern_input="*test*", ref_pattern="*test*")
+        else:
+            self.test_creates(ref_pattern_input="*test*", ref_pattern=None)
+
     def test_rejects_bad_scheme(self):
         transaction.commit()
         view = self.makeView(
@@ -603,16 +632,37 @@ class TestWebhookViewBase(WebhookViewTestHelpers):
             ),
         )
 
-    def test_saves(self):
+    def test_rendering_check_ref_pattern_field(self):
+        # Verify that `ref_pattern` field exists in webhooks form for git
+        # repositories, but not for other webhook pages
+        if IGitRepository.providedBy(self.target):
+            self.assertThat(
+                self.makeView("+index")(),
+                soupmatchers.HTMLContains(
+                    soupmatchers.TagWithId("field.ref_pattern")
+                ),
+            )
+        else:
+            self.assertThat(
+                self.makeView("+index")(),
+                soupmatchers.HTMLContains(
+                    soupmatchers.TagWithId("field.ref_pattern", count=0)
+                ),
+            )
+
+    def test_saves(self, ref_pattern_input=None, ref_pattern=None):
+        form_data = {
+            "field.delivery_url": "http://example.com/edited";,
+            "field.active": "off",
+            "field.event_types-empty-marker": "1",
+            "field.actions.save": "Save webhook",
+        }
+        if ref_pattern_input:
+            form_data.update({"field.ref_pattern": ref_pattern_input})
         view = self.makeView(
             "+index",
             method="POST",
-            form={
-                "field.delivery_url": "http://example.com/edited";,
-                "field.active": "off",
-                "field.event_types-empty-marker": "1",
-                "field.actions.save": "Save webhook",
-            },
+            form=form_data,
         )
         self.assertEqual([], view.errors)
         self.assertThat(
@@ -621,9 +671,19 @@ class TestWebhookViewBase(WebhookViewTestHelpers):
                 delivery_url="http://example.com/edited";,
                 active=False,
                 event_types=[],
+                ref_pattern=ref_pattern,
             ),
         )
 
+    def test_saves_with_ref_pattern_field(self):
+        # Editing `ref_pattern` field works for git repository webhooks, but
+        # not for other webhooks (edit is successful, but `ref_pattern` is not
+        # added)
+        if IGitRepository.providedBy(self.target):
+            self.test_saves(ref_pattern_input="*test*", ref_pattern="*test*")
+        else:
+            self.test_saves(ref_pattern_input="*test*", ref_pattern=None)
+
     def test_rejects_bad_scheme(self):
         transaction.commit()
         view = self.makeView(

References