Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JENKINS-57583 Ignore merge commmit when comparing revisions #227

Merged
merged 3 commits into from
May 23, 2019
Merged

JENKINS-57583 Ignore merge commmit when comparing revisions #227

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
019cb08
[JENKINS-57371] - Enable graceful fallback for merge hash
bitwiseman May 18, 2019
257a628
JENKINS-57583 Ignore merge commmit when comparing revisions
bitwiseman May 22, 2019
226406f
Merge branch 'master' into JENKINS-57583
bitwiseman May 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import hudson.model.Item;
import hudson.plugins.git.GitSCM;
import hudson.scm.SCM;

import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.OutputStream;
import java.nio.charset.StandardCharsets;
Expand Down Expand Up @@ -88,7 +90,6 @@ protected GitHubSCMFileSystem(GitHub gitHub, GHRepository repo, String refName,
PullRequestSCMRevision prRev = (PullRequestSCMRevision) rev;
PullRequestSCMHead pr = (PullRequestSCMHead) prRev.getHead();
if (pr.isMerge()) {
prRev.validateMergeHash(repo);
this.ref = prRev.getMergeHash();
} else {
this.ref = prRev.getPullHash();
Expand Down Expand Up @@ -279,7 +280,17 @@ public SCMFileSystem build(@NonNull SCMSource source, @NonNull SCMHead head, @Ch
refName = "tags/" + head.getName();
} else if (head instanceof PullRequestSCMHead) {
refName = null;
if (rev == null) {
if (rev != null && rev instanceof PullRequestSCMRevision) {
PullRequestSCMRevision prRev = (PullRequestSCMRevision) rev;
if (((PullRequestSCMHead)head).isMerge()) {
if (prRev.getMergeHash() == null) {
// we need to release here as we are not throwing an exception or transferring responsibility to FS
Connector.release(github);
return null;
}
prRev.validateMergeHash();
}
} else {
// we need to release here as we are not throwing an exception or transferring responsibility to FS
Connector.release(github);
return null;
Expand Down
89 changes: 65 additions & 24 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,14 +1081,19 @@ private static void retrievePullRequest(
}
return;
}
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
for (final ChangeRequestCheckoutStrategy strategy : strategies.get(fork)) {
final String branchName;
if (strategies.get(fork).size() == 1) {
branchName = "PR-" + number;
} else {
branchName = "PR-" + number + "-" + strategy.name().toLowerCase(Locale.ENGLISH);
}

// PR details only needed for merge PRs
if (strategy == ChangeRequestCheckoutStrategy.MERGE) {
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
}

if (request.process(new PullRequestSCMHead(
pr, branchName, strategy == ChangeRequestCheckoutStrategy.MERGE
),
Expand Down Expand Up @@ -1251,7 +1256,8 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l
int number = Integer.parseInt(prMatcher.group(1));
listener.getLogger().format("Attempting to resolve %s as pull request %d%n", headName, number);
try {
GHPullRequest pr = getDetailedGHPullRequest(number, listener, github, ghRepository);
Connector.checkApiRateLimit(listener, github);
GHPullRequest pr = ghRepository.getPullRequest(number);
if (pr != null) {
boolean fork = !ghRepository.getOwner().equals(pr.getHead().getUser());
Set<ChangeRequestCheckoutStrategy> strategies;
Expand Down Expand Up @@ -1304,12 +1310,15 @@ protected SCMRevision retrieve(@NonNull String headName, @NonNull TaskListener l
PullRequestSCMHead head = new PullRequestSCMHead(
pr, headName, strategy == ChangeRequestCheckoutStrategy.MERGE
);
if (head.isMerge()) {
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
}
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, head, listener, github, ghRepository);

switch (strategy) {
case MERGE:
try {
prRev.validateMergeHash(ghRepository);
prRev.validateMergeHash();
} catch (AbortException e) {
listener.getLogger().format("Resolved %s as pull request %d: %s.%n%n",
headName,
Expand Down Expand Up @@ -1499,12 +1508,13 @@ protected SCMRevision retrieve(SCMHead head, TaskListener listener) throws IOExc
repositoryUrl = ghRepository.getHtmlUrl();
if (head instanceof PullRequestSCMHead) {
PullRequestSCMHead prhead = (PullRequestSCMHead) head;

GHPullRequest pr = getDetailedGHPullRequest(prhead.getNumber(), listener, github, ghRepository);
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, prhead, listener, github, ghRepository);
Connector.checkApiRateLimit(listener, github);
GHPullRequest pr = ghRepository.getPullRequest(prhead.getNumber());
if (prhead.isMerge()) {
prRev.validateMergeHash(ghRepository);
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
}
PullRequestSCMRevision prRev = createPullRequestSCMRevision(pr, prhead, listener, github, ghRepository);
prRev.validateMergeHash();
return prRev;
} else if (head instanceof GitHubTagSCMHead) {
GitHubTagSCMHead tagHead = (GitHubTagSCMHead) head;
Expand Down Expand Up @@ -1532,26 +1542,57 @@ private static PullRequestSCMRevision createPullRequestSCMRevision(GHPullRequest
String baseHash = pr.getBase().getSha();
String prHeadHash = pr.getHead().getSha();
String mergeHash = null;
switch (prhead.getCheckoutStrategy()) {
case MERGE:
Connector.checkApiRateLimit(listener, github);
baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha();
if (pr.getMergeable() != null) {
mergeHash = Boolean.TRUE.equals(pr.getMergeable()) ? pr.getMergeCommitSha() : PullRequestSCMRevision.NOT_MERGEABLE_HASH;

if (prhead.isMerge()) {
if (Boolean.FALSE.equals(pr.getMergeable())) {
mergeHash = PullRequestSCMRevision.NOT_MERGEABLE_HASH;
} else if (Boolean.TRUE.equals(pr.getMergeable())) {
String proposedMergeHash = pr.getMergeCommitSha();
GHCommit commit = null;
try {
commit = ghRepository.getCommit(proposedMergeHash);
} catch (FileNotFoundException e) {
listener.getLogger().format("Pull request %s : github merge_commit_sha not found (%s). Close and reopen the PR to reset its merge hash.%n",
pr.getNumber(),
proposedMergeHash);
} catch (IOException e) {
throw new AbortException("Error while retrieving pull request " + pr.getNumber() + " merge hash : " + e.toString());
}
break;
default:
break;
}
return new PullRequestSCMRevision(prhead, baseHash, prHeadHash, mergeHash);
}

if (commit != null) {
List<String> parents = commit.getParentSHA1s();
// Merge commits always merge against the most recent base commit they can detect.
if (parents.size() != 2) {
listener.getLogger().format("WARNING: Invalid github merge_commit_sha for pull request %s : merge commit %s with parents - %s.%n",
pr.getNumber(),
proposedMergeHash,
StringUtils.join(parents, "+"));
} else if (!parents.contains(prHeadHash)) {
// This is maintains the existing behavior from pre-2.5.x when the merge_commit_sha is out of sync from the requested prHead
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",
pr.getNumber(),
prHeadHash,
proposedMergeHash,
StringUtils.join(parents, "+"));
} else {
// We found a merge_commit_sha with 2 parents and one matches the prHeadHash
// Use the other parent hash as the base. This keeps the merge hash in sync with head and base.
// It is possible that head or base hash will not exist in their branch by the time we build
// This is be true (and cause a failure) regardless of how we determine the commits.
mergeHash = proposedMergeHash;
baseHash = prHeadHash.equals(parents.get(0)) ? parents.get(1) : parents.get(0);
}
}
}

// Merge PR jobs always merge against the most recent base branch commit they can detect.
// For an invalid merge_commit_sha, we need to query for most recent base commit separately
if (mergeHash == null) {
baseHash = ghRepository.getRef("heads/" + pr.getBase().getRef()).getObject().getSha();
}
}

private static GHPullRequest getDetailedGHPullRequest(int number, TaskListener listener, GitHub github, GHRepository ghRepository) throws IOException, InterruptedException {
Connector.checkApiRateLimit(listener, github);
GHPullRequest pr = ghRepository.getPullRequest(number);
ensureDetailedGHPullRequest(pr, listener, github, ghRepository);
return pr;
return new PullRequestSCMRevision(prhead, baseHash, prHeadHash, mergeHash);
}

private static void ensureDetailedGHPullRequest(GHPullRequest pr, TaskListener listener, GitHub github, GHRepository ghRepository) throws IOException, InterruptedException {
Expand Down
32 changes: 9 additions & 23 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/PullRequestSCMRevision.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,27 +106,9 @@ public String getMergeHash() {
return mergeHash;
}

void validateMergeHash(@NonNull GHRepository repo) throws IOException {
if (!isMerge()) {
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not a merge head");
} else if (this.mergeHash == null) {
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Unknown merge state " + this.toString());
} else if (this.mergeHash == NOT_MERGEABLE_HASH) {
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not mergeable " + this.toString());
} else {
GHCommit commit = null;
try {
commit = repo.getCommit(this.mergeHash);
} catch (FileNotFoundException e) {
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.");
} catch (IOException e) {
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : " + e.toString());
}
assert(commit != null);
List<String> parents = commit.getParentSHA1s();
if (parents.size() != 2 || !parents.contains(this.getBaseHash()) || !parents.contains(this.getPullHash())) {
throw new AbortException("Invalid merge hash for pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Head and base commits do match merge commit " + this.toString() );
}
void validateMergeHash() throws AbortException {
if (this.mergeHash == NOT_MERGEABLE_HASH) {
throw new AbortException("Pull request " + ((PullRequestSCMHead)this.getHead()).getNumber() + " : Not mergeable at " + this.toString());
}
}

Expand All @@ -136,12 +118,16 @@ public boolean equivalent(ChangeRequestSCMRevision<?> o) {
return false;
}
PullRequestSCMRevision other = (PullRequestSCMRevision) o;
return getHead().equals(other.getHead()) && pullHash.equals(other.pullHash) && StringUtils.equals(getMergeHash(), other.getMergeHash());

// JENKINS-57583 - Equivalent is used to make decisiions about when to build.
// mergeHash is an implementation detail of github, generated from base and target
// If only mergeHash changes we do not consider it a different revision
return getHead().equals(other.getHead()) && pullHash.equals(other.pullHash);
}

@Override
public int _hashCode() {
return pullHash.hashCode() + ((mergeHash == null) ? 0 : mergeHash.hashCode());
return pullHash.hashCode();
}

@Override
Expand Down
27 changes: 23 additions & 4 deletions src/test/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMFileSystemTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import jenkins.plugins.git.AbstractGitSCMSource;
import jenkins.plugins.git.GitSCMFileSystem;
import jenkins.scm.api.SCMFile;
import jenkins.scm.api.SCMFileSystem;
import jenkins.scm.api.SCMHead;
Expand All @@ -47,6 +48,7 @@

import org.apache.commons.io.IOUtils;
import org.hamcrest.Matchers;
import org.hamcrest.core.IsNull;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Rule;
Expand All @@ -68,6 +70,7 @@
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.core.IsNull.nullValue;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
import static org.junit.Assume.assumeThat;

@RunWith(Parameterized.class)
Expand Down Expand Up @@ -100,9 +103,15 @@ public class GitHubSCMFileSystemTest {

public static PullRequestSCMRevision prMergeInvalidRevision = new PullRequestSCMRevision(
prMerge,
"INVALIDc3c8284d8c6d5886d473db98f2126071c",
"8f1314fc3c8284d8c6d5886d473db98f2126071c",
"c0e024f89969b976da165eecaa71e09dc60c3da1",
"38814ca33833ff5583624c29f305be9133f27a40");
null);

public static PullRequestSCMRevision prMergeNotMergeableRevision = new PullRequestSCMRevision(
prMerge,
"8f1314fc3c8284d8c6d5886d473db98f2126071c",
"c0e024f89969b976da165eecaa71e09dc60c3da1",
PullRequestSCMRevision.NOT_MERGEABLE_HASH);


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

@Test(expected = AbortException.class)
@Test
public void resolveDirPRInvalidMerge() throws Exception {
assumeThat(revision, nullValue());

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

SCMFileSystem fs = SCMFileSystem.of(source, prMerge, prMergeInvalidRevision);
assertThat(fs, instanceOf(GitHubSCMFileSystem.class));
assertThat(fs, nullValue());
}

@Test(expected = AbortException.class)
public void resolveDirPRNotMergeable() throws Exception {
assumeThat(revision, nullValue());

assertThat(prMergeNotMergeableRevision.isMerge(), is(true));

SCMFileSystem fs = SCMFileSystem.of(source, prMerge, prMergeNotMergeableRevision);
fail();
}

}