Skip to content

Commit eb7e1ce

Browse files
bitwisemanbitwiseman
committed
[JENKINS-57371] - Enable graceful fallback for merge hash
We tried using strict merge commit handling to prevent cloning on master. This resulted in some edge cases for some users where PRs would simply not build and there was not viable workaround. Change: * Merge hash has been changed to best effort. If it is missing or inconsistent, Jenkins will gracefully revert to cloning on master. * Most validity checking for merge hash has been moved to create time. * Plugin tries harder than before to create a valid merge hash revision.
1 parent 5b2434f commit eb7e1ce

File tree

6 files changed

+212
-131
lines changed

6 files changed

+212
-131
lines changed

‎src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java‎

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@
3333
import hudson.model.Item;
3434
import hudson.plugins.git.GitSCM;
3535
import hudson.scm.SCM;
36+
37+
import java.io.FileNotFoundException;
3638
import java.io.IOException;
3739
import java.io.OutputStream;
3840
import java.nio.charset.StandardCharsets;
@@ -88,7 +90,6 @@ protected GitHubSCMFileSystem(GitHub gitHub, GHRepository repo, String refName,
8890
PullRequestSCMRevision prRev = (PullRequestSCMRevision) rev;
8991
PullRequestSCMHead pr = (PullRequestSCMHead) prRev.getHead();
9092
if (pr.isMerge()) {
91-
prRev.validateMergeHash(repo);
9293
this.ref = prRev.getMergeHash();
9394
} else {
9495
this.ref = prRev.getPullHash();
@@ -279,7 +280,17 @@ public SCMFileSystem build(@NonNull SCMSource source, @NonNull SCMHead head, @Ch
279280
refName = "tags/" + head.getName();
280281
} else if (head instanceof PullRequestSCMHead) {
281282
refName = null;
282-
if (rev == null) {
283+
if (rev != null && rev instanceof PullRequestSCMRevision) {
284+
PullRequestSCMRevision prRev = (PullRequestSCMRevision) rev;
285+
if (((PullRequestSCMHead)head).isMerge()) {
286+
if (prRev.getMergeHash() == null) {
287+
// we need to release here as we are not throwing an exception or transferring responsibility to FS
288+
Connector.release(github);
289+
return null;
290+
}
291+
prRev.validateMergeHash();
292+
}
293+
} else {
283294
// we need to release here as we are not throwing an exception or transferring responsibility to FS
284295
Connector.release(github);
285296
return null;

‎src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java‎

Lines changed: 66 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,14 +1081,19 @@ private static void retrievePullRequest(
10811081
}
10821082
return;
10831083
}
1084-
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
10851084
for (final ChangeRequestCheckoutStrategy strategy : strategies.get(fork)) {
10861085
final String branchName;
10871086
if (strategies.get(fork).size() == 1) {
10881087
branchName = "PR-" + number;
10891088
} else {
10901089
branchName = "PR-" + number + "-" + strategy.name().toLowerCase(Locale.ENGLISH);
10911090
}
1091+
1092+
// PR details only needed for merge PRs
1093+
if (strategy == ChangeRequestCheckoutStrategy.MERGE) {
1094+
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
1095+
}
1096+
10921097
if (request.process(new PullRequestSCMHead(
10931098
pr, branchName, strategy == ChangeRequestCheckoutStrategy.MERGE
10941099
),
@@ -1251,7 +1256,8 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l
12511256
int number = Integer.parseInt(prMatcher.group(1));
12521257
listener.getLogger().format("Attempting to resolve %s as pull request %d%n", headName, number);
12531258
try {
1254-
GHPullRequest pr = getDetailedGHPullRequest(number, listener, github, ghRepository);
1259+
Connector.checkApiRateLimit(listener, github);
1260+
GHPullRequest pr = ghRepository.getPullRequest(number);
12551261
if (pr != null) {
12561262
boolean fork = !ghRepository.getOwner().equals(pr.getHead().getUser());
12571263
Set<ChangeRequestCheckoutStrategy> strategies;
@@ -1304,12 +1310,15 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l
13041310
PullRequestSCMHead head = new PullRequestSCMHead(
13051311
pr, headName, strategy == ChangeRequestCheckoutStrategy.MERGE
13061312
);
1313+
if (head.isMerge()) {
1314+
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
1315+
}
13071316
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, head, listener, github, ghRepository);
13081317

13091318
switch (strategy) {
13101319
case MERGE:
13111320
try {
1312-
prRev.validateMergeHash(ghRepository);
1321+
prRev.validateMergeHash();
13131322
} catch (AbortException e) {
13141323
listener.getLogger().format("Resolved %s as pull request %d: %s.%n%n",
13151324
headName,
@@ -1499,12 +1508,13 @@ protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOExc
14991508
repositoryUrl = ghRepository.getHtmlUrl();
15001509
if (head instanceof PullRequestSCMHead) {
15011510
PullRequestSCMHead prhead = (PullRequestSCMHead) head;
1502-
1503-
GHPullRequest pr = getDetailedGHPullRequest(prhead.getNumber(), listener, github, ghRepository);
1504-
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, prhead, listener, github, ghRepository);
1511+
Connector.checkApiRateLimit(listener, github);
1512+
GHPullRequest pr = ghRepository.getPullRequest(prhead.getNumber());
15051513
if (prhead.isMerge()) {
1506-
prRev.validateMergeHash(ghRepository);
1514+
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
15071515
}
1516+
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, prhead, listener, github, ghRepository);
1517+
prRev.validateMergeHash();
15081518
return prRev;
15091519
} else if (head instanceof GitHubTagSCMHead) {
15101520
GitHubTagSCMHead tagHead = (GitHubTagSCMHead) head;
@@ -1532,26 +1542,57 @@ private static PullRequestSCMRevision createPullRequestSCMRevision(GHPullRequest
15321542
String baseHash = pr.getBase().getSha();
15331543
String prHeadHash = pr.getHead().getSha();
15341544
String mergeHash = null;
1535-
switch (prhead.getCheckoutStrategy()) {
1536-
case MERGE:
1537-
Connector.checkApiRateLimit(listener, github);
1538-
baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha();
1539-
if (pr.getMergeable() != null) {
1540-
mergeHash = Boolean.TRUE.equals(pr.getMergeable()) ? pr.getMergeCommitSha() : PullRequestSCMRevision.NOT_MERGEABLE_HASH;
1545+
1546+
if (prhead.isMerge()) {
1547+
if (Boolean.FALSE.equals(pr.getMergeable())) {
1548+
mergeHash = PullRequestSCMRevision.NOT_MERGEABLE_HASH;
1549+
} else if (Boolean.TRUE.equals(pr.getMergeable())) {
1550+
String proposedMergeHash = pr.getMergeCommitSha();
1551+
GHCommit commit = null;
1552+
try {
1553+
commit = ghRepository.getCommit(proposedMergeHash);
1554+
} catch (FileNotFoundException e) {
1555+
listener.getLogger().format("Pull request %s : github merge_commit_sha not found (%s). Close and reopen the PR to reset its merge hash.%n",
1556+
pr.getNumber(),
1557+
proposedMergeHash);
1558+
} catch (IOException e) {
1559+
throw new AbortException("Error while retrieving pull request " + pr.getNumber() + " merge hash : " + e.toString());
15411560
}
1542-
break;
1543-
default:
1544-
break;
1545-
}
1546-
return new PullRequestSCMRevision(prhead, baseHash, prHeadHash, mergeHash);
1547-
}
15481561

1562+
if (commit != null) {
1563+
List<String> parents = commit.getParentSHA1s();
1564+
// Merge commits always merge against the most recent base commit they can detect.
1565+
if (parents.size() != 2) {
1566+
listener.getLogger().format("WARNING: Invalid github merge_commit_sha for pull request %s : merge commit %s with parents - %s.%n",
1567+
pr.getNumber(),
1568+
proposedMergeHash,
1569+
StringUtils.join(parents, "+"));
1570+
} else if (!parents.contains(prHeadHash)) {
1571+
// This is maintains the existing behavior from pre-2.5.x when the merge_commit_sha is out of sync from the requested prHead
1572+
listener.getLogger().format("WARNING: Invalid github merge_commit_sha for pull request %s : Head commit %s does match merge commit %s with parents - %s.%n",
1573+
pr.getNumber(),
1574+
prHeadHash,
1575+
proposedMergeHash,
1576+
StringUtils.join(parents, "+"));
1577+
} else {
1578+
// We found a merge_commit_sha with 2 parents and one matches the prHeadHash
1579+
// Use the other parent hash as the base. This keeps the merge hash in sync with head and base.
1580+
// It is possible that head or base hash will not exist in their branch by the time we build
1581+
// This is be true (and cause a failure) regardless of how we determine the commits.
1582+
mergeHash = proposedMergeHash;
1583+
baseHash = prHeadHash.equals(parents.get(0)) ? parents.get(1) : parents.get(0);
1584+
}
1585+
}
1586+
}
1587+
1588+
// Merge PR jobs always merge against the most recent base branch commit they can detect.
1589+
// For an invalid merge_commit_sha, we need to query for most recent base commit separately
1590+
if (mergeHash == null) {
1591+
baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha();
1592+
}
1593+
}
15491594

1550-
private static GHPullRequest getDetailedGHPullRequest(int number, TaskListener listener, GitHub github, GHRepository ghRepository) throws IOException, InterruptedException {
1551-
Connector.checkApiRateLimit(listener, github);
1552-
GHPullRequest pr = ghRepository.getPullRequest(number);
1553-
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
1554-
return pr;
1595+
return new PullRequestSCMRevision(prhead, baseHash, prHeadHash, mergeHash);
15551596
}
15561597

15571598
private static void ensureDetailedGHPullRequest(GHPullRequest pr, TaskListener listener, GitHub github, GHRepository ghRepository) throws IOException, InterruptedException {
@@ -1561,7 +1602,7 @@ private static void ensureDetailedGHPullRequest(GHPullRequest pr, TaskListener l
15611602
Connector.checkApiRateLimit(listener, github);
15621603
while (pr.getMergeable() == null && retryCountdown > 1) {
15631604
listener.getLogger().format(
1564-
"Could not determine the mergability of pull request %d. Retrying %d more times...%n",
1605+
"Waiting for GitHub to provide a merge commit for pull request %d. Retrying %d more times...%n",
15651606
pr.getNumber(),
15661607
retryCountdown);
15671608
retryCountdown -= 1;

‎src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java‎

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -106,27 +106,9 @@ public String getMergeHash() {
106106
return mergeHash;
107107
}
108108

109-
void validateMergeHash(@NonNull GHRepository repo) throws IOException {
110-
if (!isMerge()) {
111-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not a merge head");
112-
} else if (this.mergeHash == null) {
113-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Unknown merge state " + this.toString());
114-
} else if (this.mergeHash == NOT_MERGEABLE_HASH) {
115-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not mergeable " + this.toString());
116-
} else {
117-
GHCommit commit = null;
118-
try {
119-
commit = repo.getCommit(this.mergeHash);
120-
} catch (FileNotFoundException e) {
121-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : commit not found (" + this.mergeHash + "). Close and reopen the PR to reset its merge hash.");
122-
} catch (IOException e) {
123-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : " + e.toString());
124-
}
125-
assert(commit != null);
126-
List<String> parents = commit.getParentSHA1s();
127-
if (parents.size() != 2 || !parents.contains(this.getBaseHash()) || !parents.contains(this.getPullHash())) {
128-
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Head and base commits do match merge commit " + this.toString() );
129-
}
109+
void validateMergeHash() throws AbortException {
110+
if (this.mergeHash == NOT_MERGEABLE_HASH) {
111+
throw new AbortException("Pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not mergeable at " + this.toString());
130112
}
131113
}
132114

‎src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystemTest.java‎

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import java.io.InputStream;
3939
import java.nio.charset.StandardCharsets;
4040
import jenkins.plugins.git.AbstractGitSCMSource;
41+
import jenkins.plugins.git.GitSCMFileSystem;
4142
import jenkins.scm.api.SCMFile;
4243
import jenkins.scm.api.SCMFileSystem;
4344
import jenkins.scm.api.SCMHead;
@@ -47,6 +48,7 @@
4748

4849
import org.apache.commons.io.IOUtils;
4950
import org.hamcrest.Matchers;
51+
import org.hamcrest.core.IsNull;
5052
import org.junit.Before;
5153
import org.junit.ClassRule;
5254
import org.junit.Rule;
@@ -68,6 +70,7 @@
6870
import static org.hamcrest.Matchers.notNullValue;
6971
import static org.hamcrest.core.IsNull.nullValue;
7072
import static org.junit.Assert.assertThat;
73+
import static org.junit.Assert.fail;
7174
import static org.junit.Assume.assumeThat;
7275

7376
@RunWith(Parameterized.class)
@@ -100,9 +103,15 @@ public class GitHubSCMFileSystemTest {
100103

101104
public static PullRequestSCMRevision prMergeInvalidRevision = new PullRequestSCMRevision(
102105
prMerge,
103-
"INVALIDc3c8284d8c6d5886d473db98f2126071c",
106+
"8f1314fc3c8284d8c6d5886d473db98f2126071c",
104107
"c0e024f89969b976da165eecaa71e09dc60c3da1",
105-
"38814ca33833ff5583624c29f305be9133f27a40");
108+
null);
109+
110+
public static PullRequestSCMRevision prMergeNotMergeableRevision = new PullRequestSCMRevision(
111+
prMerge,
112+
"8f1314fc3c8284d8c6d5886d473db98f2126071c",
113+
"c0e024f89969b976da165eecaa71e09dc60c3da1",
114+
PullRequestSCMRevision.NOT_MERGEABLE_HASH);
106115

107116

108117
@Rule
@@ -284,14 +293,24 @@ public void resolveDirPRMerge() throws Exception {
284293
assertThat(fs.getRoot().child("fu").getType(), is(SCMFile.Type.DIRECTORY));
285294
}
286295

287-
@Test(expected = AbortException.class)
296+
@Test
288297
public void resolveDirPRInvalidMerge() throws Exception {
289298
assumeThat(revision, nullValue());
290299

291300
assertThat(prMergeInvalidRevision.isMerge(), is(true));
292301

293302
SCMFileSystem fs = SCMFileSystem.of(source, prMerge, prMergeInvalidRevision);
294-
assertThat(fs, instanceOf(GitHubSCMFileSystem.class));
303+
assertThat(fs, nullValue());
304+
}
305+
306+
@Test(expected = AbortException.class)
307+
public void resolveDirPRNotMergeable() throws Exception {
308+
assumeThat(revision, nullValue());
309+
310+
assertThat(prMergeNotMergeableRevision.isMerge(), is(true));
311+
312+
SCMFileSystem fs = SCMFileSystem.of(source, prMerge, prMergeNotMergeableRevision);
313+
fail();
295314
}
296315

297316
}

0 commit comments

Comments
 (0)