Support Bitbucket push webhooks that contain multiple changes

[sandro-h]

Issue Type

• Feature enhancement

Summary

Normally, when a change is pushed to a Bitbucket repository, it issues a push webhook call with a single change element:

  "changes": [
    {
      "ref": {
        "id": "refs/heads/release/1.0",
        "displayId": "release/1.0",
        "type": "BRANCH"
      },
      "refId": "refs/heads/release/1.0",
      "fromHash": "cc3eb9e8f1c8d0a1bc4088b0fb863d0e371c178e",
      "toHash": "88dad105761a7beade0ecea5a873fa50c4a628f8",
      "type": "UPDATE"
    }
  ]

However, Bitbucket has an automatic branch merging feature, which propagates a single PR merge to multiple newer release branches. In this case, Bitbucket issues a single push webhook call with one change element for each target branch:

"changes": [
  {
    "ref": {
      "id": "refs/heads/release/1.1",
      "displayId": "release/1.1",
      "type": "BRANCH"
    },
    "refId": "refs/heads/release/1.1",
    "fromHash": "ccfeb36044ec53a2cb2ed258fcc4bf22d9d9c9c6",
    "toHash": "c3212a36f1d531b512dd6b4c9aaa19541c6256a0",
    "type": "UPDATE"
  },
  {
    "ref": {
      "id": "refs/heads/release/1.2",
      "displayId": "release/1.2",
      "type": "BRANCH"
    },
    "refId": "refs/heads/release/1.2",
    "fromHash": "4dd5ec79b362f506adf6de5084275d7bd9f8cea3",
    "toHash": "39e89ea6608f4c1293b8f03f7a3c4c8ca2b8cad6",
    "type": "UPDATE"
  },
  {
    "ref": {
      "id": "refs/heads/master",
      "displayId": "master",
      "type": "BRANCH"
    },
    "refId": "refs/heads/master",
    "fromHash": "a97d7d57257c39f2268e5ed4279be6dd99b29f8c",
    "toHash": "071e30bb9ee23a07fa82cea63f64e97a8c698f64",
    "type": "UPDATE"
  }
]

GoCD ignores all except the first change in this list:

gocd/api/api-webhook-v1/src/main/java/com/thoughtworks/go/apiv1/webhook/request/payload/push/HostedBitbucketPush.java

Lines 36 to 44 in 2daf703

	@Override
	public String branch() {
	return this.changes.stream()
	.filter(change -> change.ref != null)
	.filter(change -> equalsIgnoreCase(change.ref.type, "branch"))
	.map(change -> change.ref.displayId)
	.findFirst()
	.orElse(""); // if pushing a tag, this might be blank
	}

Therefore, even though a change happened on the remaining branches, the corresponding branch-specific pipelines are not triggered.

Suggested feature enhancement

Adjust the webhook logic to issue a material update for each element in the changes array.

I'm happy to provide a PR for this. Here's my proposed change. Please let me know if this makes sense before I start implementing anything:

• Modify PushPayload interface so branch() returns collection
• Modify the implementations accordingly:
  • Return all changes in the Bitbucket implementations (probably a Set, to guard against a branch appearing twice)
  • Return a singleton collection for GitHub and Gitlab (AFAIK they always have only one change per webhook call)
• Modify PushWebhookControllerV1.notify to call updateGitMaterial for each returned branch. This requires "merging" the response:
  • If all changes have an empty tag, return Ignoring push to non-branch.
  • If any change successfully triggers a material update, return success
  • If none of the changes trigger a material update, return No matching materials!

[chadlwilson]

At a high level, it seems entirely reasonable to support this, and in a general sense of course PRs are welcomed! How quickly we can effectively review them is a separate issue 😅

Personally I am not super familiar with the code in this area, but your approach sounds reasonable, and it seems the surface area of the changes are pretty limited. Any guidance from @marques-work , @kritika-singh3 or @maheshp on implementation approach or suitability of this proposed change would be useful.

The only main thoughts on design I might have are

• looks like Bitbucket Cloud perhaps doesn't support this yet, so maybe we can only test with hosted? https://jira.atlassian.com/browse/BCLOUD-14286
• do we think there is any harm in processing multiple for Github/Gitlab/BB-cloud? It's going to seem like a bit of a "WTF" when one looks at the code to have a return type of a Set but then to see them findFirst()
• I wonder what the trade-offs are for treating as success if only 1 successfully triggers, rather than if all trigger, and an error otherwise. Usually I'd try to understand what things can trigger a failure to figure out what's appropriate, and what the webhook caller will do in response (retry? log some warnings on Bitbucket Server side?) but not so familiar here so will leave it as "things to think about" 🙂
  • If we do decide on returning success on 1/n successful, perhaps we can vary the response message to indicate the partial success. (I am assuming the message content is irrelevant, except for debugging, and the 2xx status is the only relevant thing).

Personally, I can probably help out a bit with any of your local setup required, and getting the battery of tests to run on any PR you submit.

[sandro-h]

Thanks for being open to this 👍

looks like Bitbucket Cloud perhaps doesn't support this yet, so maybe we can only test with hosted?

Wow, I'm so used to Hosted lagging behind Cloud that I just assumed it's a thing on Cloud already 😄 I'm fine with limiting the change to Hosted, since that's what we use anyway.

On the other hand, as you state in your second point, if there is no harm in handling it for all host types, it would be more future proof to do it everywhere (i.e. if that cloud ticket is ever implemented). Based on the sample payloads from the tests, I don't think Gitlab or Github support multiple changes though, so they'll simply return a Set of 1 element.

Usually I'd try to understand what things can trigger a failure to figure out what's appropriate,

My assumption based on the existing code - where there is no explicit "failure" response - is that it can only fail in finding a matching material, and there are no other failure states (besides a runtime exception which is handled by a generic error handler I assume).

perhaps we can vary the response message to indicate the partial success

I was also thinking about this. My original thought was that the message could get rather large and wouldn't be handled in any automated way anyway. But you're right that it would help with debugging at least.

[sandro-h]

While we haven't heard from the other subject matter experts yet, I interpret the response as favorable, so I went ahead and opened a PR. No rush with the review though 🙂

[chadlwilson]

We finally released this with 22.1.0 today: https://www.gocd.org/releases/#22-1-0 🥳

[sandro-h]

Congrats on the release and thanks! 🥳 We'll be upgrading soon 🙂

[chadlwilson]

I realise I missed your name off the contributors; that's my bad. Will be fixed shortly - GitHub seems to be recovering from one of its frequent recent, err, "issues".