-
Notifications
You must be signed in to change notification settings - Fork 34
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
Slack: assume any non-JSON response is an incoming webhook #67
Slack: assume any non-JSON response is an incoming webhook #67
Conversation
Slack returns these responses as text/html while Mattermost returns them as text/plain. To support both, we need to either add another clause to the if: || strings.HasPrefix(content, "text/plain") either reverse the current logic, as does this commit.
@yuri-tceretian @grobinson-grafana Can you please take a look into this pull request to get notifications working again correctly in Mattermost? |
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.
LGTM. Will leave to @grobinson-grafana to give a formal approval as we apply the practice of two reviewers per PR. Thanks for contribution!
I think this will need to be backported to 9.4.x branch after this one is merged. I can take care of it. |
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! I also checked chat.postMessage
and it returns application/json
so this should work as expected!
* Fix typo in test name * TestSendSlackRequest: test with content-type header * Slack: assume any non-JSON response is an incoming webhook. Slack returns these responses as text/html while Mattermost returns them as text/plain. # Conflicts: # alerting/notifier/channels/slack_test.go
* Fix typo in test name * TestSendSlackRequest: test with content-type header * Slack: assume any non-JSON response is an incoming webhook. Slack returns these responses as text/html while Mattermost returns them as text/plain.
…) (#84) Co-authored-by: Baptiste Fontaine <b@ptistefontaine.fr>
Slack returns these responses as
text/html
while Mattermost returns them astext/plain
(see grafana/grafana#64079 (comment)). To support both, we need to either add another clause to the if (|| strings.HasPrefix(content, "text/plain")
), either reverse the current logic, as does this PR.I also added tests on the
Content-Type
header as they were missing. This fixes grafana/grafana#64079.