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

Pushover: support HTML, URL title and custom sounds #1634

Merged
merged 7 commits into from
Dec 18, 2018
Merged

Pushover: support HTML, URL title and custom sounds #1634

merged 7 commits into from
Dec 18, 2018

Conversation

tdabasinskas
Copy link
Contributor

Extending Pushover integration to allow extended customization:

As for the last one, I've enabled HTML support for all the messages, as I don't really see the reason why not to do so (either way, you need to use explicit HTML tags within your message to use this functionality). Anyhow, if you think otherwise, I'm good with making this configurable as well.

@tdabasinskas tdabasinskas changed the title Extend Pushover support Pushover: support HTML, URL title and custom sounds Nov 22, 2018
@simonpasquier
Copy link
Member

message should be rendered through tmplHTML() rather than tmpl(). Also I suspect that you need to add pushover.default.url_title and pushover.default.sound to template/default.tmpl.

@tdabasinskas
Copy link
Contributor Author

Hi @simonpasquier ,

Thanks for the comments.

OK, since you mentioned HTML message contents should be handled via tmplHTML, I've decided to add optional (defaulting to true) HTML parameter for this notifier. Now, if the parameter is enabled, we use tmplHTML() to render the message; otherwise - we use tmpl().

Regarding adding the default sound and url_title values to the default template, I considered doing that, but I think it's better to leave these empty by default, which means the user's default sound will be used and the title of the URL will be the same as the URL itself.

Please let me know if any further changes are required.

@simonpasquier
Copy link
Member

Regarding adding the default sound and url_title values to the default template, I considered doing that, but I think it's better to leave these empty by default, which means the user's default sound will be used and the title of the URL will be the same as the URL itself.

The problem is that pushover.default.url_title and pushover.default.sound aren't defined in the default template so the notification will fail without additional configuration. I suggest that you leave the default values as ''.

Signed-off-by: Tomas Dabasinskas <tomas@dabasinskas.net>
Signed-off-by: Tomas Dabasinskas <tomas@dabasinskas.net>
Signed-off-by: Tomas Dabasinskas <tomas@dabasinskas.net>
Signed-off-by: Tomas Dabasinskas <tomas@dabasinskas.net>
Signed-off-by: Tomas Dabasinskas <tomas@dabasinskas.net>
Signed-off-by: Tomas Dabasinskas <tomas@dabasinskas.net>
@tdabasinskas
Copy link
Contributor Author

Hi @simonpasquier ,

I've removed the default values for these two fields and confirmed it's working when they are not set.

Anyhow, it seems that the travics-ci job is failing and I'm not sure why.

@simonpasquier
Copy link
Member

It was a transient Travis issue, I've triggered the tests and it's all green now. I'd prefer to have text-only notifications as the default to stick with what existed before. @mxinden @stuartnelson3 might have another opinion though.

@mxinden
Copy link
Member

mxinden commented Dec 9, 2018

@mxinden @stuartnelson3 might have another opinion though.

No opinion on this from my side.

@tdabasinskas
Copy link
Contributor Author

Hi, @simonpasquier,

Are we still pending on something to have this merged?

@simonpasquier
Copy link
Member

Lets wait whether or not @stuartnelson3 is fine with notifications being sent as HTML by default.

@stuartnelson3
Copy link
Contributor

I have the same opinion as @simonpasquier. Even though we haven't reached a major version release yet, it would be nice to maintain behavior when possible, to prevent surprises for users. Since users have to explicitly write HTML tags in their messages, they can also opt-in to enable HTML at that point.

As per the comments provided, keeping the HTML disabled by default to keep the previous behavior.

Signed-off-by: Tomas Dabasinskas <tomas@dabasinskas.net>
@tdabasinskas
Copy link
Contributor Author

@simonpasquier , @stuartnelson3 ,

As requested, I've made the HTML to be disabled by default.

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.

👍

@stuartnelson3 stuartnelson3 merged commit cfc0d9c into prometheus:master Dec 18, 2018
@tdabasinskas tdabasinskas deleted the pushover-extend branch December 18, 2018 14:27
mxinden pushed a commit to mxinden/alertmanager that referenced this pull request Jan 14, 2019
* Support HTML inside Pushover message

Signed-off-by: Tomas Dabasinskas <tomas@dabasinskas.net>
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.

4 participants