Add support for Github stacked pull requests behind opt in system property

[juliovalcarcel]

This pull request adds support for Github Stacked Pull Requests (behind a feature flag) by fixing a merge-strategy build failure caused by stacked pull requests. This is my first contribution to this repo or to a Jenkins plugin, so I have some questions/notes in the reviewers section to confirm my approach/design. Additionally I have put this up for review to get feedback now, but in addition to the unit tests, I have deployed this plugin to our Jenkins instance to run for the next week or two to see if we run into issues with it enabled. From my testing this all addresses our issues.

Description

When a stacked pull request targets the branch of another open pull request (PR B "the child" and PR A "the base"), the build of PR B fails at checkout with this error:

merge: <sha> - not something we can merge

The failure occurs when PR A is behind the target branch. In this case GitHub sets the base parent of the merge commit of PR B to the merge preview of PR A (refs/pull/A/merge), but no branch contains this commit. The plugin then uses this commit as the base and tries to merge against it, but Git cannot find the commit, so the build fails.

This pull request adds an opt-in system property which defaults to off. When enabled the plugin fetches refs/pull/N/merge and checks out the GitHub merge commit directly. My thought/design of this is that the plugin could either replicate the logic or just rely on GitHub's logic, and I thought it would be easier/more robust to use their logic. The merge commit is fetched into its own ref (…/<name>-merge) so it does not collide with the pull-head ref.

The new property is:

org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.preferGitHubMergeCommit

Related to #1545

Notes/Questions for Reviewers

• The plugin defers to GitHub's own logic for what the merge commit is and doesn't use Git locally. Is this the correct approach? Should I evaluate something different?
• The option changes the behavior for all merge pull requests, not just for stacked pull requests only. My thought was this is okay given that the alternative was to recreate and then have to maintain parallel logic to what Github is already doing so instead of a branching case, just rely on Github.
• I left this as a system property to start to allow for this be gradually added, once Github Stacked PRs are out of public preview this could graduate to a UI setting or maybe the default?
• This does not change when builds are triggered: equivalent() is untouched, so JENKINS-57583 ("don't rebuild a merge PR when only the target branch changed") still holds. A consequence is that a stacked PR does not automatically rebuild when only its base PR advances, the same as any merge PR today.

Reviewer's manual test instructions

1. Create PR A with a target of master.
2. Create PR B with a target of the branch of PR A.
3. Add a commit to master, so PR A is behind master.
4. Set the pull request strategy to merge, then build PR B. The build fails.
5. Enable the option by running org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.preferGitHubMergeCommit = true in the Script Console.
6. Build PR B again. The build completes.
7. Set the option back to false (or remove the property and restart). The behavior returns to the default.

Submitter checklist

• Link to Github ticket in description, if appropriate.
• Change is code complete and matches issue description
• Automated tests have been added to exercise the changes
• Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

• Run the changes and verify that the change matches the issue description
• Reviewed the code
• Verified that the appropriate tests have been written or valid explanation given

Documentation changes

• Link to jenkins.io PR, or an explanation for why no doc changes are needed

No documentation change is required. The option is an opt-in system property for preview. Happy to add documentation if requested.

Users/aliases to notify

[jglick]

I am not maintaining this plugin actively but FWIW: #1545 (comment)

(CC @jtnord)

[jtnord]

So stacks are fun..
If you have branch protections that require reviews, dismissing stale reviews then you can't merge them.
If you require CI status then with merge strategy the. I can see that you could also end up invalidating them as the stack merged because "fun things".

As for the commits, it is now possible to fetch commits via their sha ref, so adding that ref (or refs) consistently when fetching would be what I think we should do rather than attempt to re-implement things.

Have not yet looked at the code, this is late night thoughts.

[jtnord]

Should be able to fetch the commit directly (which should handle all cases?)

(although I recall (perhaps not GH but something else?) sometimes there was an race condition between the merge commit sha being in the metadata and it being available for pulling.)

[juliovalcarcel]

Updated and re-tested on my Jenkins instance (Swapped back to PR-Merge from PR-Head) and still worked. Let me know if there is something you want handled for the potential race condition, or if since this is feature gated, leave that for a future issue to resolve?

Also fixed SpotBugs.