Thanks for this work!
Added a few initial thoughts that we can go through together later
Diff comments:
> 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
> @@ -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(
I'm on the fence about this.
IMO the point was to not substantially modify the webhooks that exist but just
subscope them - this was so that users wouldn't really feel these differences
unless they subscope their existing webhooks.
Here we are modifying it a lot more - which is nice because we are adding
detail, but makes merge-proposal:0.1 payloads not consistent. If it were to
change substancially, we might as well create new webhook scopes instead of
subscopes. See last comment as well.
> + {
> + "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.
>
> @@ -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:
Given you are already filtering out the `edited_fields == None` case above, do
we need to check specific fields here? If there are any edited_fields I'm
thinking it should be an `::edit` case always. This would also make this logic
simpler
> + _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):
> 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
We can discuss if we want to go this way, but this doesn't match the spec. In
the spec we always trigger it with the old (parent) scope - there is even a git
patch of the code in the spec.
The idea was that all webhooks would trigger the parent webhook type
(`merge-proposal:0.1`) and the subscopping simply filtered the triggering.
Otherwise this is just the same as creating new webhooks, it's not really a
sub-scopping of the existing webhook.
My preference would be to always trigger the `merge-proposal:0.1` webhook,
regardless of subscopes, and eventually adding an extra field to the payload
about the what triggered it (we already have the `action` but I wouldn't want
to change the existing behaviour, so perhaps this would need to be a new
field...).
Let's discuss further
> + # 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:
--
https://code.launchpad.net/~alvarocs/launchpad/+git/launchpad/+merge/485196
Your team Launchpad code reviewers is requested to review the proposed merge of
~alvarocs/launchpad:mp-webhooks into launchpad:master.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
