plugins » flowdock-plugin #3 SUCCESS
This pull request looks good

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

Sorry for the delay in getting back to you about this pull request! Can you provide an example of tag configuration with expandable variables? I don't seem to be able to get any variables expanded from the tag string (tried with: ${JOB_NAME}, ${BUILD_NUMBER}, etc). Those tags seem to arrive as they are to the Flowdock API.

@anttipitkanen that looks like a state from before merging the code in this PR. I've been using this modification for some time now and it expands every macro I put there, which among others includes ${BUILD_NUMBER}.

I think I found the issue: the token-macro plugin is marked as optional dependency, but in order to send the notification successfully it needs to be installed. To resolve this, there should be some fallback for the cases where the user has not installed the plugin (documentation: https://wiki.jenkins-ci.org/display/JENKINS/Dependencies+among+plugins).

Another way would be marking the plugin as required dependency, but that's not really desirable solution.

On a side note, I would love to avoid the "catch-all" in the notifyFlowdock method since it loses stack traces and other information about exceptions that are really unexpected and should be rather allowed to bubble up. Catching only MacroEvaluationException would be optimal in my opinion since that's something we expect to see and can handle there.

@anttipitkanen I have applied your suggestions. Please review.

A couple of things I found while testing the latest version:

If the token-macro plugin does not exist, you'll run into java.lang.ClassNotFoundException: org.jenkinsci.plugins.tokenmacro.MacroEvaluationException as the try-catch uses it. I guess there's no pretty way around that except for catching just Exception or maybe loading the class manually inside the plugin conditional. I would probably go for the former as it's easier to implement and the "catch-all" only affects the expandAll method call.

The token-macro plugin is fetched and assigned in the constructor which seems to result in value null for me every time with Jenkins v1.580. However, if I move it to the notifyFlowdock just before the try-catch, it finds the plugin nicely and uses it to expand macros.

This was implemented without plugin dependencies in pull request #8

That's slightly different stuff - it expands only environment variables. Token macros are slightly more useful.