add baseUrl parameter to support slack-compatible integrations #204
Conversation
|
Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests. |
|
I'm not very familiar with Jenkins plugins, but as I'm interested in the feature, I just reviewed the PR. Looks very good to me. 👍 |
| final String sendAs, final boolean startNotification, final boolean notifyAborted, final boolean notifyFailure, | ||
| final boolean notifyNotBuilt, final boolean notifySuccess, final boolean notifyUnstable, final boolean notifyBackToNormal, | ||
| final boolean notifyRepeatedFailure, final boolean includeTestSummary, CommitInfoChoice commitInfoChoice, | ||
| boolean includeCustomMessage, String customMessage) { | ||
| super(); | ||
| this.baseUrl = baseUrl; |
There was a problem hiding this comment.
Perhaps trim off potential spaces. Just trimming here should be enough for other instances where SlackNotifier is used.
|
👎 for the issues I pointed out. Other than that, looks good. |
|
Hi, I followed exactly how other parameters are handled. There is no trimming there, so I can either leave it without trimming, or we should trim all of them? |
|
I would say correct the trimming associated with this change and then a second pull request can be created to address trimming all of the other values. In general, I prefer to trim and sanitize human input as much as possible. I'll defer to @kmadel for the actual merge. |
|
👍 works. Thanks |
|
Needs help text. Also, and example in the README would be nice. |
|
👍 Any news on this? |
|
I plan on performing a release this weekend. If you would like this PR to be in it then please rebase/resolve. Also, please add help text for the option so users know what it is for. |
|
This PR is superseded by #293. |
Solution for #174 - additional configuration parameter to specify the full URL (before token) to support slack clones with compatible API, e.g. rocket.chat.