From 4c0ffd05e0936e4f8435cbeaa20ab687847805ec Mon Sep 17 00:00:00 2001 From: Dominic Evans Date: Wed, 25 May 2022 14:04:22 +0100 Subject: [PATCH 1/4] fix: don't trigger poll unless target ref matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing webhook handling code would trigger the GitSCM poll for the default branch regardless of whether or not the target ref of the webhook push event matched the branch specifier of the Job definition. This is problematic and often leads to duplicate builds for the same branch+revision if pushes to other branches happened to occur whilst build(s) were already in progress for that branch. This is because the GitSCM trigger just compares the HEAD revision of any branch matching the branch specifier against the last *completed* build of that branch (ignoring in-progress builds) and schedules a new build for it. The code for this PR is mostly a rebased version of the original changes proposed by @aserrallerios under https://github.com/jenkinsci/github-plugin/pull/44 with some small refinements. Co-authored-by: Albert Serrallé Ríos --- .../com/cloudbees/jenkins/GitHubBranch.java | 59 +++++++++++++++++ .../cloudbees/jenkins/GitHubPushTrigger.java | 8 +++ .../GitHubRepositoryNameContributor.java | 63 +++++++++++++++++++ .../com/cloudbees/jenkins/GitHubTrigger.java | 29 ++++++++- .../plugins/github/migration/Migrator.java | 3 + .../DefaultPushGHEventSubscriber.java | 55 ++++++++++------ .../cloudbees/jenkins/GitHubBranchTest.java | 42 +++++++++++++ .../DefaultPushGHEventListenerTest.java | 28 +++++++-- 8 files changed, 264 insertions(+), 23 deletions(-) create mode 100644 src/main/java/com/cloudbees/jenkins/GitHubBranch.java create mode 100644 src/test/java/com/cloudbees/jenkins/GitHubBranchTest.java diff --git a/src/main/java/com/cloudbees/jenkins/GitHubBranch.java b/src/main/java/com/cloudbees/jenkins/GitHubBranch.java new file mode 100644 index 000000000..029f5d983 --- /dev/null +++ b/src/main/java/com/cloudbees/jenkins/GitHubBranch.java @@ -0,0 +1,59 @@ +package com.cloudbees.jenkins; + +import hudson.plugins.git.BranchSpec; + +import java.util.logging.Level; +import java.util.logging.Logger; + +/** + * Identifies a GitHub branch and hides the SCM branch from its clients. + */ +public class GitHubBranch { + + private static final String GITHUB_BRANCH_PREFIX = "refs/heads"; + private static final int GITHUB_BRANCH_PREFIX_LENGTH = 10; + + private final BranchSpec branch; + + private GitHubBranch(final BranchSpec branch) { + this.branch = branch; + } + + public boolean matches(GitHubRepositoryName repository, String ref) { + // ref (github) -> refs/heads/master + // branch (git SCM) -> REPO/remote/branch + // matching the meaningful part of the github branch name with + // the configured SCM branch expression + if (ref != null && !ref.equals("")) { + String branchExpression = "*"; + if (repository != null && repository.repositoryName != null) { + branchExpression = repository.repositoryName; + } + if (ref.startsWith(GITHUB_BRANCH_PREFIX)) { + branchExpression += ref.substring(GITHUB_BRANCH_PREFIX_LENGTH); + } else { + branchExpression += "/" + ref; + } + LOGGER.log(Level.FINE, "Does SCM branch " + branch.getName() + + " match GitHub branch " + branchExpression + "?"); + return branch.matches(branchExpression); + } + return false; + } + + public static GitHubBranch create(BranchSpec branch) { + if (isValidGitHubBranch(branch)) { + return new GitHubBranch(branch); + } else { + return null; + } + } + + // TODO: implement logic to validate git SCM branches. + private static boolean isValidGitHubBranch(BranchSpec branch) { + return true; + } + + private static final Logger LOGGER = Logger.getLogger(GitHubBranch.class + .getName()); +} diff --git a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java index 4cae5f049..900f22b3c 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java @@ -195,6 +195,14 @@ public Set getGitHubRepositories() { return Collections.emptySet(); } + /** + * @deprecated + * Use {@link GitHubRepositoryNameContributor#parseAssociatedBranches(AbstractProject)} + */ + public Set getGitHubBranches() { + return Collections.emptySet(); + } + @Override public void start(Job project, boolean newInstance) { super.start(project, newInstance); diff --git a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java index 572a77631..20d75d309 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java @@ -10,6 +10,7 @@ import hudson.model.Item; import hudson.model.Job; import hudson.model.TaskListener; +import hudson.plugins.git.BranchSpec; import hudson.plugins.git.GitSCM; import hudson.scm.SCM; import jenkins.model.Jenkins; @@ -90,6 +91,38 @@ public void parseAssociatedNames(AbstractProject job, Collection result) { + if (Util.isOverridden( + GitHubRepositoryNameContributor.class, + getClass(), + "parseAssociatedBranches", + Job.class, + Collection.class + )) { + // if this impl is legacy, it cannot contribute to non-jobs, so not an error + if (item instanceof Job) { + parseAssociatedBranches((Job) item, result); + } + } else if (Util.isOverridden( + GitHubRepositoryNameContributor.class, + getClass(), + "parseAssociatedBranches", + AbstractProject.class, + Collection.class + )) { + // if this impl is legacy, it cannot contribute to non-projects, so not an error + if (item instanceof AbstractProject) { + parseAssociatedBranches((AbstractProject) item, result); + } + } else { + throw new AbstractMethodError("you must override the new overload of parseAssociatedBranches"); + } + }; + public static ExtensionList all() { return Jenkins.getInstance().getExtensionList(GitHubRepositoryNameContributor.class); } @@ -118,6 +151,14 @@ public static Collection parseAssociatedNames(Item item) { return names; } + public static Collection parseAssociatedBranches(Item item) { + Set names = new HashSet(); + for (GitHubRepositoryNameContributor c : all()) { + c.parseAssociatedBranches(item, names); + } + return names; + } + /** * Default implementation that looks at SCMs */ @@ -134,6 +175,16 @@ public void parseAssociatedNames(Item item, Collection res } } + @Override + public void parseAssociatedBranches(Item item, Collection result) { + SCMTriggerItem triggerItem = SCMTriggerItems.asSCMTriggerItem(item); + if (triggerItem != null) { + for (SCM scm : triggerItem.getSCMs()) { + addBranches(scm, result); + } + } + } + protected EnvVars buildEnv(Job job) { EnvVars env = new EnvVars(); for (EnvironmentContributor contributor : EnvironmentContributor.all()) { @@ -160,5 +211,17 @@ protected static void addRepositories(SCM scm, EnvVars env, Collection r) { + if (scm instanceof GitSCM) { + GitSCM git = (GitSCM) scm; + for (BranchSpec branch : git.getBranches()) { + GitHubBranch gitHubBranch = GitHubBranch.create(branch); + if (gitHubBranch != null) { + r.add(gitHubBranch); + } + } + } + } } } diff --git a/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java b/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java index 9d44eb838..7fd00b178 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java @@ -40,9 +40,26 @@ public interface GitHubTrigger { */ Set getGitHubRepositories(); + /** + * Obtains the list of the branches that this trigger is looking at. + * + * If the implementation of this class maintain its own list of GitHub branches, it should + * continue to implement this method for backward compatibility, and it gets picked up by + * {@link GitHubRepositoryNameContributor#parseAssociatedBranches(AbstractProject)}. + * + *

+ * Alternatively, if the implementation doesn't worry about the backward compatibility, it can + * implement this method to return an empty collection, then just implement {@link GitHubRepositoryNameContributor}. + * + * @deprecated + * Call {@link GitHubRepositoryNameContributor#parseAssociatedBranches(AbstractProject)} instead. + */ + Set getGitHubBranches(); + /** * Contributes {@link GitHubRepositoryName} from {@link GitHubTrigger#getGitHubRepositories()} - * for backward compatibility + * and {@link GitHubBranch} from {@link GitHubTrigger#getGitHubBranches()} for + * backward compatibility. */ @Extension class GitHubRepositoryNameContributorImpl extends GitHubRepositoryNameContributor { @@ -56,5 +73,15 @@ public void parseAssociatedNames(Item item, Collection res } } } + + @Override + public void parseAssociatedBranches(Item item, Collection result) { + if (item instanceof ParameterizedJobMixIn.ParameterizedJob) { + ParameterizedJobMixIn.ParameterizedJob p = (ParameterizedJobMixIn.ParameterizedJob) item; + for (GitHubTrigger ght : Util.filter(p.getTriggers().values(), GitHubTrigger.class)) { + result.addAll(ght.getGitHubBranches()); + } + } + } } } diff --git a/src/main/java/org/jenkinsci/plugins/github/migration/Migrator.java b/src/main/java/org/jenkinsci/plugins/github/migration/Migrator.java index 9ed3ca0da..2cd9e1c1e 100644 --- a/src/main/java/org/jenkinsci/plugins/github/migration/Migrator.java +++ b/src/main/java/org/jenkinsci/plugins/github/migration/Migrator.java @@ -38,6 +38,9 @@ public class Migrator { public void migrate() throws IOException { LOGGER.debug("Check if GitHub Plugin needs config migration"); GitHubPushTrigger.DescriptorImpl descriptor = GitHubPushTrigger.DescriptorImpl.get(); + if (descriptor == null) { + return; + } descriptor.load(); if (isNotEmpty(descriptor.getCredentials())) { diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java index 7568af0e9..c638b302a 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java @@ -1,5 +1,6 @@ package org.jenkinsci.plugins.github.webhook.subscriber; +import com.cloudbees.jenkins.GitHubBranch; import com.cloudbees.jenkins.GitHubPushTrigger; import com.cloudbees.jenkins.GitHubRepositoryName; import com.cloudbees.jenkins.GitHubRepositoryNameContributor; @@ -75,6 +76,7 @@ protected void onEvent(final GHSubscriberEvent event) { } URL repoUrl = push.getRepository().getUrl(); final String pusherName = push.getPusher().getName(); + final String ref = push.getRef(); LOGGER.info("Received PushEvent for {} from {}", repoUrl, event.getOrigin()); GitHubRepositoryName fromEventRepository = GitHubRepositoryName.create(repoUrl.toExternalForm()); @@ -97,31 +99,48 @@ protected void onEvent(final GHSubscriberEvent event) { // run in high privilege to see all the projects anonymous users don't see. // this is safe because when we actually schedule a build, it's a build that can // happen at some random time anyway. - ACL.impersonate(ACL.SYSTEM, new Runnable() { + Runnable body = new Runnable() { @Override public void run() { - for (Item job : Jenkins.getInstance().getAllItems(Item.class)) { + Jenkins j = Jenkins.getInstanceOrNull(); + if (j == null) { + return; + } + + for (Item job : j.getAllItems(Item.class)) { GitHubPushTrigger trigger = triggerFrom(job, GitHubPushTrigger.class); - if (trigger != null) { - String fullDisplayName = job.getFullDisplayName(); - LOGGER.debug("Considering to poke {}", fullDisplayName); - if (GitHubRepositoryNameContributor.parseAssociatedNames(job) - .contains(changedRepository)) { - LOGGER.info("Poked {}", fullDisplayName); - trigger.onPost(GitHubTriggerEvent.create() - .withTimestamp(event.getTimestamp()) - .withOrigin(event.getOrigin()) - .withTriggeredByUser(pusherName) - .build() - ); - } else { - LOGGER.debug("Skipped {} because it doesn't have a matching repository.", - fullDisplayName); + if (trigger == null) { + continue; + } + String fullDisplayName = job.getFullDisplayName(); + LOGGER.debug("Considering to poke {}", fullDisplayName); + if (!GitHubRepositoryNameContributor.parseAssociatedNames(job).contains(changedRepository)) { + LOGGER.debug("Skipped {} because it doesn't have a matching repository.", fullDisplayName); + continue; + } + boolean foundBranch = false; + for (GitHubBranch branch : GitHubRepositoryNameContributor.parseAssociatedBranches(job)) { + if (branch.matches(changedRepository, ref)) { + foundBranch = true; + break; } } + if (!foundBranch) { + LOGGER.debug("Skipped {} because it doesn't have a matching branch specifier.", + job.getFullDisplayName()); + continue; + } + LOGGER.info("Poked {}", fullDisplayName); + trigger.onPost(GitHubTriggerEvent.create() + .withTimestamp(event.getTimestamp()) + .withOrigin(event.getOrigin()) + .withTriggeredByUser(pusherName) + .build() + ); } } - }); + }; + ACL.impersonate(ACL.SYSTEM, body); for (GitHubWebHook.Listener listener : ExtensionList.lookup(GitHubWebHook.Listener.class)) { listener.onPushRepositoryChanged(pusherName, changedRepository); diff --git a/src/test/java/com/cloudbees/jenkins/GitHubBranchTest.java b/src/test/java/com/cloudbees/jenkins/GitHubBranchTest.java new file mode 100644 index 000000000..b78431a9a --- /dev/null +++ b/src/test/java/com/cloudbees/jenkins/GitHubBranchTest.java @@ -0,0 +1,42 @@ +package com.cloudbees.jenkins; + +import hudson.plugins.git.BranchSpec; +import org.junit.Assert; + +import org.junit.Test; + +public class GitHubBranchTest { + + @Test + public void testBranchMatch() throws Exception, InterruptedException { + GitHubRepositoryName repo = GitHubRepositoryName.create("github-repo"); + + BranchSpec scmBranch = new BranchSpec("*/github-branch"); + GitHubBranch branch = GitHubBranch.create(scmBranch); + + Assert.assertTrue(branch.matches(repo, "refs/heads/github-branch")); + Assert.assertTrue(branch.matches(repo, "github-branch")); + } + + @Test + public void testBranchDoesNotMatch() throws Exception, InterruptedException { + GitHubRepositoryName repo = GitHubRepositoryName.create("github-repo"); + + BranchSpec scmBranch = new BranchSpec("*/github-branch"); + GitHubBranch branch = GitHubBranch.create(scmBranch); + + Assert.assertFalse(branch.matches(repo, "refs/heads/github-branch-2")); + Assert.assertFalse(branch.matches(repo, "github-branch-2")); + } + + @Test + public void testRepoDoesNotMatch() throws Exception, InterruptedException { + GitHubRepositoryName repo = GitHubRepositoryName.create("github-repo"); + + BranchSpec scmBranch = new BranchSpec("github-repo-2/github-branch"); + GitHubBranch branch = GitHubBranch.create(scmBranch); + + Assert.assertFalse(branch.matches(repo, "refs/heads/github-branch")); + Assert.assertFalse(branch.matches(repo, "github-branch")); + } +} diff --git a/src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java b/src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java index b83e762f7..5e79cf702 100644 --- a/src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java @@ -3,7 +3,10 @@ import com.cloudbees.jenkins.GitHubPushTrigger; import com.cloudbees.jenkins.GitHubRepositoryNameContributor; import com.cloudbees.jenkins.GitHubTriggerEvent; +import com.cloudbees.jenkins.GitHubWebHook; + import hudson.ExtensionList; +import hudson.model.EnvironmentContributor; import hudson.model.FreeStyleProject; import hudson.model.Item; import hudson.plugins.git.GitSCM; @@ -75,14 +78,31 @@ public void shouldParsePushPayload() { Jenkins jenkins = mock(Jenkins.class); when(jenkins.getAllItems(Item.class)).thenReturn(Collections.singletonList(prj)); - ExtensionList extensionList = mock(ExtensionList.class); - List gitHubRepositoryNameContributorList = + { + ExtensionList extensionList = mock(ExtensionList.class); + List gitHubRepositoryNameContributorList = Collections.singletonList(new GitHubRepositoryNameContributor.FromSCM()); - when(extensionList.iterator()).thenReturn(gitHubRepositoryNameContributorList.iterator()); - when(jenkins.getExtensionList(GitHubRepositoryNameContributor.class)).thenReturn(extensionList); + when(extensionList.iterator()).thenReturn(gitHubRepositoryNameContributorList.iterator()); + when(jenkins.getExtensionList(GitHubRepositoryNameContributor.class)).thenReturn(extensionList); + } + + { + ExtensionList extensionList = mock(ExtensionList.class); + List environmentContributorList = Collections.emptyList(); + when(extensionList.iterator()).thenReturn(environmentContributorList.iterator()); + when(jenkins.getExtensionList(EnvironmentContributor.class)).thenReturn(extensionList); + } + + { + ExtensionList extensionList = mock(ExtensionList.class); + List listenerList = Collections.emptyList(); + when(extensionList.iterator()).thenReturn(listenerList.iterator()); + when(jenkins.getExtensionList(GitHubWebHook.Listener.class)).thenReturn(extensionList); + } try (MockedStatic mockedJenkins = mockStatic(Jenkins.class)) { mockedJenkins.when(Jenkins::getInstance).thenReturn(jenkins); + mockedJenkins.when(Jenkins::getInstanceOrNull).thenReturn(jenkins); new DefaultPushGHEventSubscriber().onEvent(subscriberEvent); } From 3e6a0f6339c3eadc205a21e80f4e6697b98d6db2 Mon Sep 17 00:00:00 2001 From: Dominic Evans Date: Wed, 25 May 2022 21:41:13 +0100 Subject: [PATCH 2/4] fix: include target ref in GitHubPushCause --- .../cloudbees/jenkins/GitHubPushCause.java | 23 +++++--- .../cloudbees/jenkins/GitHubPushTrigger.java | 8 +-- .../GitHubRepositoryNameContributor.java | 18 ++++++- .../com/cloudbees/jenkins/GitHubTrigger.java | 2 +- .../cloudbees/jenkins/GitHubTriggerEvent.java | 27 ++++++++-- .../DefaultPushGHEventSubscriber.java | 52 ++++++++++++------- .../jenkins/GitHubPushTriggerTest.java | 3 +- .../DefaultPushGHEventListenerTest.java | 3 ++ 8 files changed, 101 insertions(+), 35 deletions(-) diff --git a/src/main/java/com/cloudbees/jenkins/GitHubPushCause.java b/src/main/java/com/cloudbees/jenkins/GitHubPushCause.java index 1604e5e87..683e0cfce 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubPushCause.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubPushCause.java @@ -19,30 +19,37 @@ public class GitHubPushCause extends SCMTriggerCause { * The name of the user who pushed to GitHub. */ private String pushedBy; + /** + * The target ref of the triggering GitHub push. + */ + private String ref; - public GitHubPushCause(String pusher) { - this("", pusher); + public GitHubPushCause(String pusher, String ref) { + this("", pusher, ref); } - public GitHubPushCause(String pollingLog, String pusher) { + public GitHubPushCause(String pollingLog, String pusher, String ref) { super(pollingLog); - pushedBy = pusher; + this.pushedBy = pusher; + this.ref = ref; } - public GitHubPushCause(File pollingLog, String pusher) throws IOException { + public GitHubPushCause(File pollingLog, String pusher, String ref) throws IOException { super(pollingLog); - pushedBy = pusher; + this.pushedBy = pusher; + this.ref = ref; } @Override public String getShortDescription() { - return format("Started by GitHub push by %s", trimToEmpty(pushedBy)); + return format("Started by GitHub push to %s by %s", trimToEmpty(ref), trimToEmpty(pushedBy)); } @Override public boolean equals(Object o) { return o instanceof GitHubPushCause && Objects.equals(this.pushedBy, ((GitHubPushCause) o).pushedBy) + && Objects.equals(this.ref, ((GitHubPushCause) o).ref) && super.equals(o); } @@ -50,7 +57,7 @@ public boolean equals(Object o) { public int hashCode() { int hash = super.hashCode(); hash = 89 * hash + Objects.hash(this.pushedBy); + hash = 89 * hash + Objects.hash(this.ref); return hash; } } - diff --git a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java index 900f22b3c..7e1fd2c97 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java @@ -84,10 +84,11 @@ public void onPost() { /** * Called when a POST is made. */ - public void onPost(String triggeredByUser) { + public void onPost(String triggeredByUser, String triggeredByRef) { onPost(GitHubTriggerEvent.create() .withOrigin(SCMEvent.originOf(Stapler.getCurrentRequest2())) .withTriggeredByUser(triggeredByUser) + .withTriggeredByRef(triggeredByRef) .build() ); } @@ -103,6 +104,7 @@ public void onPost(final GitHubTriggerEvent event) { Job currentJob = notNull(job, "Job can't be null"); final String pushBy = event.getTriggeredByUser(); + final String ref = event.getTriggeredByRef(); DescriptorImpl d = getDescriptor(); d.checkThreadPoolSizeAndUpdateIfNecessary(); d.queue.execute(new Runnable() { @@ -151,10 +153,10 @@ public void run() { if (runPolling()) { GitHubPushCause cause; try { - cause = new GitHubPushCause(getLogFileForJob(currentJob), pushBy); + cause = new GitHubPushCause(getLogFileForJob(currentJob), pushBy, ref); } catch (IOException e) { LOGGER.warn("Failed to parse the polling log", e); - cause = new GitHubPushCause(pushBy); + cause = new GitHubPushCause(pushBy, ref); } if (asParameterizedJobMixIn(currentJob).scheduleBuild(cause)) { diff --git a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java index 20d75d309..de6829225 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java @@ -112,7 +112,7 @@ public void parseAssociatedNames(AbstractProject job, Collection parseAssociatedNames(Item item) { return names; } + /** + * @deprecated Use {@link #parseAssociatedBranches(Job)} + */ + @Deprecated + public static Collection parseAssociatedBranches(AbstractProject job) { + return parseAssociatedBranches((Item) job); + } + + /** + * @deprecated Use {@link #parseAssociatedBranches(Item)} + */ + @Deprecated + public static Collection parseAssociatedBranches(Job job) { + return parseAssociatedBranches((Item) job); + } + public static Collection parseAssociatedBranches(Item item) { Set names = new HashSet(); for (GitHubRepositoryNameContributor c : all()) { diff --git a/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java b/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java index 7fd00b178..6ab9ce61e 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubTrigger.java @@ -23,7 +23,7 @@ public interface GitHubTrigger { void onPost(); // TODO: document me - void onPost(String triggeredByUser); + void onPost(String triggeredByUser, String triggeredByRef); /** * Obtains the list of the repositories that this trigger is looking at. diff --git a/src/main/java/com/cloudbees/jenkins/GitHubTriggerEvent.java b/src/main/java/com/cloudbees/jenkins/GitHubTriggerEvent.java index fdae66124..c062b4ab8 100644 --- a/src/main/java/com/cloudbees/jenkins/GitHubTriggerEvent.java +++ b/src/main/java/com/cloudbees/jenkins/GitHubTriggerEvent.java @@ -22,11 +22,16 @@ public class GitHubTriggerEvent { * The user that the event was provided by. */ private final String triggeredByUser; + /** + * The target ref that the push event was for. + */ + private final String triggeredByRef; - private GitHubTriggerEvent(long timestamp, String origin, String triggeredByUser) { + private GitHubTriggerEvent(long timestamp, String origin, String triggeredByUser, String triggeredByRef) { this.timestamp = timestamp; this.origin = origin; this.triggeredByUser = triggeredByUser; + this.triggeredByRef = triggeredByRef; } public static Builder create() { @@ -45,6 +50,10 @@ public String getTriggeredByUser() { return triggeredByUser; } + public String getTriggeredByRef() { + return triggeredByRef; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -62,7 +71,10 @@ public boolean equals(Object o) { if (origin != null ? !origin.equals(that.origin) : that.origin != null) { return false; } - return triggeredByUser != null ? triggeredByUser.equals(that.triggeredByUser) : that.triggeredByUser == null; + if (triggeredByUser != null ? !triggeredByUser.equals(that.triggeredByUser) : that.triggeredByUser != null) { + return false; + } + return triggeredByRef != null ? triggeredByRef.equals(that.triggeredByRef) : that.triggeredByRef == null; } @Override @@ -70,6 +82,7 @@ public int hashCode() { int result = (int) (timestamp ^ (timestamp >>> 32)); result = 31 * result + (origin != null ? origin.hashCode() : 0); result = 31 * result + (triggeredByUser != null ? triggeredByUser.hashCode() : 0); + result = 31 * result + (triggeredByRef != null ? triggeredByRef.hashCode() : 0); return result; } @@ -79,6 +92,7 @@ public String toString() { + "timestamp=" + timestamp + ", origin='" + origin + '\'' + ", triggeredByUser='" + triggeredByUser + '\'' + + ", triggeredByRef='" + triggeredByRef + '\'' + '}'; } @@ -89,6 +103,7 @@ public static class Builder { private long timestamp; private String origin; private String triggeredByUser; + private String triggeredByRef; private Builder() { timestamp = System.currentTimeMillis(); @@ -109,8 +124,13 @@ public Builder withTriggeredByUser(String triggeredByUser) { return this; } + public Builder withTriggeredByRef(String triggeredByRef) { + this.triggeredByRef = triggeredByRef; + return this; + } + public GitHubTriggerEvent build() { - return new GitHubTriggerEvent(timestamp, origin, triggeredByUser); + return new GitHubTriggerEvent(timestamp, origin, triggeredByUser, triggeredByRef); } @Override @@ -119,6 +139,7 @@ public String toString() { + "timestamp=" + timestamp + ", origin='" + origin + '\'' + ", triggeredByUser='" + triggeredByUser + '\'' + + ", triggeredByRef='" + triggeredByRef + '\'' + '}'; } } diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java index c638b302a..ceb3fdbfc 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java @@ -22,6 +22,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.Collection; import java.util.Set; import static com.google.common.collect.Sets.immutableEnumSet; @@ -66,6 +67,9 @@ protected Set events() { * @param event only PUSH event */ @Override + @edu.umd.cs.findbugs.annotations.SuppressFBWarnings( + value = "SIC_INNER_SHOULD_BE_STATIC_ANON", justification = "anonymous inner class is acceptable" + ) protected void onEvent(final GHSubscriberEvent event) { GHEventPayload.Push push; try { @@ -77,7 +81,7 @@ protected void onEvent(final GHSubscriberEvent event) { URL repoUrl = push.getRepository().getUrl(); final String pusherName = push.getPusher().getName(); final String ref = push.getRef(); - LOGGER.info("Received PushEvent for {} from {}", repoUrl, event.getOrigin()); + LOGGER.info("Received PushEvent for {} {} from {}", repoUrl, ref, event.getOrigin()); GitHubRepositoryName fromEventRepository = GitHubRepositoryName.create(repoUrl.toExternalForm()); if (fromEventRepository == null) { @@ -94,12 +98,8 @@ protected void onEvent(final GHSubscriberEvent event) { } final GitHubRepositoryName changedRepository = fromEventRepository; - if (changedRepository != null) { - // run in high privilege to see all the projects anonymous users don't see. - // this is safe because when we actually schedule a build, it's a build that can - // happen at some random time anyway. - Runnable body = new Runnable() { + final Runnable body = new Runnable() { @Override public void run() { Jenkins j = Jenkins.getInstanceOrNull(); @@ -114,32 +114,48 @@ public void run() { } String fullDisplayName = job.getFullDisplayName(); LOGGER.debug("Considering to poke {}", fullDisplayName); - if (!GitHubRepositoryNameContributor.parseAssociatedNames(job).contains(changedRepository)) { - LOGGER.debug("Skipped {} because it doesn't have a matching repository.", fullDisplayName); + final Collection names = + GitHubRepositoryNameContributor.parseAssociatedNames(job); + if (!names.contains(changedRepository)) { + LOGGER.debug( + "Skipped {} because {} doesn't have a matching repository for {}.", + fullDisplayName, names, changedRepository); continue; } - boolean foundBranch = false; - for (GitHubBranch branch : GitHubRepositoryNameContributor.parseAssociatedBranches(job)) { - if (branch.matches(changedRepository, ref)) { - foundBranch = true; - break; + try { + final Collection branchSpecs = + GitHubRepositoryNameContributor.parseAssociatedBranches(job); + if (!branchSpecs.isEmpty()) { + boolean foundBranch = false; + for (GitHubBranch branch : branchSpecs) { + if (branch.matches(changedRepository, ref)) { + foundBranch = true; + break; + } + } + if (!foundBranch) { + LOGGER.debug("Skipped {} because it doesn't have a matching branch specifier.", + job.getFullDisplayName()); + continue; + } } - } - if (!foundBranch) { - LOGGER.debug("Skipped {} because it doesn't have a matching branch specifier.", - job.getFullDisplayName()); - continue; + } catch (Exception e) { + LOGGER.error("parseAssociatedBranches threw exception", e); } LOGGER.info("Poked {}", fullDisplayName); trigger.onPost(GitHubTriggerEvent.create() .withTimestamp(event.getTimestamp()) .withOrigin(event.getOrigin()) .withTriggeredByUser(pusherName) + .withTriggeredByRef(ref) .build() ); } } }; + // run in high privilege to see all the projects anonymous users don't see. + // this is safe because when we actually schedule a build, it's a build that can + // happen at some random time anyway. ACL.impersonate(ACL.SYSTEM, body); for (GitHubWebHook.Listener listener : ExtensionList.lookup(GitHubWebHook.Listener.class)) { diff --git a/src/test/java/com/cloudbees/jenkins/GitHubPushTriggerTest.java b/src/test/java/com/cloudbees/jenkins/GitHubPushTriggerTest.java index 79508225b..f51fbe1d7 100644 --- a/src/test/java/com/cloudbees/jenkins/GitHubPushTriggerTest.java +++ b/src/test/java/com/cloudbees/jenkins/GitHubPushTriggerTest.java @@ -26,6 +26,7 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.jenkinsci.plugins.github.webhook.subscriber.DefaultPushGHEventListenerTest.TRIGGERED_BY_USER_FROM_RESOURCE; +import static org.jenkinsci.plugins.github.webhook.subscriber.DefaultPushGHEventListenerTest.TRIGGERED_BY_REF_FROM_RESOURCE; /** * @author lanwen (Merkushev Kirill) @@ -69,7 +70,7 @@ public void shouldStartWorkflowByTrigger() throws Exception { buildData.buildsByBranchName = new HashMap(); buildData.getLastBuiltRevision().setSha1(ObjectId.zeroId()); - trigger.onPost(TRIGGERED_BY_USER_FROM_RESOURCE); + trigger.onPost(TRIGGERED_BY_USER_FROM_RESOURCE, TRIGGERED_BY_REF_FROM_RESOURCE); TimeUnit.SECONDS.sleep(job.getQuietPeriod()); jRule.waitUntilNoActivity(); diff --git a/src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java b/src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java index 5e79cf702..9ddec212d 100644 --- a/src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java +++ b/src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java @@ -42,6 +42,7 @@ public class DefaultPushGHEventListenerTest { public static final GitSCM GIT_SCM_FROM_RESOURCE = new GitSCM("ssh://git@github.com/lanwen/test.git"); public static final String TRIGGERED_BY_USER_FROM_RESOURCE = "lanwen"; + public static final String TRIGGERED_BY_REF_FROM_RESOURCE = "refs/heads/master"; @Rule public JenkinsRule jenkins = new JenkinsRule(); @@ -110,6 +111,7 @@ public void shouldParsePushPayload() { .withTimestamp(subscriberEvent.getTimestamp()) .withOrigin("shouldParsePushPayload") .withTriggeredByUser(TRIGGERED_BY_USER_FROM_RESOURCE) + .withTriggeredByRef(TRIGGERED_BY_REF_FROM_RESOURCE) .build() )); } @@ -133,6 +135,7 @@ public void shouldReceivePushHookOnWorkflow() throws Exception { .withTimestamp(subscriberEvent.getTimestamp()) .withOrigin("shouldReceivePushHookOnWorkflow") .withTriggeredByUser(TRIGGERED_BY_USER_FROM_RESOURCE) + .withTriggeredByRef(TRIGGERED_BY_REF_FROM_RESOURCE) .build() )); } From f808ab65ad78d589aa014cdc8460d5bb0c9f56db Mon Sep 17 00:00:00 2001 From: Dominic Evans Date: Mon, 30 May 2022 23:35:14 +0100 Subject: [PATCH 3/4] fix: log branch specifier mismatch at info level --- .../github/webhook/subscriber/DefaultPushGHEventSubscriber.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java index ceb3fdbfc..0bb600ce6 100644 --- a/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java +++ b/src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java @@ -134,7 +134,7 @@ public void run() { } } if (!foundBranch) { - LOGGER.debug("Skipped {} because it doesn't have a matching branch specifier.", + LOGGER.info("Skipped {} because it doesn't have a matching branch specifier.", job.getFullDisplayName()); continue; } From 066df56bb070d5b5f49e631e22dd8f824522654a Mon Sep 17 00:00:00 2001 From: Dominic Evans Date: Tue, 31 May 2022 09:37:55 +0100 Subject: [PATCH 4/4] chore: add suppression for spotbugs nullable warn Ref: spotbugs/spotbugs 616 --- .../jenkinsci/plugins/github/util/misc/NullSafeFunction.java | 4 ++++ .../jenkinsci/plugins/github/util/misc/NullSafePredicate.java | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/src/main/java/org/jenkinsci/plugins/github/util/misc/NullSafeFunction.java b/src/main/java/org/jenkinsci/plugins/github/util/misc/NullSafeFunction.java index 3a0918247..e51ed8287 100644 --- a/src/main/java/org/jenkinsci/plugins/github/util/misc/NullSafeFunction.java +++ b/src/main/java/org/jenkinsci/plugins/github/util/misc/NullSafeFunction.java @@ -14,6 +14,10 @@ public abstract class NullSafeFunction implements Function { @Override + @edu.umd.cs.findbugs.annotations.SuppressFBWarnings( + value = "NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE", + justification = "https://github.com/spotbugs/spotbugs/issues/616" + ) public T apply(F input) { return applyNullSafe(checkNotNull(input, "This function does not allow using null as argument")); } diff --git a/src/main/java/org/jenkinsci/plugins/github/util/misc/NullSafePredicate.java b/src/main/java/org/jenkinsci/plugins/github/util/misc/NullSafePredicate.java index 847753d59..cb085a71d 100644 --- a/src/main/java/org/jenkinsci/plugins/github/util/misc/NullSafePredicate.java +++ b/src/main/java/org/jenkinsci/plugins/github/util/misc/NullSafePredicate.java @@ -15,6 +15,10 @@ public abstract class NullSafePredicate implements Predicate { @Override + @edu.umd.cs.findbugs.annotations.SuppressFBWarnings( + value = "NP_PARAMETER_MUST_BE_NONNULL_BUT_MARKED_AS_NULLABLE", + justification = "https://github.com/spotbugs/spotbugs/issues/616" + ) public boolean apply(T input) { return applyNullSafe(checkNotNull(input, "Argument for this predicate can't be null")); }