Skip to content

[JENKINS-33161] Customized PR build behavior #60

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

Merged
jglick merged 24 commits into from
Jul 5, 2016
Merged

[JENKINS-33161] Customized PR build behavior #60

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
996cd93
[JENKINS-33161] Draft of support for customized PR build behavior.
jglick Jun 16, 2016
ce5f941
Oops, forgot to record entries in originBranchesWithPR.
jglick Jun 16, 2016
63a6006
Warning about another anomalous combination.
jglick Jun 16, 2016
ed48654
Fixing a few warnings and a test failure.
jglick Jun 16, 2016
6ce16b1
We are converting this anyway, can strip out more implementation.
jglick Jun 16, 2016
870125b
Comments.
jglick Jun 16, 2016
32a5de3
We need to be sure to feed SpecificRevisionBuildChooser the PR head h…
jglick Jun 16, 2016
7dc37d0
Revert "We are converting this anyway, can strip out more implementat…
jglick Jun 16, 2016
56f47c5
typo
jglick Jun 16, 2016
413f0b0
Noting when job names will distinguish kinds of PRs.
jglick Jun 20, 2016
5a98d2a
Specify a notification per job, so that we can distinctly see merged …
jglick Jun 20, 2016
cdfe0b3
Override toString for debugging.
jglick Jun 20, 2016
7353a02
Building merged PRs seems to work now.
jglick Jun 20, 2016
bff827f
@KostyaSha’s suggestion of *-head vs. *-merge job naming seems as goo…
jglick Jun 21, 2016
d067c0c
The getters and setters were intended to be of type boolean, not Bool…
jglick Jun 21, 2016
e003af9
Did my IDE just go nuts?
jglick Jun 21, 2016
c1feec5
Indentation fix.
jglick Jun 21, 2016
e889802
Typo.
jglick Jun 21, 2016
f7e5157
Improved form validation at @amuniz’s suggestion.
jglick Jun 21, 2016
57fa572
I18N and help.
jglick Jun 21, 2016
71d72d8
Copy-paste support for GitHubSCMNavigator, pending a cleaner way of s…
jglick Jun 21, 2016
67ccb38
Tweak to indexing log; as @amuniz pointed out, it is otherwise hard t…
jglick Jun 21, 2016
2e6048a
Need to set author & committer prior to merging, in case the master h…
jglick Jun 21, 2016
16e36f7
Added run-snapshot target.
jglick Jun 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 14 additions & 39 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubBuildStatusNotification.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,8 @@
import jenkins.scm.api.SCMRevisionAction;
import jenkins.scm.api.SCMSource;
import jenkins.scm.api.SCMSourceOwner;
import org.kohsuke.github.GHCommit;
import org.kohsuke.github.GHCommitState;
import org.kohsuke.github.GHPullRequest;
import org.kohsuke.github.GHPullRequestCommitDetail;
import org.kohsuke.github.GHRepository;
import org.kohsuke.github.GitHub;

Expand All @@ -56,7 +54,6 @@
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

Expand All @@ -73,9 +70,9 @@ public class GitHubBuildStatusNotification {

private static final Logger LOGGER = Logger.getLogger(GitHubBuildStatusNotification.class.getName());

private static void createCommitStatus(@Nonnull GHRepository repo, @Nonnull String revision, @Nonnull GHCommitState state, @Nonnull String url, @Nonnull String message) throws IOException {
private static void createCommitStatus(@Nonnull GHRepository repo, @Nonnull String revision, @Nonnull GHCommitState state, @Nonnull String url, @Nonnull String message, @Nonnull Job<?,?> job) throws IOException {
LOGGER.log(Level.FINE, "{0}/commit/{1} {2} from {3}", new Object[] {repo.getHtmlUrl(), revision, state, url});
repo.createCommitStatus(revision, state, url, message, "Jenkins");
repo.createCommitStatus(revision, state, url, message, "Jenkins job " + job.getName());
Copy link

@kevin-brown kevin-brown Jul 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes issues right now as it's not possible to require Jenkins status checks to pass anymore without GitHub. It relies on the name being consistent across checks, which isn't possible when the check name is always changing.

Copy link

@kevin-brown kevin-brown Jul 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this issue has already been filed as JENKINS-36574.

}

@SuppressWarnings("deprecation") // Run.getAbsoluteUrl appropriate here
Expand All @@ -96,17 +93,18 @@ private static void createBuildCommitStatus(Run<?,?> build, TaskListener listene
try {
Result result = build.getResult();
String revisionToNotify = resolveHeadCommit(repo, revision);
Job<?,?> job = build.getParent();
if (Result.SUCCESS.equals(result)) {
createCommitStatus(repo, revisionToNotify, GHCommitState.SUCCESS, url, Messages.GitHubBuildStatusNotification_CommitStatus_Good());
createCommitStatus(repo, revisionToNotify, GHCommitState.SUCCESS, url, Messages.GitHubBuildStatusNotification_CommitStatus_Good(), job);
} else if (Result.UNSTABLE.equals(result)) {
createCommitStatus(repo, revisionToNotify, GHCommitState.FAILURE, url, Messages.GitHubBuildStatusNotification_CommitStatus_Unstable());
createCommitStatus(repo, revisionToNotify, GHCommitState.FAILURE, url, Messages.GitHubBuildStatusNotification_CommitStatus_Unstable(), job);
} else if (Result.FAILURE.equals(result)) {
createCommitStatus(repo, revisionToNotify, GHCommitState.FAILURE, url, Messages.GitHubBuildStatusNotification_CommitStatus_Failure());
createCommitStatus(repo, revisionToNotify, GHCommitState.FAILURE, url, Messages.GitHubBuildStatusNotification_CommitStatus_Failure(), job);
} else if (result != null) { // ABORTED etc.
createCommitStatus(repo, revisionToNotify, GHCommitState.ERROR, url, Messages.GitHubBuildStatusNotification_CommitStatus_Other());
createCommitStatus(repo, revisionToNotify, GHCommitState.ERROR, url, Messages.GitHubBuildStatusNotification_CommitStatus_Other(), job);
} else {
ignoreError = true;
createCommitStatus(repo, revisionToNotify, GHCommitState.PENDING, url, Messages.GitHubBuildStatusNotification_CommitStatus_Pending());
createCommitStatus(repo, revisionToNotify, GHCommitState.PENDING, url, Messages.GitHubBuildStatusNotification_CommitStatus_Pending(), job);
}
if (result != null) {
listener.getLogger().format("%n" + Messages.GitHubBuildStatusNotification_CommitStatusSet() + "%n%n");
Expand Down Expand Up @@ -198,7 +196,7 @@ private static GitHubSCMSource getSCMSource(final SCMSourceOwner scmSourceOwner)
}
// Has not been built yet, so we can only guess that the current PR head is what will be built.
// In fact the submitter might push another commit before this build even starts.
createCommitStatus(repo, pr.getHead().getSha(), GHCommitState.PENDING, url, "This pull request is scheduled to be built");
createCommitStatus(repo, pr.getHead().getSha(), GHCommitState.PENDING, url, "This pull request is scheduled to be built", job);
}
} catch (FileNotFoundException fnfe) {
LOGGER.log(Level.WARNING, "Could not update commit status to PENDING. Valid scan credentials? Valid scopes?");
Expand Down Expand Up @@ -239,35 +237,12 @@ private static GitHubSCMSource getSCMSource(final SCMSourceOwner scmSourceOwner)

private static String resolveHeadCommit(GHRepository repo, SCMRevision revision) throws IllegalArgumentException {
if (revision instanceof SCMRevisionImpl) {
SCMRevisionImpl rev = (SCMRevisionImpl) revision;
try {
GHCommit commit = repo.getCommit(rev.getHash());
List<GHCommit> parents = commit.getParents();
// if revision has two parent commits, we have really a MergeCommit
if (parents.size() == 2) {
SCMHead head = revision.getHead();
// MergeCommit is coming from a pull request
if (head instanceof PullRequestSCMHead) {
GHPullRequest pullRequest = repo.getPullRequest(((PullRequestSCMHead) head).getNumber());
for (GHPullRequestCommitDetail commitDetail : pullRequest.listCommits()) {
if (commitDetail.getSha().equals(parents.get(0).getSHA1())) {
// Parent commit (HeadCommit) found in PR commit list
return parents.get(0).getSHA1();
}
}
// First parent commit not found in PR commit list, so returning the second one.
return parents.get(1).getSHA1();
} else {
return rev.getHash();
}
}
return rev.getHash();
} catch (IOException e) {
LOGGER.log(Level.WARNING, null, e);
throw new IllegalArgumentException(e);
}
return ((SCMRevisionImpl) revision).getHash();
} else if (revision instanceof PullRequestSCMRevision) {
return ((PullRequestSCMRevision) revision).getPullHash();
} else {
throw new IllegalArgumentException("did not recognize " + revision);
Copy link
Member Author

@jglick jglick Jun 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By keeping a reasonable representation of the SCMRevisions to begin with, we avoid the need to try to reconstruct what we just built.

}
throw new IllegalArgumentException();
}

private GitHubBuildStatusNotification() {}
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMNavigator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ public class GitHubSCMNavigator extends SCMNavigator {

@CheckForNull private String includes;
@CheckForNull private String excludes;
// TODO buildXXX to match GitHubSCMSource

@DataBoundConstructor public GitHubSCMNavigator(String apiUri, String repoOwner, String scanCredentialsId, String checkoutCredentialsId) {
this.repoOwner = repoOwner;
Expand Down
Loading