Skip to content

Commit 4c0ffd0

Browse files
dnweaserrallerios
dnwe
and
aserrallerios
committed
fix: don't trigger poll unless target ref matches
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 jenkinsci#44 with some small refinements. Co-authored-by: Albert Serrallé Ríos <aserrallerios@gmail.com>
1 parent 8783201 commit 4c0ffd0

8 files changed

Lines changed: 264 additions & 23 deletions

File tree

‎src/main/java/com/cloudbees/jenkins/GitHubBranch.java‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
package com.cloudbees.jenkins;
2+
3+
import hudson.plugins.git.BranchSpec;
4+
5+
import java.util.logging.Level;
6+
import java.util.logging.Logger;
7+
8+
/**
9+
* Identifies a GitHub branch and hides the SCM branch from its clients.
10+
*/
11+
public class GitHubBranch {
12+
13+
private static final String GITHUB_BRANCH_PREFIX = "refs/heads";
14+
private static final int GITHUB_BRANCH_PREFIX_LENGTH = 10;
15+
16+
private final BranchSpec branch;
17+
18+
private GitHubBranch(final BranchSpec branch) {
19+
this.branch = branch;
20+
}
21+
22+
public boolean matches(GitHubRepositoryName repository, String ref) {
23+
// ref (github) -> refs/heads/master
24+
// branch (git SCM) -> REPO/remote/branch
25+
// matching the meaningful part of the github branch name with
26+
// the configured SCM branch expression
27+
if (ref != null && !ref.equals("")) {
28+
String branchExpression = "*";
29+
if (repository != null && repository.repositoryName != null) {
30+
branchExpression = repository.repositoryName;
31+
}
32+
if (ref.startsWith(GITHUB_BRANCH_PREFIX)) {
33+
branchExpression += ref.substring(GITHUB_BRANCH_PREFIX_LENGTH);
34+
} else {
35+
branchExpression += "/" + ref;
36+
}
37+
LOGGER.log(Level.FINE, "Does SCM branch " + branch.getName()
38+
+ " match GitHub branch " + branchExpression + "?");
39+
return branch.matches(branchExpression);
40+
}
41+
return false;
42+
}
43+
44+
public static GitHubBranch create(BranchSpec branch) {
45+
if (isValidGitHubBranch(branch)) {
46+
return new GitHubBranch(branch);
47+
} else {
48+
return null;
49+
}
50+
}
51+
52+
// TODO: implement logic to validate git SCM branches.
53+
private static boolean isValidGitHubBranch(BranchSpec branch) {
54+
return true;
55+
}
56+
57+
private static final Logger LOGGER = Logger.getLogger(GitHubBranch.class
58+
.getName());
59+
}

‎src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,14 @@ public Set<GitHubRepositoryName> getGitHubRepositories() {
195195
return Collections.emptySet();
196196
}
197197

198+
/**
199+
* @deprecated
200+
* Use {@link GitHubRepositoryNameContributor#parseAssociatedBranches(AbstractProject)}
201+
*/
202+
public Set<GitHubBranch> getGitHubBranches() {
203+
return Collections.emptySet();
204+
}
205+
198206
@Override
199207
public void start(Job<?, ?> project, boolean newInstance) {
200208
super.start(project, newInstance);

‎src/main/java/com/cloudbees/jenkins/GitHubRepositoryNameContributor.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import hudson.model.Item;
1111
import hudson.model.Job;
1212
import hudson.model.TaskListener;
13+
import hudson.plugins.git.BranchSpec;
1314
import hudson.plugins.git.GitSCM;
1415
import hudson.scm.SCM;
1516
import jenkins.model.Jenkins;
@@ -90,6 +91,38 @@ public void parseAssociatedNames(AbstractProject<?, ?> job, Collection<GitHubRep
9091
}
9192
}
9293

94+
/**
95+
* Looks at the definition of {@link Item} and list up its branches, then puts them into
96+
* the collection.
97+
*/
98+
public /*abstract*/ void parseAssociatedBranches(Item item, Collection<GitHubBranch> result) {
99+
if (Util.isOverridden(
100+
GitHubRepositoryNameContributor.class,
101+
getClass(),
102+
"parseAssociatedBranches",
103+
Job.class,
104+
Collection.class
105+
)) {
106+
// if this impl is legacy, it cannot contribute to non-jobs, so not an error
107+
if (item instanceof Job) {
108+
parseAssociatedBranches((Job<?, ?>) item, result);
109+
}
110+
} else if (Util.isOverridden(
111+
GitHubRepositoryNameContributor.class,
112+
getClass(),
113+
"parseAssociatedBranches",
114+
AbstractProject.class,
115+
Collection.class
116+
)) {
117+
// if this impl is legacy, it cannot contribute to non-projects, so not an error
118+
if (item instanceof AbstractProject) {
119+
parseAssociatedBranches((AbstractProject<?, ?>) item, result);
120+
}
121+
} else {
122+
throw new AbstractMethodError("you must override the new overload of parseAssociatedBranches");
123+
}
124+
};
125+
93126
public static ExtensionList<GitHubRepositoryNameContributor> all() {
94127
return Jenkins.getInstance().getExtensionList(GitHubRepositoryNameContributor.class);
95128
}
@@ -118,6 +151,14 @@ public static Collection<GitHubRepositoryName> parseAssociatedNames(Item item) {
118151
return names;
119152
}
120153

154+
public static Collection<GitHubBranch> parseAssociatedBranches(Item item) {
155+
Set<GitHubBranch> names = new HashSet<GitHubBranch>();
156+
for (GitHubRepositoryNameContributor c : all()) {
157+
c.parseAssociatedBranches(item, names);
158+
}
159+
return names;
160+
}
161+
121162
/**
122163
* Default implementation that looks at SCMs
123164
*/
@@ -134,6 +175,16 @@ public void parseAssociatedNames(Item item, Collection<GitHubRepositoryName> res
134175
}
135176
}
136177

178+
@Override
179+
public void parseAssociatedBranches(Item item, Collection<GitHubBranch> result) {
180+
SCMTriggerItem triggerItem = SCMTriggerItems.asSCMTriggerItem(item);
181+
if (triggerItem != null) {
182+
for (SCM scm : triggerItem.getSCMs()) {
183+
addBranches(scm, result);
184+
}
185+
}
186+
}
187+
137188
protected EnvVars buildEnv(Job<?, ?> job) {
138189
EnvVars env = new EnvVars();
139190
for (EnvironmentContributor contributor : EnvironmentContributor.all()) {
@@ -160,5 +211,17 @@ protected static void addRepositories(SCM scm, EnvVars env, Collection<GitHubRep
160211
}
161212
}
162213
}
214+
215+
protected static void addBranches(SCM scm, Collection<GitHubBranch> r) {
216+
if (scm instanceof GitSCM) {
217+
GitSCM git = (GitSCM) scm;
218+
for (BranchSpec branch : git.getBranches()) {
219+
GitHubBranch gitHubBranch = GitHubBranch.create(branch);
220+
if (gitHubBranch != null) {
221+
r.add(gitHubBranch);
222+
}
223+
}
224+
}
225+
}
163226
}
164227
}

‎src/main/java/com/cloudbees/jenkins/GitHubTrigger.java‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,26 @@ public interface GitHubTrigger {
4040
*/
4141
Set<GitHubRepositoryName> getGitHubRepositories();
4242

43+
/**
44+
* Obtains the list of the branches that this trigger is looking at.
45+
*
46+
* If the implementation of this class maintain its own list of GitHub branches, it should
47+
* continue to implement this method for backward compatibility, and it gets picked up by
48+
* {@link GitHubRepositoryNameContributor#parseAssociatedBranches(AbstractProject)}.
49+
*
50+
* <p>
51+
* Alternatively, if the implementation doesn't worry about the backward compatibility, it can
52+
* implement this method to return an empty collection, then just implement {@link GitHubRepositoryNameContributor}.
53+
*
54+
* @deprecated
55+
* Call {@link GitHubRepositoryNameContributor#parseAssociatedBranches(AbstractProject)} instead.
56+
*/
57+
Set<GitHubBranch> getGitHubBranches();
58+
4359
/**
4460
* Contributes {@link GitHubRepositoryName} from {@link GitHubTrigger#getGitHubRepositories()}
45-
* for backward compatibility
61+
* and {@link GitHubBranch} from {@link GitHubTrigger#getGitHubBranches()} for
62+
* backward compatibility.
4663
*/
4764
@Extension
4865
class GitHubRepositoryNameContributorImpl extends GitHubRepositoryNameContributor {
@@ -56,5 +73,15 @@ public void parseAssociatedNames(Item item, Collection<GitHubRepositoryName> res
5673
}
5774
}
5875
}
76+
77+
@Override
78+
public void parseAssociatedBranches(Item item, Collection<GitHubBranch> result) {
79+
if (item instanceof ParameterizedJobMixIn.ParameterizedJob) {
80+
ParameterizedJobMixIn.ParameterizedJob p = (ParameterizedJobMixIn.ParameterizedJob) item;
81+
for (GitHubTrigger ght : Util.filter(p.getTriggers().values(), GitHubTrigger.class)) {
82+
result.addAll(ght.getGitHubBranches());
83+
}
84+
}
85+
}
5986
}
6087
}

‎src/main/java/org/jenkinsci/plugins/github/migration/Migrator.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@ public class Migrator {
3838
public void migrate() throws IOException {
3939
LOGGER.debug("Check if GitHub Plugin needs config migration");
4040
GitHubPushTrigger.DescriptorImpl descriptor = GitHubPushTrigger.DescriptorImpl.get();
41+
if (descriptor == null) {
42+
return;
43+
}
4144
descriptor.load();
4245

4346
if (isNotEmpty(descriptor.getCredentials())) {

‎src/main/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventSubscriber.java‎

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
package org.jenkinsci.plugins.github.webhook.subscriber;
22

3+
import com.cloudbees.jenkins.GitHubBranch;
34
import com.cloudbees.jenkins.GitHubPushTrigger;
45
import com.cloudbees.jenkins.GitHubRepositoryName;
56
import com.cloudbees.jenkins.GitHubRepositoryNameContributor;
@@ -75,6 +76,7 @@ protected void onEvent(final GHSubscriberEvent event) {
7576
}
7677
URL repoUrl = push.getRepository().getUrl();
7778
final String pusherName = push.getPusher().getName();
79+
final String ref = push.getRef();
7880
LOGGER.info("Received PushEvent for {} from {}", repoUrl, event.getOrigin());
7981
GitHubRepositoryName fromEventRepository = GitHubRepositoryName.create(repoUrl.toExternalForm());
8082

@@ -97,31 +99,48 @@ protected void onEvent(final GHSubscriberEvent event) {
9799
// run in high privilege to see all the projects anonymous users don't see.
98100
// this is safe because when we actually schedule a build, it's a build that can
99101
// happen at some random time anyway.
100-
ACL.impersonate(ACL.SYSTEM, new Runnable() {
102+
Runnable body = new Runnable() {
101103
@Override
102104
public void run() {
103-
for (Item job : Jenkins.getInstance().getAllItems(Item.class)) {
105+
Jenkins j = Jenkins.getInstanceOrNull();
106+
if (j == null) {
107+
return;
108+
}
109+
110+
for (Item job : j.getAllItems(Item.class)) {
104111
GitHubPushTrigger trigger = triggerFrom(job, GitHubPushTrigger.class);
105-
if (trigger != null) {
106-
String fullDisplayName = job.getFullDisplayName();
107-
LOGGER.debug("Considering to poke {}", fullDisplayName);
108-
if (GitHubRepositoryNameContributor.parseAssociatedNames(job)
109-
.contains(changedRepository)) {
110-
LOGGER.info("Poked {}", fullDisplayName);
111-
trigger.onPost(GitHubTriggerEvent.create()
112-
.withTimestamp(event.getTimestamp())
113-
.withOrigin(event.getOrigin())
114-
.withTriggeredByUser(pusherName)
115-
.build()
116-
);
117-
} else {
118-
LOGGER.debug("Skipped {} because it doesn't have a matching repository.",
119-
fullDisplayName);
112+
if (trigger == null) {
113+
continue;
114+
}
115+
String fullDisplayName = job.getFullDisplayName();
116+
LOGGER.debug("Considering to poke {}", fullDisplayName);
117+
if (!GitHubRepositoryNameContributor.parseAssociatedNames(job).contains(changedRepository)) {
118+
LOGGER.debug("Skipped {} because it doesn't have a matching repository.", fullDisplayName);
119+
continue;
120+
}
121+
boolean foundBranch = false;
122+
for (GitHubBranch branch : GitHubRepositoryNameContributor.parseAssociatedBranches(job)) {
123+
if (branch.matches(changedRepository, ref)) {
124+
foundBranch = true;
125+
break;
120126
}
121127
}
128+
if (!foundBranch) {
129+
LOGGER.debug("Skipped {} because it doesn't have a matching branch specifier.",
130+
job.getFullDisplayName());
131+
continue;
132+
}
133+
LOGGER.info("Poked {}", fullDisplayName);
134+
trigger.onPost(GitHubTriggerEvent.create()
135+
.withTimestamp(event.getTimestamp())
136+
.withOrigin(event.getOrigin())
137+
.withTriggeredByUser(pusherName)
138+
.build()
139+
);
122140
}
123141
}
124-
});
142+
};
143+
ACL.impersonate(ACL.SYSTEM, body);
125144

126145
for (GitHubWebHook.Listener listener : ExtensionList.lookup(GitHubWebHook.Listener.class)) {
127146
listener.onPushRepositoryChanged(pusherName, changedRepository);

‎src/test/java/com/cloudbees/jenkins/GitHubBranchTest.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
package com.cloudbees.jenkins;
2+
3+
import hudson.plugins.git.BranchSpec;
4+
import org.junit.Assert;
5+
6+
import org.junit.Test;
7+
8+
public class GitHubBranchTest {
9+
10+
@Test
11+
public void testBranchMatch() throws Exception, InterruptedException {
12+
GitHubRepositoryName repo = GitHubRepositoryName.create("github-repo");
13+
14+
BranchSpec scmBranch = new BranchSpec("*/github-branch");
15+
GitHubBranch branch = GitHubBranch.create(scmBranch);
16+
17+
Assert.assertTrue(branch.matches(repo, "refs/heads/github-branch"));
18+
Assert.assertTrue(branch.matches(repo, "github-branch"));
19+
}
20+
21+
@Test
22+
public void testBranchDoesNotMatch() throws Exception, InterruptedException {
23+
GitHubRepositoryName repo = GitHubRepositoryName.create("github-repo");
24+
25+
BranchSpec scmBranch = new BranchSpec("*/github-branch");
26+
GitHubBranch branch = GitHubBranch.create(scmBranch);
27+
28+
Assert.assertFalse(branch.matches(repo, "refs/heads/github-branch-2"));
29+
Assert.assertFalse(branch.matches(repo, "github-branch-2"));
30+
}
31+
32+
@Test
33+
public void testRepoDoesNotMatch() throws Exception, InterruptedException {
34+
GitHubRepositoryName repo = GitHubRepositoryName.create("github-repo");
35+
36+
BranchSpec scmBranch = new BranchSpec("github-repo-2/github-branch");
37+
GitHubBranch branch = GitHubBranch.create(scmBranch);
38+
39+
Assert.assertFalse(branch.matches(repo, "refs/heads/github-branch"));
40+
Assert.assertFalse(branch.matches(repo, "github-branch"));
41+
}
42+
}

‎src/test/java/org/jenkinsci/plugins/github/webhook/subscriber/DefaultPushGHEventListenerTest.java‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,10 @@
33
import com.cloudbees.jenkins.GitHubPushTrigger;
44
import com.cloudbees.jenkins.GitHubRepositoryNameContributor;
55
import com.cloudbees.jenkins.GitHubTriggerEvent;
6+
import com.cloudbees.jenkins.GitHubWebHook;
7+
68
import hudson.ExtensionList;
9+
import hudson.model.EnvironmentContributor;
710
import hudson.model.FreeStyleProject;
811
import hudson.model.Item;
912
import hudson.plugins.git.GitSCM;
@@ -75,14 +78,31 @@ public void shouldParsePushPayload() {
7578
Jenkins jenkins = mock(Jenkins.class);
7679
when(jenkins.getAllItems(Item.class)).thenReturn(Collections.singletonList(prj));
7780

78-
ExtensionList<GitHubRepositoryNameContributor> extensionList = mock(ExtensionList.class);
79-
List<GitHubRepositoryNameContributor> gitHubRepositoryNameContributorList =
81+
{
82+
ExtensionList<GitHubRepositoryNameContributor> extensionList = mock(ExtensionList.class);
83+
List<GitHubRepositoryNameContributor> gitHubRepositoryNameContributorList =
8084
Collections.singletonList(new GitHubRepositoryNameContributor.FromSCM());
81-
when(extensionList.iterator()).thenReturn(gitHubRepositoryNameContributorList.iterator());
82-
when(jenkins.getExtensionList(GitHubRepositoryNameContributor.class)).thenReturn(extensionList);
85+
when(extensionList.iterator()).thenReturn(gitHubRepositoryNameContributorList.iterator());
86+
when(jenkins.getExtensionList(GitHubRepositoryNameContributor.class)).thenReturn(extensionList);
87+
}
88+
89+
{
90+
ExtensionList<EnvironmentContributor> extensionList = mock(ExtensionList.class);
91+
List<EnvironmentContributor> environmentContributorList = Collections.emptyList();
92+
when(extensionList.iterator()).thenReturn(environmentContributorList.iterator());
93+
when(jenkins.getExtensionList(EnvironmentContributor.class)).thenReturn(extensionList);
94+
}
95+
96+
{
97+
ExtensionList<GitHubWebHook.Listener> extensionList = mock(ExtensionList.class);
98+
List<GitHubWebHook.Listener> listenerList = Collections.emptyList();
99+
when(extensionList.iterator()).thenReturn(listenerList.iterator());
100+
when(jenkins.getExtensionList(GitHubWebHook.Listener.class)).thenReturn(extensionList);
101+
}
83102

84103
try (MockedStatic<Jenkins> mockedJenkins = mockStatic(Jenkins.class)) {
85104
mockedJenkins.when(Jenkins::getInstance).thenReturn(jenkins);
105+
mockedJenkins.when(Jenkins::getInstanceOrNull).thenReturn(jenkins);
86106
new DefaultPushGHEventSubscriber().onEvent(subscriberEvent);
87107
}
88108

0 commit comments

Comments
 (0)