Adding JenkinsEventNotifier to send events back to TFS/VSTS #167
Conversation
Adding "team-events-connect" endpoint
| * - Job Completion event | ||
| */ | ||
| public class JenkinsEventNotifier { | ||
| protected final static Logger log = Logger.getLogger(JenkinsEventNotifier.class.getName()); |
There was a problem hiding this comment.
(I don't know if we care): I think Checkstyle will flag this line for the order of the keywords:
https://stackoverflow.com/questions/10299067/modifier-keyword-order-in-java
There was a problem hiding this comment.
Probably right. I will run check style and fix those issues.
There was a problem hiding this comment.
I added CheckStyle to the build but it will be in another PR since there are so many changes.
| payload.put("server", connectionParameters.getConnectionKey()); | ||
| final String jsonPayload = payload.toString(); | ||
| final JobCompletionEventArgs args = new JobCompletionEventArgs( | ||
| connectionParameters.getConnectionKey(), jsonPayload, |
There was a problem hiding this comment.
I assume you want me to put each parameter on a separate line? That makes sense.
| sb.append(url); | ||
| for(final String s : parts) { | ||
| if (StringUtils.isNotBlank(s)) { | ||
| if (sb.charAt(sb.length() - 1) != '/') { |
There was a problem hiding this comment.
Protect against an empty url?
(If url is blank, this will throw IndexOutOfBounds.)
| @Extension | ||
| public class JenkinsRunListener extends RunListener<Run> { | ||
|
|
||
| protected static Logger log = Logger.getLogger(JenkinsRunListener.class.getName()); |
There was a problem hiding this comment.
I made this change in another PR (with CheckStyle)
| sb.append(url); | ||
| for(final String s : parts) { | ||
| if (StringUtils.isNotBlank(s)) { | ||
| if (sb.charAt(sb.length() - 1) != '/') { |
There was a problem hiding this comment.
Shouldn't we check that sb.Length() > 0 ?
There was a problem hiding this comment.
absolutely, good catch.
|
|
||
| private String getStartedBy(final Run run) { | ||
| final Cause.UserIdCause cause = (Cause.UserIdCause) run.getCause(Cause.UserIdCause.class); | ||
| final String startedBy; |
There was a problem hiding this comment.
I think this would be simpler without the 'final' and the 'else' clause. Just initialize to 'null'.
There was a problem hiding this comment.
It would be less lines, but I like the final. I will see how it looks.
| try { | ||
| // Check to see if there are any collections "connected" to this Jenkins server | ||
| final ConnectionParameters connectionParameters = c.getConnectionParameters(); | ||
| if (connectionParameters == null || !connectionParameters.isSendJobCompletionEvents()) { |
There was a problem hiding this comment.
It seems to me that the method TeamCollectionConfiguration.getConnectedCollections() already does this check.
There was a problem hiding this comment.
good point. I will remove the null check and perhaps annotate the method with NotNull
|
|
||
| public void sendJobCompletionEvent(final JobCompletionEventArgs args) throws IOException { | ||
| final QueryString qs = new QueryString( | ||
| API_VERSION, "3.0-preview.1", |
There was a problem hiding this comment.
{
"id": "e0e0a1c9-beeb-4fb7-a8c8-b18e3161a50e",
"area": "hooks",
"resourceName": "externalEvents",
"routeTemplate": "_apis/public/{area}/{resource}",
"resourceVersion": 1,
"minVersion": "2.0",
"maxVersion": "4.0",
"releasedVersion": "3.2"
},
There was a problem hiding this comment.
Shouldn't we use a released API version?
There was a problem hiding this comment.
will do. Thanks for providing the version information.
| final String message = serviceHookEvent.getMessage().getText(); | ||
| final String detailedMessage = serviceHookEvent.getDetailedMessage().getText(); | ||
| final String message = serviceHookEvent.getMessage() != null ? serviceHookEvent.getMessage().getText() : ""; | ||
| final String detailedMessage = serviceHookEvent.getDetailedMessage() != null ? serviceHookEvent.getDetailedMessage().getText() : ""; |
There was a problem hiding this comment.
It looks like we do not use these messages.
There was a problem hiding this comment.
I don't see them being used either, but it would be bigger change to remove them. I only changed the code because I ran into an error here. I don't know why they were originally added.
| getPayloadSignature(connectionParameters, jsonPayload)); | ||
| client.sendJobCompletionEvent(args); | ||
| } catch (final Exception e) { | ||
| log.info("ERROR: sendJobCompletionEvent: (collection=" + c.getCollectionUrl() + ") " + e.getMessage()); |
There was a problem hiding this comment.
Maybe this log level should be "warn"? Is this error expected?
There was a problem hiding this comment.
Agreed. It is not expected. I looked for a log.error, but there wasn't one :(
Adding "team-events-connect" endpoint which allows TFS/VSTS to "connect" to the Jenkins server and start receiving events.