Skip to content
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

Allow slashes in receivers #2011

Merged

Conversation

0robustus1
Copy link
Contributor

It seems that alertmanager was splitting the Receiver in templates by the first slash and only exposing the value prior to this first slash.
I couldn't find out whether there was a particular reason for this as Receivers with slashes seem to be working well within every other part of the alertmanager.

Changes:

  • Do not split receiver by slash in templates instead expose the complete receiver
  • Percent-encode the Receiver within the default alertmanager URL.

@simonpasquier
Copy link
Member

Thanks! It looks like the code has been like that forever but I can't think of a justification for it. Maybe @stuartnelson3 has some thoughts?

@stuartnelson3
Copy link
Contributor

Can't think of anything. Seems fine to me

Copy link
Member

@simonpasquier simonpasquier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

I'm not sure why this was originally done, because a slash seems to be perfectly
valid in a receiver definition. The only issue i encountered was that linking
from a fired alert was not working, because of the splitting performed here.

Signed-off-by: Tim Reddehase <tim.reddehase@xing.com>
If Receiver is allowed to contain slashes and other special characters
we need to percent encode them properly.

Signed-off-by: Tim Reddehase <tim.reddehase@xing.com>
@simonpasquier simonpasquier force-pushed the allow-slashes-in-receivers-for-urls branch from 80c4995 to e1dbd30 Compare August 27, 2019 15:01
Signed-off-by: Tim Reddehase <tim.reddehase@xing.com>
@simonpasquier simonpasquier merged commit d517f78 into prometheus:master Aug 28, 2019
@simonpasquier
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants