github-branch-source-plugin icon indicating copy to clipboard operation
github-branch-source-plugin copied to clipboard

Return null for unmergable heads rather than excepting

Open mrginglymus opened this issue 4 years ago • 1 comments

Description

Triggered by https://github.com/jenkinsci/checks-api-plugin/pull/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 https://github.com/jenkinsci/github-branch-source-plugin/blob/ea3b723b4f12fbc3f06d985c9cc81dccea1ae2ae/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java#L1590 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

https://github.com/jenkinsci/github-branch-source-plugin/blob/ea3b723b4f12fbc3f06d985c9cc81dccea1ae2ae/src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java#L1328

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 avatar Feb 14 '21 09:02 mrginglymus

Having another look through the code, since the simplifications in https://github.com/jenkinsci/github-branch-source-plugin/pull/226 it may be that validateMergeHash could just return a bool.

mrginglymus avatar Feb 14 '21 10:02 mrginglymus