launchpad-reviewers team mailing list archive

[Bayard 'kit' Randel <kit.randel@xxxxxxxxxxxxx>]

Review: Approve

One query around the payload (realise this is a first pass), otherwise looks great.

Diff comments:

> 
> === modified file 'lib/lp/code/model/gitjob.py'
> --- lib/lp/code/model/gitjob.py	2015-07-09 20:06:17 +0000
> +++ lib/lp/code/model/gitjob.py	2015-08-10 12:52:50 +0000
> @@ -196,6 +198,26 @@
>          job.celeryRunOnCommit()
>          return job
>  
> +    @staticmethod
> +    def composeWebhookPayload(repository, refs_to_upsert, refs_to_remove):
> +        old_refs = {ref.path: ref for ref in repository.refs}
> +        ref_changes = {}
> +        for ref in refs_to_upsert.keys() + list(refs_to_remove):
> +            old = (
> +                {"commit_sha1": old_refs[ref].commit_sha1}
> +                if ref in old_refs else None)
> +            new = (
> +                {"commit_sha1": refs_to_upsert[ref]['sha1']}
> +                if ref in refs_to_upsert else None)
> +            # planRefChanges can return an unchanged ref if the cached
> +            # commit details differ.
> +            if old != new:
> +                ref_changes[ref] = {"old": old, "new": new}
> +        return {
> +            "git_repository_path": repository.shortened_path,
> +            "ref_changes": ref_changes,
> +            }
> +

Would it be worth including an explicit reference to the new head, and perhaps a timestamp?

>      def run(self):
>          """See `IGitRefScanJob`."""
>          try:


-- 
https://code.launchpad.net/~wgrant/launchpad/webhook-git-push/+merge/267510
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.