Return null for unmergable heads rather than excepting

[mrginglymus]

Description

Triggered by jenkinsci/checks-api-plugin#83.

I have zombie jobs (job is aborted, but agents remain busy executing a dead job) seemingly triggered by the following stack trace:

00:32:24.679  hudson.AbortException: Pull request 5852 : Not mergeable at xxxxxxxx+xxxxxxxxxxxx (NOT_MERGEABLE)
00:32:24.679      at org.jenkinsci.plugins.github_branch_source.PullRequestSCMRevision.validateMergeHash(PullRequestSCMRevision.java:106)
00:32:24.679      at org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.retrieve(GitHubSCMSource.java:1599)
00:32:24.679      at jenkins.scm.api.SCMSource.fetch(SCMSource.java:582)
00:32:24.679      at io.jenkins.plugins.checks.github.SCMFacade.findRevision(SCMFacade.java:156)
00:32:24.679  Caused: java.lang.IllegalStateException: Could not fetch revision from repository: xxxxxxxxxxxx
00:32:24.679      at io.jenkins.plugins.checks.github.SCMFacade.findRevision(SCMFacade.java:159)
00:32:24.679      at io.jenkins.plugins.checks.github.GitHubSCMSourceChecksContext.resolveHeadSha(GitHubSCMSourceChecksContext.java:131)
00:32:24.679      at io.jenkins.plugins.checks.github.GitHubSCMSourceChecksContext.<init>(GitHubSCMSourceChecksContext.java:46)
00:32:24.679      at io.jenkins.plugins.checks.github.GitHubSCMSourceChecksContext.fromRun(GitHubSCMSourceChecksContext.java:24)
00:32:24.679      at io.jenkins.plugins.checks.github.GitHubChecksPublisherFactory.createPublisher(GitHubChecksPublisherFactory.java:42)
00:32:24.679      at io.jenkins.plugins.checks.api.ChecksPublisherFactory.lambda$fromRun$0(ChecksPublisherFactory.java:89)
...
00:32:24.679      at io.jenkins.plugins.checks.api.ChecksPublisherFactory.fromRun(ChecksPublisherFactory.java:92)
00:32:24.679      at io.jenkins.plugins.checks.api.ChecksPublisherFactory.fromRun(ChecksPublisherFactory.java:69)
00:32:24.679      at io.jenkins.plugins.checks.steps.WithChecksStep$WithChecksStepExecution.publish(WithChecksStep.java:150)
00:32:24.679      at io.jenkins.plugins.checks.steps.WithChecksStep$WithChecksStepExecution.access$100(WithChecksStep.java:83)
00:32:24.679      at io.jenkins.plugins.checks.steps.WithChecksStep$WithChecksStepExecution$WithChecksCallBack.onStart(WithChecksStep.java:169)

The environmental behaviour that lead to this I believe was my force-pushing to the PR.

Now while there are separate issues in many different plugins that lead to this, the root exception in validateMergeHash I believe should in fact be handled in its direct caller.

This PR updates

github-branch-source-plugin/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

Line 1590 in ea3b723

protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOException, InterruptedException {

to now return null if given an unmergeable PR head rather than excepting.

This should not affect downstream consumers as this API is marked as @CheckForNull.

Note that this would bring it line with

github-branch-source-plugin/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

Line 1328 in ea3b723

protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener listener, Item retrieveContext)

which does return null for unmergeable heads (despite the API not being marked @CheckForNull).

I'm not certain that this is the best/correct way to solve this problem; I did look at fixing it in the github-checks-api plugin, but I did not want to leak the checking of this specific and ignorable issue across modules; as validateMergeHash only throws a generic AbortException it is indistinguishable from the many other uses of AbortExceptions throughout this plugin.

Submitter checklist

• Link to JIRA 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

Users/aliases to notify

cc: @timja

[mrginglymus]

Having another look through the code, since the simplifications in #226 it may be that validateMergeHash could just return a bool.

[timja]

Looks like it fixes the issue

[bitwiseman]

We only need to do this if the prHead is a merge, right?
Maybe encapsulate this in a method and call from the two different places we currently call validateMergeHash()?
getValidatedMergeOrNull()?

[bitwiseman]

This looks reasonable. A little refactoring suggested.
Also, can you create a test that exercises this? If not, I won't block the merge, but without a test there's no guarantee it won't break in the future. 😢