Alerting: Fix configuration of both incoming webhooks and bot tokens #69
Conversation
This commit fixes a bug in Slack where it was possible to configure both incoming webhooks and bot tokens at the same time. This is a bug as the intended behaviour is that users configure one or the other, but not both.
| @@ -3,7 +3,6 @@ package slack | |||
| // FullValidConfigForTesting is a string representation of a JSON object that contains all fields supported by the notifier Config. It can be used without secrets. | |||
| const FullValidConfigForTesting = `{ | |||
There was a problem hiding this comment.
I'm not sure what to do with these Yuri. Should I create one for incoming webhooks and another for bot tokens?
There was a problem hiding this comment.
I would suggest to update this one and create another test with another combination of fields
| @@ -256,7 +255,7 @@ func TestNewConfig(t *testing.T) { | |||
| settings: FullValidConfigForTesting, | |||
| expectedConfig: Config{ | |||
| EndpointURL: "http://localhost/endpoint_url", | |||
| URL: "http://localhost/url", | |||
| URL: "http://localhost/endpoint_url", | |||
There was a problem hiding this comment.
This was changed to make the tests pass, but I think these tests need to be changed as they are based on the assumption you can configure both incoming webhooks and bot tokens at the same time.
|
I'm not certain this is the way to go here, we have all the necessary information to send a notification correctly. Instead of erroring when both a URL and a token are provided, we could instead log a warning and use either the token or the webhook url (author's choice). This is obviously niche since UI validation should prevent this in the first place, but if they do happen to get into this state (like in the escalation) maybe warning + continue as normal is better than error. What do you think? |
Once Slack is configured we don't know if its a token or a webhook because the URL field is re-used when using a webhook and the bot token. For example, when using an incoming webhook the URL is |
This commit fixes a bug in Slack where it was possible to configure both incoming webhooks and bot tokens at the same time. This is a bug as the intended behaviour is that users configure one or the other, but not both.
Fixes #68
Support escalation: https://github.com/grafana/support-escalations/issues/5309