launchpad-reviewers team mailing list archive

[Ines Almeida <mp+485196@xxxxxxxxxxxxxxxxxx>]

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.