launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Diff comments:

> === modified file 'lib/lp/code/adapters/branch.py'
> --- lib/lp/code/adapters/branch.py	2015-07-08 16:05:11 +0000
> +++ lib/lp/code/adapters/branch.py	2015-10-09 16:59:27 +0000
> @@ -80,8 +81,11 @@
>      delta_values = (
>          'registrant',
>          'source_branch',
> +        'source_git_ref',
>          'target_branch',
> +        'target_git_ref',
>          'prerequisite_branch',
> +        'prerequisite_git_ref',

I don't think that's important.  All that matters is that there's enough type information in the interface to serialise them, which there is.  I prefer describing changes in terms of the combined fields here.

>          'queue_status',
>          )
>      new_values = (
> @@ -111,6 +115,17 @@
>          return BranchMergeProposalDelta(**delta.changes)
>  
>      @classmethod
> +    def composeWebhookPayload(klass, old_merge_proposal, new_merge_proposal):
> +        """Return part of a webhook payload representing the differences."""
> +        return {
> +            "old": compose_webhook_payload(
> +                IBranchMergeProposal, old_merge_proposal, klass.delta_values),
> +            "new": compose_webhook_payload(
> +                IBranchMergeProposal, new_merge_proposal,
> +                klass.delta_values + klass.new_values),
> +            }

The subsetting is a little odd, but many of the fields are useful and (as discussed) I didn't want to serialise the entire object.  On the other hand, GitHub's pull_request events provide quite a bit of state information, and I don't want to require webhook endpoints to make webservice round-trips to get the same information that they could get by screen-scraping mail notifications.  The values monitored by BranchMergeProposalDelta seem like a reasonable compromise here.

Comments should, I think, be a separate event.  I'm inclined to put votes together with them in something like the same way they look in the conversation view on merge proposals, but I haven't worked out the exact details yet.  These seem less urgent since CI integration can function without them.

Point taken on the other inconsistencies, and I've tidied those up.

> +
> +    @classmethod
>      def snapshot(klass, merge_proposal):
>          """Return a snapshot suitable for use with construct.
>  
> 
> === modified file 'lib/lp/services/webhooks/interfaces.py'
> --- lib/lp/services/webhooks/interfaces.py	2015-10-05 11:11:47 +0000
> +++ lib/lp/services/webhooks/interfaces.py	2015-10-09 16:59:27 +0000
> @@ -75,6 +77,7 @@
>  WEBHOOK_EVENT_TYPES = {
>      "bzr:push:0.1": "Bazaar push",
>      "git:push:0.1": "Git push",
> +    "merge-proposal:0.1": "Merge proposal",

GitHub also has a nearly all-encompassing "pull_request" event, so I think this is similar.  There are two exceptions: "issue_comment" (which covers comments on both issues and pull requests) and "pull_request_review_comment" (which is quite fine-grained and akin to an inline comment).  It seems reasonable to separate these on the grounds that e.g. CI jobs would rarely care about those.

>      }
>  
>  


-- 
https://code.launchpad.net/~cjwatson/launchpad/bmp-webhooks/+merge/274005
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.