launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30259
[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
