-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: implement webhook_url_file for discord and msteams #3555
Conversation
implements prometheus#3482 Signed-off-by: Philipp Born <git@pborn.eu>
implements prometheus#3536 Signed-off-by: Philipp Born <git@pborn.eu>
We would love to have this feature in the next release, @simonpasquier / @gotjosh could you maybe take a look? |
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.
I'm not a Alertmanager maintainer, so I cannot merge this - but in general LGTM! Nice work! 👍
if n.conf.WebhookURL != nil { | ||
url = n.conf.WebhookURL.String() | ||
} else { | ||
content, err := os.ReadFile(n.conf.WebhookURLFile) |
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.
A bit of a nit, but perhaps call this b
instead of content
. Two reasons:
- It's short.
- It's a somewhat idiomatic way of telling other maintainers that this variable is a byte slice instead of a string without having to check the docs for
os.ReadFile
.
content, err := os.ReadFile(n.conf.WebhookURLFile) | |
b, err := os.ReadFile(n.conf.WebhookURLFile) |
|
||
f, err := os.CreateTemp("", "webhook_url") | ||
require.NoError(t, err, "creating temp file failed") | ||
_, err = f.WriteString(u.String() + "\n") |
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.
Good test, to check for removing whitespace! 👍
if n.conf.WebhookURL != nil { | ||
url = n.conf.WebhookURL.String() | ||
} else { | ||
content, err := os.ReadFile(n.conf.WebhookURLFile) |
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.
content, err := os.ReadFile(n.conf.WebhookURLFile) | |
b, err := os.ReadFile(n.conf.WebhookURLFile) |
@tamcore Do you have time to implement the proposed changes? |
@dbluxo early next week maybe. If there'd be an official word about approval. But it should maybe also be considered, that the current code more or less lines up with the other receiver implementations. The proposed changes seem to make sense - but I feel like having it uniform might be preferable. |
@tamcore Thanks for your response, I think being consistent with the other receivers is a valid approach. |
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
Thank you so much for your contribution this looks great - @grobinson-grafana comments are very valid, but as you said this aligns with other notifiers so I'm good with this.
…3555) * feat: implement webhook_url_file for discord implements prometheus#3482 Signed-off-by: Philipp Born <git@pborn.eu> * feat: implement webhook_url_file for msteams implements prometheus#3536 Signed-off-by: Philipp Born <git@pborn.eu> --------- Signed-off-by: Philipp Born <git@pborn.eu> Signed-off-by: Gokhan Sari <gokhan@sari.me>
Closes #3536 and #3482.