Adding configurable webhook endpoint for exposing jenkins commands with a Slack outgoing-webhook #160

dpires · 2015-11-23T17:27:27Z

I've added an endpoint extension (/webhook/) to allow Slack outgoing-webhook functionality.

There is a new Slack Webhook Settings configuration option to add an outgoing-webhook token.

I have placed everything in a new jenkins.plugins.slack.webhook package.

samrocketman · 2015-11-23T17:55:22Z

Thanks for this contribution. I'm going to open this up for code review from others.

jenkinsadmin · 2015-12-28T18:01:23Z

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

OwensCode · 2016-01-06T18:56:55Z

pom.xml

@@ -55,6 +60,11 @@
            <version>20131018</version>
        </dependency>
        <dependency>
+            <groupId>org.hamcrest</groupId>
+            <artifactId>hamcrest-all</artifactId>


I think this dependency can be scoped to test.

OwensCode · 2016-01-06T19:15:17Z

My experience is that the build time increases from about 20-30 seconds to 1:20-1:30 seconds with these changes, and most of it is spent running tests. It's not a crucial issue, but it is likely to become a frustration for developers, particularly if you like following a TDD approach. Is there any specific reason for the extra time, and can it be reduced (without reducing test coverage)? I thought it was possibly related to having to start up a Jenkins instance, but I also have that happening in another branch I'm working on, and it builds in around 30 seconds.

dpires · 2016-01-06T19:47:57Z

I can see the test time being an issue. The reasoning is that the i'm waiting for projects to be created and builds to finish with a sleep.

I'll look into using a Future callback for those tests, although the outcome is probably going to be the same time.

OwensCode · 2016-01-06T21:01:43Z

I'm not that familiar with the outgoing webhooks integration. I assume that these URLs on the Jenkins server would need to be accessible from the slack servers for this to work, right? Just trying to get my head around the general idea and how it would be used.

dpires · 2016-01-06T21:40:03Z

Yes, Slack sends a POST to an endpoint which is why this /webhook endpoint is unprotected as Slack controls the body of the request so you can't add additional authorization parameters etc.

I have usage description in my original plugin before I created this PR:

https://github.com/dpires/jenkins-slack-webhook-plugin

OwensCode · 2016-01-07T22:43:29Z

src/main/java/jenkins/plugins/slack/webhook/WebhookEndpoint.java

+        globalConfig = GlobalConfiguration.all().get(GlobalConfig.class);
+    }
+
+    @Override


The pom file specifies that the compiler should target JDK 1.5, which actually means that these @OVERRIDES shouldn't be allowed because they're implementations of an interface rather than overrides of a parent class, which was only allowed from 1.6 on. I definitely prefer to have the annotation, and it works for building because our more modern compilers aren't strictly enforcing it, but my IDE doesn't like them.

However, why are we stuck using 1.5? I followed the path of where it comes from:

org.jenkins-ci.plugins.slack pom.xml --> org.jenkins-ci.plugins.plugin 1.567 -> org.jenkins-ci.jenkins 1.33

org.jenkins-ci.jenkins 1.33 was last updated in 2013, so maybe we can move to the latest org.jenkins-ci.plugins.plugin. Even that will only give us 1.6, which isn't a supported version of Java anymore.

Not sure this has anything to do with this review, but @samrocketman do you know if there are any plans to move Jenkins in general to a newer version of Java? Otherwise, maybe the next thing I can work on is updating these versions to the latest.

The Jenkins project seems to want Java 1.7 and later. ref https://jenkins-ci.org/blog/2015/04/06/good-bye-java6/

Here's a link to another plugin I support updating the compatibility.

jenkinsci/github-oauth-plugin#42

OwensCode · 2016-01-08T17:01:16Z

Either an alternative way to implement this, or an idea for a future feature: send messages over Amazon SQS instead of posting over http directly to the Jenkins server.

https://slack.com/apps/A0F827G56-amazon-sqs
http://aws.amazon.com/sqs/
https://wiki.jenkins-ci.org/display/JENKINS/GitHub+SQS+Plugin

I know my company would be more interested in this route.

dpires · 2016-01-08T17:38:15Z

You can also use a Slack bot like Hubot or Lita to implement something similar to this PR, however this is the simplest solution as it requires zero dependencies/integrations.

dpires · 2016-01-08T20:45:33Z

I've modified the endpoint to be configurable. Without setting the endpoint Jenkins will return a standard 404 like any other unknown endpoints.

OwensCode · 2016-01-08T22:17:58Z

src/main/java/jenkins/plugins/slack/webhook/WebhookEndpoint.java

+
+    @Override
+    public String getUrlName() {
+        return "/"+globalConfig.getSlackOutgoingWebhookURL();


Much better. Thanks for the changes. Looking at jenkins.model.Jenkins.getDynamic(String) which calls this, it would be better to return null when the url from the configuration is null/empty.

public Object getDynamic(String token) { for (Action a : getActions()) { String url = a.getUrlName(); if (url==null) continue; if (url.equals(token) || url.equals('/' + token)) return a; } // etc. return null; }

Good find. Done.

OwensCode · 2016-01-11T14:43:33Z

src/main/resources/jenkins/plugins/slack/webhook/GlobalConfig/help-slackOutgoingWebhookURL.html

@@ -0,0 +1,5 @@
+<div>
+This is the webhook endpoint to send requests to.


I think it would be good to point out here that leaving it blank will disable the functionality, and that the endpoint needs to be publicly accessible from the Slack servers. I think that will alleviate some concerns, and clarify why it isn't working when someone hasn't opened their firewall etc.

OwensCode · 2016-01-12T01:21:17Z

src/main/java/jenkins/plugins/slack/webhook/WebhookEndpoint.java

+                triggerWord+" list projects", "Return a list of buildable projects",
+                this,
+                "listProjects")
+            .addRoute("^"+triggerWord+" run ([a-zA-Z0-9_\\-\\.]+)",


Commands work well, but do not support projects that have spaces in the name. Not sure if there are other characters that Jenkins allows that are not accepted here.

dpires · 2016-01-12T03:26:33Z

I've updated the command regex to support all valid/safe characters (and charsets) for a project name and updated the tests.

dpires · 2016-01-18T20:56:05Z

Can this be added as a merge candidate?

OwensCode · 2016-01-19T13:30:38Z

I've been through most of the code in this pull request and @dpires has fixed everything I've commented on so far. Will try to get through the rest of it asap, but it's looking good so far. Not sure what it takes to make it a merge candidate.

OwensCode · 2016-01-19T13:53:30Z

src/main/java/jenkins/plugins/slack/webhook/WebhookEndpoint.java

+@Extension
+public class WebhookEndpoint implements UnprotectedRootAction {
+
+    private String slackUser;


Just noticed that it looks like WebhookEndpoint is a singleton; there is only ever 1 instantiated per Jenkins instance. If I'm correct, you have a race condition on the slackUser member variable. I think you'll need to pass it around in method calls instead of using a member variable. Maybe you could encapsulate all the required information in an object.

The globalConfig seems fine. I was concerned it would not dynamically pickup configuration changes, but it does.

Good find, that is a race condition.

OwensCode · 2016-01-27T14:57:40Z

src/main/java/jenkins/plugins/slack/webhook/CommandRouter.java

+        return message;
+    }
+
+    private  Method getHandlerMethod(Object handlerInstance,


This method is not being used after the class was refactored.

OwensCode · 2016-01-27T18:52:57Z

This is getting really close to getting a thumbs up from me. There are a few unaddressed comments (none of which are critical) and I'd like to see if I can get the test to run a little faster, but otherwise it's looking good.

dpires · 2016-01-27T20:31:31Z

I've added a condensed project list when projects are > 10. Still has all information, just without 3 line breaks.

I've tried it with 30+ projects and it's not that bad, still readable.

…th a Slack outgoing-webhook

OwensCode · 2016-01-28T22:32:59Z

I think @dpires has addressed all the concerns I raised. Thanks! @samrocketman this pull request looks good to me. The only thing that would be nice to improve is the run time for the unit tests, but it's not terrible and I'm not sure whether it is actually possible to improve it substantially or not. Let me know if I can do anything more to help get this merged.

Adding configurable webhook endpoint for exposing jenkins commands with a Slack outgoing-webhook

OwensCode · 2016-03-08T14:46:18Z

Thanks @kmadel!

kmadel · 2016-03-16T10:17:33Z

@dpires There appears to be certain Jenkins configurations where your changes cause a NPE:

Caused by: java.lang.NullPointerException
    at jenkins.plugins.slack.webhook.WebhookEndpoint.getUrlName(WebhookEndpoint.java:50)
    at jenkins.model.Jenkins.getDynamic(Jenkins.java:2892)
    ... 83 more

May be related to #189
Would you be able to update with a default value - say slack-webhook - that may be overridden from the config already there?

yoanisgil · 2016-03-16T12:56:56Z

@dpires @kmadel those of us who upgraded to version 2.0 are experiencing a NPE. See here for details:

https://issues.jenkins-ci.org/browse/JENKINS-33556

the solution is either to downgrade to version 1.8.1 or set a for the Outgoing Webhook URL Endpoint setting. We should probably track this on a different issue. Let me know if you want me to create one.

yoanisgil · 2016-03-16T13:04:06Z

src/main/java/jenkins/plugins/slack/webhook/WebhookEndpoint.java

+    public String getUrlName() {
+        String url = globalConfig.getSlackOutgoingWebhookURL();
+        if (url == null || url.equals(""))
+            return null;


I think it needs to return a default value other wise it will cause a NPE here:

https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/jenkins/model/Jenkins.java#L4295

Issue reported here: https://issues.jenkins-ci.org/browse/JENKINS-33556

OwensCode · 2016-03-16T13:21:24Z

@dpires originally had a default value for the endpoint, but when I reviewed the code I was concerned that some administrators might not like that this is enabled by default as they might see it as a security risk. The version of Jenkins we tested against specifically skipped endpoints that returned null for the URL, which is why @dpires ended up taking the current approach. Hopefully there is an alternative to conditionally enabling the endpoint, otherwise a quick fix that would maybe be okay is to default it to a randomly generated string.

ssbarnea · 2016-03-16T14:32:18Z

The NPE was a really upsetting surprise on our instance. Please remove version 2.0.0 from jenkins plugin directory until you fix it!

dpires · 2016-03-16T15:41:01Z

I've changed the url to a random uuid string if the endpoint is not configured. See pr #190

samrocketman · 2016-03-16T22:29:09Z

@ssbarnea unfortunately I'm not sure I have access to remove it. Jenkins synchronizes the update center every 8 hours roughly.

samrocketman · 2016-03-16T22:30:25Z

The web endpoint can be disabled permanently with a script console script I crafted.

https://github.com/samrocketman/jenkins-script-console-scripts/blob/master/disable-slack-webhooks.groovy

Blaisorblade · 2016-04-29T18:25:51Z

@dpires I found out about this because, even after the fix, this feature got in my way with its bad UI.
I don't care for the feature, but with this (version 2.0.1), I get a warning that I need to set the fill in the extra configuration, without mentioning this is entirely optional.

TL; DR. That warning sign is spam, stop using it.

What's worse, docs are missing enough context that to figure out that I can ignore this whole thing, I needed to figure it out in full by skimming this pull request (spending >= half an hour). (In case you wonder, I'm no newbie).

I was not looking to use this feature. I was just reviewing my settings after upgrading the Slack plugin, because the plugin update form told me that the configuration format changed, so I feared I would need to setup the connection from scratch.

After upgrading to 2.0.1, my "Manage Jenkins" page has a section "Slack Webhook Settings" which tells me (in yellow, with warning signs) "Please set a Slack outgoing webhook token" and "Please set a url endpoint".

I only want to keep using the existing integration with Slack. Do I now need to fill those as well? The GUI suggests yes, but it seems that I should ignore them unless I need some unspecified functionality.

This is not documented here: https://wiki.jenkins-ci.org/display/JENKINS/Slack+Plugin
The docs for these settings aren't very helpful.
This seems related to https://github.com/dpires/jenkins-slack-webhook-plugin, and its README doesn't state when I would need it.
On reflection, it seems that when the plugin README says "the following commands are available", it means "they're available in Slack". And the whole point is to "allow triggering Jenkins builds from inside Slack", something entirely optional.
Docs are awfully incomplete.
1. Apparently I need to divine that to configure outgoing webhooks in Slack I need to add a Slack app, otherwise good luck finding the settings.
2. Where do I define the name of the webhook endpoint? It seems that the name is defined in this plugin config, and is an arbitrary string that is not already used by some other URL (so why is it configurable instead of being fixed to slackwebhook)?

In fairness, lots of things in Jenkins show little care for UX, but this was pretty bad.

OwensCode · 2016-04-30T22:23:50Z

@Blaisorblade these are good points and somebody else did remark that it would work better with a checkbox for enabling the feature (with it disabled by default). I think that would have alleviated your confusion because you would have been able to ignore it more easily. I think I'd be interested in making that improvement (if nobody else does in the mean time) if we can get a few other things moving first.

I did want to point out that all this work is voluntary; people spending time to contribute something they feel will be valuable to others. @dpires invested his own time to add this feature and others agreed it could be useful.

Blaisorblade · 2016-05-02T06:22:35Z

@OwensCode Thanks for your kind answer.

I did want to point out that all this work is voluntary; people spending time to contribute something they feel will be valuable to others. @dpires invested his own time to add this feature and others agreed it could be useful.

Well, sorry for the tone of my comment. The UX was frustrating, but venting that frustration to developers who didn't realize it is a bad habit, I'm trying to stop.

a checkbox for enabling the feature (with it disabled by default). I think that would have alleviated your confusion because you would have been able to ignore it more easily

I agree. Better descriptions would also help, but writing for somebody who knows less can be hard, so I guess I should contribute those.
Also, I guess it'd be better to have this checkbox (and subordinate options) together with the main ones. Somehow, options for the Slack plugin webhook are totally separate from options for the Slack plugin, and come before.

samrocketman added the code review requested label Nov 23, 2015

OwensCode reviewed Jan 6, 2016
View reviewed changes

OwensCode reviewed Jan 7, 2016
View reviewed changes

dpires force-pushed the master branch from 27cc45a to 94757fb Compare January 8, 2016 21:02

dpires changed the title ~~Adding webhook/ endpoint for exposing jenkins commands with a Slack outgoing-webhook~~ Adding configurable webhook endpoint for exposing jenkins commands with a Slack outgoing-webhook Jan 8, 2016

OwensCode reviewed Jan 8, 2016
View reviewed changes

dpires force-pushed the master branch from 94757fb to 1579940 Compare January 8, 2016 22:45

OwensCode reviewed Jan 11, 2016
View reviewed changes

dpires force-pushed the master branch from 1579940 to aed97bb Compare January 11, 2016 18:59

OwensCode reviewed Jan 12, 2016
View reviewed changes

dpires force-pushed the master branch from aed97bb to 374c2d2 Compare January 12, 2016 03:25

dpires force-pushed the master branch from 374c2d2 to 2e76591 Compare January 13, 2016 04:10

OwensCode reviewed Jan 19, 2016
View reviewed changes

OwensCode reviewed Jan 27, 2016
View reviewed changes

dpires force-pushed the master branch from f40bd09 to 955c032 Compare January 27, 2016 20:07

dpires force-pushed the master branch 2 times, most recently from a4934d8 to abacc96 Compare January 28, 2016 17:22

Adding configurable webhook endpoint for exposing jenkins commands wi…

c5a45e0

…th a Slack outgoing-webhook

dpires force-pushed the master branch from abacc96 to c5a45e0 Compare January 28, 2016 20:43

kmadel added the candidate to merge label Mar 7, 2016

kmadel added this to the slack-2.0 milestone Mar 7, 2016

kmadel added a commit that referenced this pull request Mar 7, 2016

Merge pull request #160 from dpires/master

734d4bc

Adding configurable webhook endpoint for exposing jenkins commands with a Slack outgoing-webhook

kmadel merged commit 734d4bc into jenkinsci:master Mar 7, 2016

yoanisgil reviewed Mar 16, 2016
View reviewed changes

samrocketman mentioned this pull request Mar 16, 2016

Upgrading to Slack 2.0 outbound webhooks breaks Jenkins [replicated] #191

Closed

samrocketman mentioned this pull request Apr 30, 2017

Slack Webhook Settings #281

Closed

timja mentioned this pull request Aug 18, 2019

Remove slack outgoing webhook functionality #613

Merged

		@@ -0,0 +1,5 @@
		<div>
		This is the webhook endpoint to send requests to.

Adding configurable webhook endpoint for exposing jenkins commands with a Slack outgoing-webhook #160

Adding configurable webhook endpoint for exposing jenkins commands with a Slack outgoing-webhook #160

Conversation

dpires commented Nov 23, 2015

samrocketman commented Nov 23, 2015

jenkinsadmin commented Dec 28, 2015

Choose a reason for hiding this comment

OwensCode commented Jan 6, 2016

dpires commented Jan 6, 2016

OwensCode commented Jan 6, 2016

dpires commented Jan 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwensCode commented Jan 8, 2016

dpires commented Jan 8, 2016

dpires commented Jan 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpires commented Jan 12, 2016

dpires commented Jan 18, 2016

OwensCode commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwensCode commented Jan 27, 2016

dpires commented Jan 27, 2016

OwensCode commented Jan 28, 2016

OwensCode commented Mar 8, 2016

kmadel commented Mar 16, 2016

yoanisgil commented Mar 16, 2016

Choose a reason for hiding this comment

OwensCode commented Mar 16, 2016

ssbarnea commented Mar 16, 2016

dpires commented Mar 16, 2016

samrocketman commented Mar 16, 2016

samrocketman commented Mar 16, 2016

Blaisorblade commented Apr 29, 2016 • edited Loading

OwensCode commented Apr 30, 2016

Blaisorblade commented May 2, 2016

Blaisorblade commented Apr 29, 2016 •

edited

Loading