launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #32449
[Merge] ~alvarocs/launchpad:mp-webhooks into launchpad:master
Alvaro Crespo Serrano has proposed merging ~alvarocs/launchpad:mp-webhooks into launchpad:master.
Commit message:
Implement merge-proposal webhook sub-scopes
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~alvarocs/launchpad/+git/launchpad/+merge/485196
Teams that integrate Launchpad with CI systems need to react to specific merge-proposal changes (creation, reviews, status change...). The legacy 'merge-proposal:0.1' was triggered too often and not always. Moreover consumers couldn't know what actually changed.
Code changes:
-lp/services/webhooks/interfaces.py: register the 6 new subscopes
-lp/services/webhooks/model.py:
- WebhookSet.trigger() now accepts sub-scopes
-lp/code/subscribers/branchmergeproposal.py:
- _trigger_webhook now receives the event_type
- filter out any unknown possible webhook trigger (inline draft comments...)
- handlers emit the correct subscope
- new review_comment_created to trigger a webhook for top level comment.
-lib/lp/code/configure.zcml: link review_comment_created method for when a
comment is created
Tests:
-lp/code/model/tests/test_branchmergeproposal.py:
- update existing tests with event_type input
- new class TestMergeProposalWebhookGranularity to test subscopes
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~alvarocs/launchpad:mp-webhooks into launchpad:master.
diff --git a/lib/lp/code/configure.zcml b/lib/lp/code/configure.zcml
index 4e50bbf..95a84f9 100644
--- a/lib/lp/code/configure.zcml
+++ b/lib/lp/code/configure.zcml
@@ -296,6 +296,10 @@
for="lp.code.interfaces.branchmergeproposal.IBranchMergeProposal
lazr.lifecycle.interfaces.IObjectModifiedEvent"
handler="lp.code.subscribers.karma.branch_merge_modified"/>
+ <subscriber
+ for="lp.code.interfaces.codereviewcomment.ICodeReviewComment
+ lazr.lifecycle.interfaces.IObjectCreatedEvent"
+ handler="lp.code.subscribers.branchmergeproposal.review_comment_created"/>
<!-- hierarchy -->
diff --git a/lib/lp/code/model/tests/test_branchmergeproposal.py b/lib/lp/code/model/tests/test_branchmergeproposal.py
index e628757..03b4cb8 100644
--- a/lib/lp/code/model/tests/test_branchmergeproposal.py
+++ b/lib/lp/code/model/tests/test_branchmergeproposal.py
@@ -1235,11 +1235,13 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
}
return {k: Equals(v) for k, v in payload.items()}
- def assertCorrectDelivery(self, expected_payload, hook, delivery):
+ def assertCorrectDelivery(
+ self, expected_payload, hook, delivery, event_type
+ ):
self.assertThat(
delivery,
MatchesStructure(
- event_type=Equals("merge-proposal:0.1"),
+ event_type=Equals(event_type),
payload=MatchesDict(expected_payload),
),
)
@@ -1250,7 +1252,9 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
repr(delivery),
)
- def assertCorrectLogging(self, expected_redacted_payload, hook, logger):
+ def assertCorrectLogging(
+ self, expected_redacted_payload, hook, logger, event_type
+ ):
with dbuser(config.IWebhookDeliveryJobSource.dbuser):
self.assertThat(
logger.output,
@@ -1258,7 +1262,7 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
[
(
hook,
- "merge-proposal:0.1",
+ event_type,
MatchesDict(expected_redacted_payload),
)
]
@@ -1297,8 +1301,13 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
expected_payload,
new=MatchesDict(self.getExpectedPayload(proposal, redact=True)),
)
- self.assertCorrectDelivery(expected_payload, hook, delivery)
- self.assertCorrectLogging(expected_redacted_payload, hook, logger)
+ expected_event = "merge-proposal:0.1"
+ self.assertCorrectDelivery(
+ expected_payload, hook, delivery, expected_event
+ )
+ self.assertCorrectLogging(
+ expected_redacted_payload, hook, logger, expected_event
+ )
def test_create_triggers_webhooks(self):
# When a merge proposal is created, any relevant webhooks are
@@ -1325,8 +1334,13 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
expected_payload,
new=MatchesDict(self.getExpectedPayload(proposal, redact=True)),
)
- self.assertCorrectDelivery(expected_payload, hook, delivery)
- self.assertCorrectLogging(expected_redacted_payload, hook, logger)
+ expected_event = "merge-proposal:0.1"
+ self.assertCorrectDelivery(
+ expected_payload, hook, delivery, expected_event
+ )
+ self.assertCorrectLogging(
+ expected_redacted_payload, hook, logger, expected_event
+ )
def test_modify_triggers_webhooks(self):
logger = self.useFixture(FakeLogger())
@@ -1363,9 +1377,14 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
expected_redacted_payload["new"] = MatchesDict(
self.getExpectedPayload(proposal, redact=True)
)
+ expected_event = "merge-proposal:0.1"
delivery = hook.deliveries.one()
- self.assertCorrectDelivery(expected_payload, hook, delivery)
- self.assertCorrectLogging(expected_redacted_payload, hook, logger)
+ self.assertCorrectDelivery(
+ expected_payload, hook, delivery, expected_event
+ )
+ self.assertCorrectLogging(
+ expected_redacted_payload, hook, logger, expected_event
+ )
def test_delete_triggers_webhooks(self):
# When an existing merge proposal is deleted, any relevant webhooks
@@ -1393,8 +1412,13 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
)
proposal.deleteProposal()
delivery = hook.deliveries.one()
- self.assertCorrectDelivery(expected_payload, hook, delivery)
- self.assertCorrectLogging(expected_redacted_payload, hook, logger)
+ expected_event = "merge-proposal:0.1"
+ self.assertCorrectDelivery(
+ expected_payload, hook, delivery, expected_event
+ )
+ self.assertCorrectLogging(
+ expected_redacted_payload, hook, logger, expected_event
+ )
def _create_for_webhook_with_git_ref_pattern(
self, git_ref_pattern, expect_delivery
@@ -1524,6 +1548,147 @@ class TestMergeProposalWebhooks(WithVCSScenarios, TestCaseWithFactory):
)
+class TestMergeProposalWebhookGranularity(
+ WithVCSScenarios, TestCaseWithFactory
+):
+ """Fine-grained webhook behaviour for merge-proposals."""
+
+ # Use the full functional stack so updatePreviewDiff can store its diff
+ # via the in-memory test-librarian provided by the layer.
+ layer = LaunchpadFunctionalLayer
+
+ valid_diff = (
+ "diff --git a/foo b/foo\n"
+ "new file mode 100644\n"
+ "index 0000000..e69de29\n"
+ "--- /dev/null\n"
+ "+++ b/foo\n"
+ "@@ -0,0 +1 @@\n"
+ "+dummy\n"
+ )
+
+ def _make_mp_and_hook(self, event_types):
+ source = self.makeBranch()
+ target = self.makeBranch(same_target_as=source)
+ mp = source.addLandingTarget(
+ self.factory.makePerson(), target, needs_review=True
+ )
+ hook = self.factory.makeWebhook(
+ target=target.repository if self.git else target,
+ event_types=event_types,
+ )
+ return mp, hook, target
+
+ def _latest_delivery(self, hook):
+ with admin_logged_in():
+ return hook.deliveries.one()
+
+ def test_edit_triggers_webhook(self):
+ """Changing the description fires ::edit."""
+ mp, hook, target = self._make_mp_and_hook(["merge-proposal:0.1::edit"])
+
+ login_person(target.owner)
+ with BranchMergeProposalNoPreviewDiffDelta.monitor(mp):
+ mp.description = "New description"
+
+ self.assertEqual(
+ "merge-proposal:0.1::edit", self._latest_delivery(hook).event_type
+ )
+
+ def test_review_without_inlines_always_triggers(self):
+ """A main comment without inline comments fires exactly once
+ ::review webhook."""
+ mp, hook, target = self._make_mp_and_hook(
+ ["merge-proposal:0.1::review"]
+ )
+
+ login_person(target.owner)
+ preview = mp.updatePreviewDiff(self.valid_diff, "a", "b")
+ transaction.commit()
+
+ author = self.factory.makePerson()
+ login_person(author)
+ mp.createComment(
+ author,
+ subject="LGTM",
+ content="All good!",
+ previewdiff_id=preview.id,
+ vote=CodeReviewVote.APPROVE,
+ inline_comments={},
+ )
+ transaction.commit()
+
+ with admin_logged_in():
+ self.assertEqual(1, hook.deliveries.count())
+ self.assertEqual(
+ "merge-proposal:0.1::review",
+ self._latest_delivery(hook).event_type,
+ )
+
+ def test_review_with_main_and_inline_comments_trigger_once(self):
+ """A main comment and inline comments fires exactly one
+ ::review webhook."""
+ mp, hook, target = self._make_mp_and_hook(
+ ["merge-proposal:0.1::review"]
+ )
+ login_person(target.owner)
+
+ preview = mp.updatePreviewDiff(self.valid_diff, "a", "b")
+ transaction.commit()
+
+ author = self.factory.makePerson()
+ login_person(author)
+ mp.createComment(
+ author,
+ subject="LGTM-with-inline",
+ content="Looks fine with a couple of remarks.",
+ previewdiff_id=preview.id,
+ vote=CodeReviewVote.APPROVE,
+ inline_comments={"10": "one remark", "20": "another remark"},
+ )
+ transaction.commit()
+
+ with admin_logged_in():
+ self.assertEqual(1, hook.deliveries.count())
+ self.assertEqual(
+ "merge-proposal:0.1::review",
+ self._latest_delivery(hook).event_type,
+ )
+
+ def test_parent_scope_still_receives_create(self):
+ """A hook that only has 'merge-proposal:0.1' still fires
+ on ::create."""
+ target = self.makeBranch()
+ hook = self.factory.makeWebhook(
+ target=target.repository if self.git else target,
+ event_types=["merge-proposal:0.1"],
+ )
+
+ # Creating the MP should schedule the webhook.
+ login_person(target.owner)
+ source = self.makeBranch(same_target_as=target)
+ source.addLandingTarget(
+ self.factory.makePerson(), target, needs_review=True
+ )
+
+ self.assertEqual(
+ "merge-proposal:0.1", self._latest_delivery(hook).event_type
+ )
+
+ def test_subscope_does_not_leak_to_other_subscope(self):
+ """A hook for ::status-change is *not* triggered by ::edit."""
+ mp, hook, target = self._make_mp_and_hook(
+ ["merge-proposal:0.1::status-change"]
+ )
+
+ # Change something that is *not* the status (description).
+ login_person(target.owner)
+ mp.description = "just a wording tweak"
+ transaction.commit()
+ with admin_logged_in():
+ self.assertEqual(0, hook.deliveries.count())
+
+
class TestGetAddress(TestCaseWithFactory):
"""Test that the address property gives expected results."""
diff --git a/lib/lp/code/subscribers/branchmergeproposal.py b/lib/lp/code/subscribers/branchmergeproposal.py
index ec1874b..e4470dd 100644
--- a/lib/lp/code/subscribers/branchmergeproposal.py
+++ b/lib/lp/code/subscribers/branchmergeproposal.py
@@ -45,7 +45,7 @@ def _compose_merge_proposal_webhook_payload(merge_proposal):
)
-def _trigger_webhook(merge_proposal, payload):
+def _trigger_webhook(merge_proposal, payload, event_type):
payload = dict(payload)
payload["merge_proposal"] = canonical_url(
merge_proposal, force_local_path=True
@@ -63,13 +63,34 @@ def _trigger_webhook(merge_proposal, payload):
getUtility(IWebhookSet).trigger(
target,
- "merge-proposal:0.1",
+ event_type,
payload,
context=merge_proposal,
git_refs=git_refs,
)
+def review_comment_created(comment, event):
+ """Fire a webhook when a main comment is posted."""
+ merge_proposal = comment.branch_merge_proposal
+ base_payload = _compose_merge_proposal_webhook_payload(merge_proposal)
+ # Add review-specific fields to the payload
+ base_payload.update(
+ {
+ "review": comment.id,
+ "vote": comment.vote.title if comment.vote else None,
+ "author": comment.owner.name,
+ "content": comment.message.text_contents,
+ "date_created": comment.date_created.isoformat(),
+ }
+ )
+ payload = {
+ "action": "reviewed",
+ "new": base_payload,
+ }
+ _trigger_webhook(merge_proposal, payload, "merge-proposal:0.1::review")
+
+
def merge_proposal_created(merge_proposal, event):
"""A new merge proposal has been created.
@@ -80,7 +101,7 @@ def merge_proposal_created(merge_proposal, event):
"action": "created",
"new": _compose_merge_proposal_webhook_payload(merge_proposal),
}
- _trigger_webhook(merge_proposal, payload)
+ _trigger_webhook(merge_proposal, payload, "merge-proposal:0.1::create")
def merge_proposal_needs_review(merge_proposal, event):
@@ -94,6 +115,11 @@ def merge_proposal_needs_review(merge_proposal, event):
def merge_proposal_modified(merge_proposal, event):
"""Notify branch subscribers when merge proposals are updated."""
+ old_status = event.object_before_modification.queue_status
+ new_status = merge_proposal.queue_status
+ # No webhook triggered if the status and edited fields are the same
+ if event.edited_fields is None and old_status == new_status:
+ return
# Check the user.
if event.user is None:
return
@@ -101,8 +127,6 @@ def merge_proposal_modified(merge_proposal, event):
from_person = None
else:
from_person = IPerson(event.user)
- old_status = event.object_before_modification.queue_status
- new_status = merge_proposal.queue_status
in_progress_states = (
BranchMergeProposalStatus.WORK_IN_PROGRESS,
@@ -141,7 +165,24 @@ def merge_proposal_modified(merge_proposal, event):
for field in payload["old"]:
if not hasattr(event.object_before_modification, field):
payload["old"][field] = payload["new"][field]
- _trigger_webhook(merge_proposal, payload)
+
+ # Filter modified fields to trigger the corresponding webhook
+ edited_fields = event.edited_fields or []
+ if old_status != new_status:
+ _trigger_webhook(
+ merge_proposal,
+ payload,
+ event_type="merge-proposal:0.1::status-change",
+ )
+ elif "description" in edited_fields:
+ _trigger_webhook(
+ merge_proposal, payload, event_type="merge-proposal:0.1::edit"
+ )
+ elif "commit_message" in edited_fields:
+ _trigger_webhook(
+ merge_proposal, payload, event_type="merge-proposal:0.1::edit"
+ )
+ return
def review_requested(vote_reference, event):
@@ -161,4 +202,4 @@ def merge_proposal_deleted(merge_proposal, event):
"action": "deleted",
"old": _compose_merge_proposal_webhook_payload(merge_proposal),
}
- _trigger_webhook(merge_proposal, payload)
+ _trigger_webhook(merge_proposal, payload, "merge-proposal:0.1::delete")
diff --git a/lib/lp/services/webhooks/interfaces.py b/lib/lp/services/webhooks/interfaces.py
index 166c503..bee5de4 100644
--- a/lib/lp/services/webhooks/interfaces.py
+++ b/lib/lp/services/webhooks/interfaces.py
@@ -56,6 +56,13 @@ WEBHOOK_EVENT_TYPES = {
"git:push:0.1": "Git push",
"livefs:build:0.1": "Live filesystem build",
"merge-proposal:0.1": "Merge proposal",
+ # New granular MP scopes
+ "merge-proposal:0.1::create": "Create",
+ "merge-proposal:0.1::push": "Push",
+ "merge-proposal:0.1::review": "Review",
+ "merge-proposal:0.1::edit": "Edit",
+ "merge-proposal:0.1::status-change": "Status Change",
+ "merge-proposal:0.1::delete": "Delete",
"oci-recipe:build:0.1": "OCI recipe build",
"snap:build:0.1": "Snap build",
"craft-recipe:build:0.1": "Craft recipe build",
diff --git a/lib/lp/services/webhooks/model.py b/lib/lp/services/webhooks/model.py
index a3d6503..a13ef7f 100644
--- a/lib/lp/services/webhooks/model.py
+++ b/lib/lp/services/webhooks/model.py
@@ -359,13 +359,28 @@ class WebhookSet:
# 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.
+ parent_event_type = event_type.split("::", 1)[0]
for webhook in self.findByTarget(target):
+ # Support sub-scoped event types or the parent subscope.
+ matched_type = None
+ # Allow triggering if the webhook is subscribed to the exact
+ # subscope
if (
webhook.active
and event_type in webhook.event_types
and self._checkGitRefs(webhook, git_refs)
):
- WebhookDeliveryJob.create(webhook, event_type, payload)
+ matched_type = event_type
+ # Allow triggering if the webhook is subscribed to the parent
+ # subscope
+ elif (
+ webhook.active
+ and parent_event_type in webhook.event_types
+ and self._checkGitRefs(webhook, git_refs)
+ ):
+ matched_type = parent_event_type
+ if matched_type is not None:
+ WebhookDeliveryJob.create(webhook, matched_type, payload)
class WebhookTargetMixin:
Follow ups
