notify/telegram: Set API URL and ParseMode defaults#2879
notify/telegram: Set API URL and ParseMode defaults#2879metalmatze wants to merge 3 commits intoprometheus:mainfrom
Conversation
Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>
gotjosh
left a comment
gotjosh
left a comment
There was a problem hiding this comment.
LGTM
Can you please fix the title of the PR so that it specifies that what we're fixing is the URL?
From notify/telegram: Set API and ParseMode defaults to Telegram: Set API URL and ParseMode defaults
notify/telegram/telegram.go
Outdated
| } | ||
|
|
||
| if conf.APIUrl == nil { | ||
| apiURL, err := url.Parse("https://api.telegram.org") |
There was a problem hiding this comment.
This should be done during unmarhalling of the configuration, not when calling New.
There was a problem hiding this comment.
Good shout Julien.
On a second look at
alertmanager/config/notifiers.go
Lines 140 to 147 in 71d61c9
It seems like both of these belong within DefaultTelegramConfig
There was a problem hiding this comment.
nit really because the API should come from DefaultGlobalConfig
notify/telegram/telegram.go
Outdated
| if conf.ParseMode == "" { | ||
| // The default Telegram template is in HTML, | ||
| // so we need to set the parse mode to HTML for Telegram not to return a parse error. | ||
| conf.ParseMode = "HTML" |
There was a problem hiding this comment.
nit: Can this also be a const? Perhaps a suggestion is defaultParseMode = "HTML"
There was a problem hiding this comment.
This should be done during unmarhalling of the configuration, not when calling New.
There was a problem hiding this comment.
Our current default is actually MarkdownV2 this would effectively be a breaking change and I don't think it was the intent of #2827.
Looking at the current template - I don't see how it is HTML, am I missing something? https://github.com/prometheus/alertmanager/blob/main/template/default.tmpl#L105-L114
gotjosh
left a comment
gotjosh
left a comment
There was a problem hiding this comment.
On a second look, it seems like some parts of these are not correct.
I suggest we split this into two PRs.
-
The default URL is correct and seems straightforward - we need to add it to the docs though.
-
The default parse mode is currently Markdown, and setting the default to HTML would be a breaking change based on what's currently here.
notify/telegram/telegram.go
Outdated
| } | ||
|
|
||
| if conf.APIUrl == nil { | ||
| apiURL, err := url.Parse("https://api.telegram.org") |
There was a problem hiding this comment.
Good shout Julien.
On a second look at
alertmanager/config/notifiers.go
Lines 140 to 147 in 71d61c9
It seems like both of these belong within DefaultTelegramConfig
notify/telegram/telegram.go
Outdated
| if conf.ParseMode == "" { | ||
| // The default Telegram template is in HTML, | ||
| // so we need to set the parse mode to HTML for Telegram not to return a parse error. | ||
| conf.ParseMode = "HTML" |
There was a problem hiding this comment.
Our current default is actually MarkdownV2 this would effectively be a breaking change and I don't think it was the intent of #2827.
Looking at the current template - I don't see how it is HTML, am I missing something? https://github.com/prometheus/alertmanager/blob/main/template/default.tmpl#L105-L114
|
Actually the API URl should come from DefaultGlobalConfig. i do not know why it is not the case. |
metalmatze
changed the title
|
I've looked into the parseMode issue again. It seems that Telegram doesn't allow the
Most notably the alertmanager/template/default.tmpl Lines 8 to 10 in f958b8b Unless we want to change the default template for all receivers, we should probably make |
Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>
Signed-off-by: Matthias Loibl <mail@matthiasloibl.com>
|
Not sure why the tests fail. I can try rebasing later. |
|
Once you rebase this should be fixed - it relates to #2899 |
3 participants
For Telegram, there is really just one API URL, and other than for testing I never saw anything changing it. Therefore, as asked in #2876 we can set the default.
The default Telegram template that's shipped with Alertmanager uses HTML and if send to Telegram without the parseMode set to
HTMLTelegram's API will return an error:Closes #2866
Closes #2876