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.

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

[FIXED JENKINS-41811] Expose event origin to listeners #164

Merged
merged 10 commits into from
Feb 10, 2017
8 changes: 7 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,13 @@
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>git</artifactId>
<version>2.4.0</version>
<version>2.4.0</version> <!-- should really be 2.6.4 to match scm-api but ok as git wass forward compat -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Workflow tests need rework for the updated git dependency, but as git plugin didn't break the rules of SCM API we don't need to update to 2.6.4 / 3.0.4... just would be missing the SCMEvent integration in git plugin, but the tests in this plugin do not rely on that functionality, so no harm

Copy link
Member

Choose a reason for hiding this comment

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

typo

</dependency>

<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>scm-api</artifactId>
<version>2.0.3</version>
</dependency>

<dependency>
Expand Down
14 changes: 13 additions & 1 deletion src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import hudson.util.StreamTaskListener;
import jenkins.model.Jenkins;
import jenkins.model.ParameterizedJobMixIn;
import jenkins.scm.api.SCMEvent;
import jenkins.triggers.SCMTriggerItem.SCMTriggerItems;
import org.apache.commons.jelly.XMLOutput;
import org.jenkinsci.plugins.github.GitHubPlugin;
Expand All @@ -31,6 +32,7 @@
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.stapler.AncestorInPath;
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.Stapler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -57,7 +59,7 @@
*
* @author Kohsuke Kawaguchi
*/
public class GitHubPushTrigger extends Trigger<Job<?, ?>> implements GitHubTrigger {
public class GitHubPushTrigger extends Trigger<Job<?, ?>> implements GitHubTrigger2 {
Copy link
Member

Choose a reason for hiding this comment

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

can we just drop this Interface implementation? Do you know somebody who use it?

Copy link
Member

Choose a reason for hiding this comment

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

You can't know who is using it.

Copy link
Member

Choose a reason for hiding this comment

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

so we can just continue to implement old interface (marked as deprecated) and do nothing with its methods and just use brand new method

Copy link
Member

Choose a reason for hiding this comment

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

Or switch to java8?


@DataBoundConstructor
public GitHubPushTrigger() {
Expand All @@ -75,6 +77,13 @@ public void onPost() {
* Called when a POST is made.
*/
public void onPost(String triggeredByUser) {
onPost(SCMEvent.originOf(Stapler.getCurrentRequest()), triggeredByUser);
Copy link
Member

Choose a reason for hiding this comment

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

So why not call Stapler.getCurrentRequest() in GitHubTrigger implementation, it still the same thread?

}

/**
* Called when a POST is made.
*/
public void onPost(final String origin, String triggeredByUser) {
Copy link
Member

Choose a reason for hiding this comment

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

S maybe replace it with more common object with fields that will be expandable in future?

final String pushBy = triggeredByUser;
DescriptorImpl d = getDescriptor();
d.checkThreadPoolSizeAndUpdateIfNecessary();
Expand All @@ -87,6 +96,9 @@ private boolean runPolling() {
PrintStream logger = listener.getLogger();
long start = System.currentTimeMillis();
logger.println("Started on " + DateFormat.getDateTimeInstance().format(new Date()));
if (origin != null) {
logger.println("Started by event from " + origin);
}
boolean result = SCMTriggerItems.asSCMTriggerItem(job).poll(listener).hasChanges();
logger.println("Done. Took " + Util.getTimeSpanString(System.currentTimeMillis() - start));
if (result) {
Expand Down
1 change: 1 addition & 0 deletions src/main/java/com/cloudbees/jenkins/GitHubTrigger.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* and triggers a build.
*
* @author aaronwalker
* @deprecated extend {@link GitHubTrigger2} instead
Copy link
Member

Choose a reason for hiding this comment

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

Careful—does this produce deprecation warnings for implementers of GitHubTrigger2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Who has implemented it? it is doubtful the interface is even required.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't show as deprecated in my IDE
screen shot 2017-02-07 at 19 10 08

Copy link
Member

Choose a reason for hiding this comment

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

i think @jglick mean compiler warnings, but i guess they work only on annotations.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if this interface is required for https://github.com/jenkinsci/github-sqs-plugin/blob/master/src/main/java/com/base2services/jenkins/SqsBuildTrigger.java or if it is a gratuitous linkage between the two plugins. It is the only OSS consumer of this interface... the only reason for it to exist is so that the two trigger options will both work for triggering builds... which seems a really crappy UX

screen shot 2017-02-09 at 17 14 04

i.e. is it obvious to the user that checking one or both of the above check boxes will do exactly the same thing?

And those check boxes are kind of against the idiomatic Jenkins way for configuring hooks, i.e. through Poll SCM

screen shot 2017-02-09 at 17 14 29

I really think all this needs a good tidy up... but I am not going to take that on in this PR ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

(FTR the "Build when a message is published to an SQS Queue" sounds more that a bit scary... what kind of message and what queue? Does it mean that every time any message is pushed to any SQS Queue anywhere in the world that my build will be triggered?)

*/
public interface GitHubTrigger {

Expand Down
15 changes: 15 additions & 0 deletions src/main/java/com/cloudbees/jenkins/GitHubTrigger2.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package com.cloudbees.jenkins;

import hudson.triggers.Trigger;

/**
* Optional interface that can be implemented by {@link Trigger} that watches out for a change in GitHub
* and triggers a build.
*
* @author aaronwalker
*/
public interface GitHubTrigger2 extends GitHubTrigger {

// TODO: document me
void onPost(String origin, String triggeredByUser);
}
10 changes: 9 additions & 1 deletion src/main/java/com/cloudbees/jenkins/GitHubWebHook.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,18 @@
import hudson.model.UnprotectedRootAction;
import hudson.util.SequentialExecutionQueue;
import jenkins.model.Jenkins;
import jenkins.scm.api.SCMEvent;
import org.apache.commons.lang3.Validate;
import org.jenkinsci.plugins.github.GitHubPlugin;
import org.jenkinsci.plugins.github.extension.GHEventsSubscriber;
import org.jenkinsci.plugins.github.internal.GHPluginConfigException;
import org.jenkinsci.plugins.github.webhook.GHEventHeader;
import org.jenkinsci.plugins.github.webhook.GHEventPayload;
import org.jenkinsci.plugins.github.webhook.RequirePostWithGHHookPayload;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;
import org.kohsuke.github.GHEvent;
import org.kohsuke.stapler.Stapler;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -114,7 +118,7 @@ public List<Item> reRegisterAllHooks() {
public void doIndex(@Nonnull @GHEventHeader GHEvent event, @Nonnull @GHEventPayload String payload) {
from(GHEventsSubscriber.all())
.filter(isInterestedIn(event))
.transform(processEvent(event, payload)).toList();
.transform(processEvent(SCMEvent.originOf(Stapler.getCurrentRequest()), event, payload)).toList();
}

private <T extends Item> Function<T, T> reRegisterHookForJob() {
Expand Down Expand Up @@ -153,7 +157,11 @@ public static Jenkins getJenkinsInstance() throws IllegalStateException {
* Other plugins may be interested in listening for these updates.
*
* @since 1.8
* @deprecated working theory is that this API is not required any more with the {@link SCMEvent} based API,
* if wrong, please raise a JIRA
*/
@Deprecated
@Restricted(NoExternalUse.class)
Copy link
Member Author

Choose a reason for hiding this comment

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

From what I can see the only consumer of this API is the 1.x GitHub Branch Source. If there are other use cases for consumers remaining post SCM API 2.0.x updates then this would need to be augmented to pass through the origin.

I'd much rather kill off the code path, so by adding the @Restricted we will flush out any impls without breaking them until they update their dependency

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@jglick that single impl is not in GHBS 2.0.x

public abstract static class Listener implements ExtensionPoint {

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@
import java.lang.reflect.Modifier;
import javax.annotation.CheckForNull;
import jenkins.model.Jenkins;
import jenkins.scm.api.SCMEvent;
import org.jenkinsci.plugins.github.util.misc.NullSafeFunction;
import org.jenkinsci.plugins.github.util.misc.NullSafePredicate;
import org.kohsuke.github.GHEvent;
import org.kohsuke.stapler.StaplerRequest;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -115,15 +117,32 @@ private boolean checkIsApplicableItem() {
* This method called when root action receives webhook from GH and this extension is interested in such
* events (provided by {@link #events()} method). By default do nothing and can be overrided to implement any
* parse logic
* Don't call it directly, use {@link #processEvent(GHEvent, String)} static function
* Don't call it directly, use {@link #processEvent(String, GHEvent, String)} static function
*
* @param event gh-event (as of PUSH, ISSUE...). One of returned by {@link #events()} method. Never null.
* @param payload payload of gh-event. Never blank. Can be parsed with help of GitHub#parseEventPayload
* @deprecated override {@link #onEvent(String, GHEvent, String)} instead.
*/
@Deprecated
protected void onEvent(GHEvent event, String payload) {
// do nothing by default
}

/**
* This method called when root action receives webhook from GH and this extension is interested in such
* events (provided by {@link #events()} method). By default do nothing and can be overrided to implement any
* parse logic
* Don't call it directly, use {@link #processEvent(String, GHEvent, String)} static function
*
* @param origin the origin of the event (or {@code null})
* @param event gh-event (as of PUSH, ISSUE...). One of returned by {@link #events()} method. Never null.
* @param payload payload of gh-event. Never blank. Can be parsed with help of GitHub#parseEventPayload
* @since TODO
*/
protected void onEvent(String origin, GHEvent event, String payload) {
onEvent(event, payload);
}

/**
* @return All subscriber extensions
*/
Expand Down Expand Up @@ -200,13 +219,31 @@ protected boolean applyNullSafe(@Nonnull GHEventsSubscriber subscriber) {
* @param payload string content of hook from GH. Never blank
*
* @return function to process {@link GHEventsSubscriber} list. Returns null on apply.
* @deprecated use {@link #processEvent(String, GHEvent, String)}
*/
@Deprecated
public static Function<GHEventsSubscriber, Void> processEvent(final GHEvent event, final String payload) {
return processEvent(null, event, payload);
}

/**
* Function which calls {@link #onEvent(GHEvent, String)} for every subscriber on apply
*
* @param origin the origin of the event or {@code null} if the origin is unknown,
* {@link SCMEvent#originOf(StaplerRequest)} is usually the best way to generate the origin.
* @param event from hook. Applied only with event from {@link #events()} set
* @param payload string content of hook from GH. Never blank
*
* @return function to process {@link GHEventsSubscriber} list. Returns null on apply.
* @since TODO
*/
public static Function<GHEventsSubscriber, Void> processEvent(final String origin, final GHEvent event,
final String payload) {
return new NullSafeFunction<GHEventsSubscriber, Void>() {
@Override
protected Void applyNullSafe(@Nonnull GHEventsSubscriber subscriber) {
try {
subscriber.onEvent(event, payload);
subscriber.onEvent(origin, event, payload);
} catch (Throwable t) {
LOGGER.error("Subscriber {} failed to process {} hook, skipping...",
subscriber.getClass().getName(), event, t);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
import com.cloudbees.jenkins.GitHubPushTrigger;
import com.cloudbees.jenkins.GitHubRepositoryName;
import com.cloudbees.jenkins.GitHubRepositoryNameContributor;
import com.cloudbees.jenkins.GitHubTrigger;
import com.cloudbees.jenkins.GitHubWebHook;
import hudson.Extension;
import hudson.ExtensionList;
import hudson.model.Item;
import hudson.security.ACL;
import java.io.IOException;
Expand Down Expand Up @@ -64,7 +64,7 @@ protected Set<GHEvent> events() {
* @param payload payload of gh-event. Never blank
*/
@Override
protected void onEvent(GHEvent event, String payload) {
protected void onEvent(final String origin, GHEvent event, String payload) {
GHEventPayload.Push push;
try {
push = GitHub.offline().parseEventPayload(new StringReader(payload), GHEventPayload.Push.class);
Expand All @@ -74,7 +74,7 @@ protected void onEvent(GHEvent event, String payload) {
}
URL repoUrl = push.getRepository().getUrl();
final String pusherName = push.getPusher().getName();
LOGGER.info("Received PushEvent for {}", repoUrl);
LOGGER.info("Received PushEvent for {} from {}", repoUrl, origin);
final GitHubRepositoryName changedRepository = GitHubRepositoryName.create(repoUrl.toExternalForm());

if (changedRepository != null) {
Expand All @@ -85,14 +85,14 @@ protected void onEvent(GHEvent event, String payload) {
@Override
public void run() {
for (Item job : Jenkins.getInstance().getAllItems(Item.class)) {
GitHubTrigger trigger = triggerFrom(job, GitHubPushTrigger.class);
GitHubPushTrigger trigger = triggerFrom(job, GitHubPushTrigger.class);
Copy link
Member Author

Choose a reason for hiding this comment

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

Highlights the pointlessness of the GitHubTrigger interface when the only other user could avoid it completely: https://github.com/jenkinsci/github-sqs-plugin/blob/d5886f8c3669e4ca61c1c359cd216aca713c7aeb/src/main/java/com/base2services/jenkins/github/GitHubTriggerProcessor.java#L87 which doesn't need to down-cast to the interface type as it knows that the triggerClass is always SqsBuildTrigger

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(pusherName);
trigger.onPost(origin, pusherName);
Copy link
Member

Choose a reason for hiding this comment

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

Must at least catch AbstractMethodError here.

} else {
LOGGER.debug("Skipped {} because it doesn't have a matching repository.",
fullDisplayName);
Expand All @@ -102,8 +102,7 @@ public void run() {
}
});

for (GitHubWebHook.Listener listener : Jenkins.getInstance()
.getExtensionList(GitHubWebHook.Listener.class)) {
for (GitHubWebHook.Listener listener : ExtensionList.lookup(GitHubWebHook.Listener.class)) {
listener.onPushRepositoryChanged(pusherName, changedRepository);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ public void shouldReportAboutHookProblemOnUnregister() {
public void shouldResolveOnPingHook() {
monitor.registerProblem(REPO_FROM_PING_PAYLOAD, new IOException());

GHEventsSubscriber.processEvent(GHEvent.PING, classpath("payloads/ping.json")).apply(pingSubscr);
GHEventsSubscriber.processEvent(null, GHEvent.PING, classpath("payloads/ping.json")).apply(pingSubscr);

assertThat("ping resolves problem", monitor.isProblemWith(REPO_FROM_PING_PAYLOAD), is(false));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,9 @@ public void shouldParsePushPayload() throws Exception {
prj.setScm(GIT_SCM_FROM_RESOURCE);

new DefaultPushGHEventSubscriber()
.onEvent(GHEvent.PUSH, classpath("payloads/push.json"));
.onEvent("shouldParsePushPayload", GHEvent.PUSH, classpath("payloads/push.json"));

verify(trigger).onPost(TRIGGERED_BY_USER_FROM_RESOURCE);
verify(trigger).onPost("shouldParsePushPayload", TRIGGERED_BY_USER_FROM_RESOURCE);
}

@Test
Expand All @@ -68,9 +68,9 @@ public void shouldReceivePushHookOnWorkflow() throws Exception {
jenkins.assertBuildStatusSuccess(job.scheduleBuild2(0));

new DefaultPushGHEventSubscriber()
.onEvent(GHEvent.PUSH, classpath("payloads/push.json"));
.onEvent("shouldReceivePushHookOnWorkflow", GHEvent.PUSH, classpath("payloads/push.json"));

verify(trigger).onPost(TRIGGERED_BY_USER_FROM_RESOURCE);
verify(trigger).onPost("shouldReceivePushHookOnWorkflow", TRIGGERED_BY_USER_FROM_RESOURCE);
}

@Test
Expand All @@ -83,8 +83,8 @@ public void shouldNotReceivePushHookOnWorkflowWithNoBuilds() throws Exception {
job.setDefinition(new CpsFlowDefinition(classpath(getClass(), "workflow-definition.groovy")));

new DefaultPushGHEventSubscriber()
.onEvent(GHEvent.PUSH, classpath("payloads/push.json"));
.onEvent("shouldNotReceivePushHookOnWorkflowWithNoBuilds", GHEvent.PUSH, classpath("payloads/push.json"));

verify(trigger, never()).onPost(TRIGGERED_BY_USER_FROM_RESOURCE);
verify(trigger, never()).onPost("shouldNotReceivePushHookOnWorkflowWithNoBuilds", TRIGGERED_BY_USER_FROM_RESOURCE);
}
}