-
Notifications
You must be signed in to change notification settings - Fork 411
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
Fixes issue #125 where this plugin is nested inside another publisher #361
Conversation
I'm highly interested in this PR. What can I do to get it merged quicker? |
I'm very interested in this as well |
Hi, |
I'd also love to have this reviewed & merged.... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
Really wanting this to be available, when is 2.4 going to be released? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Without this, Notifier#perform runs the Slack message publish code too early, resulting in messages like this: ``` test - jenkinsci#68 Success after 6.9 sec and counting (Open) ``` `isBuilding()` should be true in this instance. Instead, always run the plugin with `runAfterFinalised=true.` This restores the older behavior, (before jenkinsci#361, jenkinsci#125) where SlackListener run the publish code after `isBuilding()` was false and the end message was: ``` test - jenkinsci#80 Success after 6 sec (Open) ```
Without this, Notifier#perform runs the Slack message publish code too early, resulting in messages like this: ``` test - #68 Success after 6.9 sec and counting (Open) ``` `isBuilding()` should be true in this instance. Instead, always run the plugin with `runAfterFinalised=true.` This restores the older behavior, (before #361, #125) where SlackListener run the publish code after `isBuilding()` was false and the end message was: ``` test - #80 Success after 6 sec (Open) ```
This is a fix for notifications failing when the Slack publisher is nested within another post-build action such as a conditional action or a template.
Unless I'm missing something it seems unnecessary to have a separate listener for build events outside of the core Notifier implementation. This may simply have been here for legacy reasons, but it makes it difficult to identify which Publisher(s) to run.
I'm going to run a few more tests on this locally in case I'm missing something obvious and breaking anything, but would welcome any comments.