From 14e8bb094e517d1c62a58800d4335fa82d453779 Mon Sep 17 00:00:00 2001 From: David Tanner Date: Thu, 2 Jul 2015 11:11:14 -0600 Subject: [PATCH 1/4] Add logging, and fix merge. Added logging and some fixes for the credentials. Fix the null pointers that are happening in pullRequestMerge --- pom.xml | 2 +- .../org/jenkinsci/plugins/ghprb/Ghprb.java | 27 +++++++++-- .../plugins/ghprb/GhprbGitHubAuth.java | 8 +--- .../plugins/ghprb/GhprbPullRequest.java | 48 ++++++++++--------- .../plugins/ghprb/GhprbPullRequestMerge.java | 7 +-- .../jenkinsci/plugins/ghprb/GhprbTrigger.java | 4 ++ 6 files changed, 59 insertions(+), 37 deletions(-) diff --git a/pom.xml b/pom.xml index ed920f005..6835504f2 100644 --- a/pom.xml +++ b/pom.xml @@ -10,7 +10,7 @@ ghprb GitHub Pull Request Builder - 1.25-SNAPSHOT + 1.24.5-SNAPSHOT hpi https://wiki.jenkins-ci.org/display/JENKINS/GitHub+pull+request+builder+plugin diff --git a/src/main/java/org/jenkinsci/plugins/ghprb/Ghprb.java b/src/main/java/org/jenkinsci/plugins/ghprb/Ghprb.java index 0c5af83cf..3c59a98e7 100644 --- a/src/main/java/org/jenkinsci/plugins/ghprb/Ghprb.java +++ b/src/main/java/org/jenkinsci/plugins/ghprb/Ghprb.java @@ -21,6 +21,7 @@ import hudson.model.AbstractProject; import hudson.model.Cause; import hudson.model.Item; +import hudson.model.ItemGroup; import hudson.model.Result; import hudson.model.Saveable; import hudson.model.TaskListener; @@ -47,6 +48,8 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; +import jenkins.model.Jenkins; + /** * @author janinko */ @@ -367,10 +370,26 @@ public static void addIfMissing(DescribableList credentials; + + if (context == null) { + credentials = CredentialsProvider.lookupCredentials(StandardCredentials.class, Jenkins.getInstance(), ACL.SYSTEM, + URIRequirementBuilder.fromUri(uri).build()); + } else { + credentials = CredentialsProvider.lookupCredentials(StandardCredentials.class, context, ACL.SYSTEM, + URIRequirementBuilder.fromUri(uri).build()); + } + + logger.log(Level.FINE, "Found {0} credentials", new Object[]{credentials.size()}); + + return (credentialId == null) ? null : CredentialsMatchers.firstOrNull(credentials, CredentialsMatchers.withId(credentialId)); } diff --git a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbGitHubAuth.java b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbGitHubAuth.java index 6ee5e6d4b..08d0d01e7 100644 --- a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbGitHubAuth.java +++ b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbGitHubAuth.java @@ -4,7 +4,6 @@ import static hudson.Util.fixEmptyAndTrim; import java.io.IOException; -import java.net.URI; import java.net.URISyntaxException; import java.util.ArrayList; import java.util.Arrays; @@ -30,10 +29,6 @@ import com.cloudbees.plugins.credentials.common.StandardListBoxModel; import com.cloudbees.plugins.credentials.common.StandardUsernamePasswordCredentials; import com.cloudbees.plugins.credentials.domains.DomainRequirement; -import com.cloudbees.plugins.credentials.domains.HostnamePortRequirement; -import com.cloudbees.plugins.credentials.domains.HostnameRequirement; -import com.cloudbees.plugins.credentials.domains.PathRequirement; -import com.cloudbees.plugins.credentials.domains.SchemeRequirement; import com.cloudbees.plugins.credentials.domains.URIRequirementBuilder; import com.google.common.base.Joiner; @@ -112,7 +107,8 @@ private static GitHubBuilder getBuilder(Item context, String serverAPIUrl, Strin GitHubBuilder builder = new GitHubBuilder() .withEndpoint(serverAPIUrl) .withConnector(new HttpConnectorWithJenkinsProxy()); - String contextName = context == null ? "(null)" : context.getFullDisplayName(); + String contextName = context == null ? "(Jenkins.instance)" : context.getFullDisplayName(); + if (StringUtils.isEmpty(credentialsId)) { logger.log(Level.WARNING, "credentialsId not set for context {0}, using anonymous connection", contextName); return builder; diff --git a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequest.java b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequest.java index 1ed5b6964..902ad2cd7 100644 --- a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequest.java +++ b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequest.java @@ -148,27 +148,8 @@ public void check(GHPullRequest pr) { source = pr.getHead().getRef(); // If this instance was created before target was introduced (before v1.8), it can be null. } - if (isUpdated(pr)) { - logger.log(Level.INFO, "Pull request #{0} was updated on {1} at {2} by {3}", new Object[] { id, reponame, updated, author }); - - // the title could have been updated since the original PR was opened - title = pr.getTitle(); - int commentsChecked = checkComments(pr); - boolean newCommit = checkCommit(pr.getHead().getSha()); - - if (!newCommit && commentsChecked == 0) { - logger.log(Level.INFO, "Pull request #{0} was updated on repo {1} but there aren''t any new comments nor commits; " - + "that may mean that commit status was updated.", - new Object[] { id, reponame } - ); - } - try { - updated = pr.getUpdatedAt(); - } catch (IOException e) { - e.printStackTrace(); - updated = new Date(); - } - } + updatePR(pr, author); + checkSkipBuild(pr); tryBuild(pr); } @@ -180,21 +161,40 @@ public void check(GHIssueComment comment) { } try { checkComment(comment); - updated = comment.getUpdatedAt(); } catch (IOException ex) { logger.log(Level.SEVERE, "Couldn't check comment #" + comment.getId(), ex); return; } + GHPullRequest pr = null; try { pr = repo.getPullRequest(id); + updatePR(pr, comment.getUser()); } catch (IOException e) { logger.log(Level.SEVERE, "Couldn't get GHPullRequest for checking mergeable state"); } checkSkipBuild(comment.getParent()); tryBuild(pr); } + + private void updatePR(GHPullRequest pr, GHUser author) { + if (isUpdated(pr)) { + logger.log(Level.INFO, "Pull request #{0} was updated on {1} at {2} by {3}", new Object[] { id, reponame, updated, author }); + + // the title could have been updated since the original PR was opened + title = pr.getTitle(); + int commentsChecked = checkComments(pr); + boolean newCommit = checkCommit(pr.getHead().getSha()); + + if (!newCommit && commentsChecked == 0) { + logger.log(Level.INFO, "Pull request #{0} was updated on repo {1} but there aren''t any new comments nor commits; " + + "that may mean that commit status was updated.", + new Object[] { id, reponame } + ); + } + } + } public boolean isWhiteListedTargetBranch() { List branches = helper.getWhiteListTargetBranches(); @@ -216,12 +216,14 @@ public boolean isWhiteListedTargetBranch() { private boolean isUpdated(GHPullRequest pr) { Date lastUpdated = new Date(); + boolean ret = false; try { lastUpdated = pr.getUpdatedAt(); + ret = updated.compareTo(lastUpdated) < 0; + updated = lastUpdated; } catch (IOException e) { e.printStackTrace(); } - boolean ret = updated.compareTo(lastUpdated) < 0; ret = ret || !pr.getHead().getSha().equals(head); return ret; } diff --git a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequestMerge.java b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequestMerge.java index eddea0358..6a76b71ed 100644 --- a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequestMerge.java +++ b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequestMerge.java @@ -130,21 +130,22 @@ public boolean perform(AbstractBuild build, Launcher launcher, final Build } boolean merge = true; + String commentBody = cause.getCommentBody(); - if (isOnlyAdminsMerge() && !helper.isAdmin(triggerSender)) { + if (isOnlyAdminsMerge() && (triggerSender == null || !helper.isAdmin(triggerSender) )) { merge = false; logger.println("Only admins can merge this pull request, " + triggerSender.getLogin() + " is not an admin."); commentOnRequest(String.format("Code not merged because %s is not in the Admin list.", triggerSender.getName())); } - if (isOnlyTriggerPhrase() && !helper.isTriggerPhrase(cause.getCommentBody())) { + if (isOnlyTriggerPhrase() && (commentBody == null || !helper.isTriggerPhrase(cause.getCommentBody()) )) { merge = false; logger.println("The comment does not contain the required trigger phrase."); commentOnRequest(String.format("Please comment with '%s' to automerge this request", trigger.getTriggerPhrase())); } - if (isDisallowOwnCode() && isOwnCode(pr, triggerSender)) { + if (isDisallowOwnCode() && (triggerSender == null || isOwnCode(pr, triggerSender) )) { merge = false; logger.println("The commentor is also one of the contributors."); commentOnRequest(String.format("Code not merged because %s has committed code in the request.", triggerSender.getName())); diff --git a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbTrigger.java b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbTrigger.java index 7d430ccf2..a008cd2e0 100644 --- a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbTrigger.java +++ b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbTrigger.java @@ -267,6 +267,10 @@ public GitHub getGitHub() throws IOException { } public AbstractProject getActualProject() { + + if (_project != null) { + return _project; + } @SuppressWarnings("rawtypes") List projects = Jenkins.getInstance().getAllItems(AbstractProject.class); From 582677a096b2cd5a8cee7f5691f77ba363e96877 Mon Sep 17 00:00:00 2001 From: David Tanner Date: Thu, 2 Jul 2015 17:49:53 -0600 Subject: [PATCH 2/4] Update tests --- .../java/org/jenkinsci/plugins/ghprb/GhprbRepositoryTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/ghprb/GhprbRepositoryTest.java b/src/test/java/org/jenkinsci/plugins/ghprb/GhprbRepositoryTest.java index d9238fd3b..8cc632c4b 100644 --- a/src/test/java/org/jenkinsci/plugins/ghprb/GhprbRepositoryTest.java +++ b/src/test/java/org/jenkinsci/plugins/ghprb/GhprbRepositoryTest.java @@ -310,7 +310,7 @@ public void testCheckMethodWhenPrWasUpdatedWithNonKeyPhrase() throws IOException verify(ghPullRequest, times(3)).getBase(); verify(ghPullRequest, times(5)).getNumber(); verify(ghPullRequest, times(1)).getHtmlUrl(); - verify(ghPullRequest, times(4)).getUpdatedAt(); + verify(ghPullRequest, times(3)).getUpdatedAt(); verify(ghPullRequest, times(1)).getComments(); verify(ghPullRequest, times(1)).listCommits(); @@ -390,7 +390,7 @@ public void testCheckMethodWhenPrWasUpdatedWithRetestPhrase() throws IOException verify(ghPullRequest, times(8)).getHead(); verify(ghPullRequest, times(3)).getBase(); verify(ghPullRequest, times(5)).getNumber(); - verify(ghPullRequest, times(4)).getUpdatedAt(); + verify(ghPullRequest, times(3)).getUpdatedAt(); verify(ghPullRequest, times(1)).getHtmlUrl(); verify(ghPullRequest, times(1)).getComments(); From fd6f681df110cf4be6b91e70dfa13083c7de9395 Mon Sep 17 00:00:00 2001 From: David Tanner Date: Thu, 2 Jul 2015 21:59:15 -0600 Subject: [PATCH 3/4] Fix possible null pointer with pr when checking comments --- .../org/jenkinsci/plugins/ghprb/GhprbPullRequest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequest.java b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequest.java index 902ad2cd7..60a9bdeb0 100644 --- a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequest.java +++ b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbPullRequest.java @@ -179,7 +179,7 @@ public void check(GHIssueComment comment) { } private void updatePR(GHPullRequest pr, GHUser author) { - if (isUpdated(pr)) { + if (pr != null && isUpdated(pr)) { logger.log(Level.INFO, "Pull request #{0} was updated on {1} at {2} by {3}", new Object[] { id, reponame, updated, author }); // the title could have been updated since the original PR was opened @@ -215,14 +215,17 @@ public boolean isWhiteListedTargetBranch() { } private boolean isUpdated(GHPullRequest pr) { + if (pr == null) { + return false; + } Date lastUpdated = new Date(); boolean ret = false; try { lastUpdated = pr.getUpdatedAt(); ret = updated.compareTo(lastUpdated) < 0; updated = lastUpdated; - } catch (IOException e) { - e.printStackTrace(); + } catch (Exception e) { + logger.log(Level.WARNING, "Unable to update last updated date", e); } ret = ret || !pr.getHead().getSha().equals(head); return ret; From 47d9032386d87ce13f5f754da0b36b90e643e563 Mon Sep 17 00:00:00 2001 From: David Tanner Date: Thu, 2 Jul 2015 22:23:56 -0600 Subject: [PATCH 4/4] Change version back --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 6835504f2..ed920f005 100644 --- a/pom.xml +++ b/pom.xml @@ -10,7 +10,7 @@ ghprb GitHub Pull Request Builder - 1.24.5-SNAPSHOT + 1.25-SNAPSHOT hpi https://wiki.jenkins-ci.org/display/JENKINS/GitHub+pull+request+builder+plugin