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

feat(alerting): add email and click action to ntfy provider #778

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

stendler
Copy link

@stendler stendler commented May 26, 2024

Summary

Adds the email and click properties to the ntfy provider.

  • email - will optionally send the notification also to this email
  • click - will optionally add a link to the notification to be clicked on (e.g., a link to the gatus instance)

Documentation to publishing on ntfy via JSON: https://docs.ntfy.sh/publish/#publish-as-json

Edit: Now also adds the properties firebase and cache, which currently cannot be defined in JSON but as http headers (binwiederhier/ntfy#1119).

Checklist

  • Tested and/or added tests to validate that the changes work as intended, if applicable.
    • manually tested configurations with and without these properties
  • Updated documentation in README.md, if applicable.

@stendler
Copy link
Author

stendler commented May 26, 2024

There are also other properties supported by ntfy, which are not implemented yet:

  • actions to define custom action buttons for the notificaton
  • attach - URL to an attachment (sounds less useful for gatus)
  • markdown (probably breaks with the current message content, especially containing patterns)
  • icon URL (could be useful for someone)
  • delay
  • call (requires a authentication to test)

https://docs.ntfy.sh/publish/#publish-as-json

There are also cache and firebase supported using the http request instead of JSON. Not sure if they are also supported via JSON. Those are currently only supported via http request headers (see binwiederhier/ntfy#1119 & binwiederhier/ntfy#1123).

That and other not implemented properties could be worked around via the custom provider for now.

Copy link
Owner

@TwiN TwiN left a comment

Choose a reason for hiding this comment

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

Looks good, but could you create a few tests?

@TwiN
Copy link
Owner

TwiN commented Jun 13, 2024

There are also other properties supported by ntfy, which are not implemented yet:

* [`actions`](https://docs.ntfy.sh/publish/#action-buttons) to define custom action buttons for the notificaton

* `attach` - URL to an attachment (sounds less useful for gatus)

* `markdown` (probably breaks with the current message content, especially containing patterns)

* [`icon`](https://docs.ntfy.sh/publish/#icons) URL (could be useful for someone)

* `delay`

* [`call`](https://docs.ntfy.sh/publish/#phone-calls) (requires a authentication to test)

https://docs.ntfy.sh/publish/#publish-as-json

There are also cache and firebase supported using the http request instead of JSON. Not sure if they are also supported via JSON.

That and not implemented properties could be worked around via the custom provider for now.

@stendler WRT Not sure if they are also supported via JSON - if you're unsure, would it be possible to omit those features from this PR? I'd rather just include the features we are 100% sure works.

@TwiN TwiN added feature New feature or request area/alerting Related to alerting labels Jun 13, 2024
@stendler
Copy link
Author

WRT Not sure if they are also supported via JSON - if you're unsure, would it be possible to omit those features from this PR? I'd rather just include the features we are 100% sure works.

Sorry, I have not updated that comment but only the PR summary: cache and firebase are not supported in the JSON payload, BUT as http request headers. That's what I've done in the implementation.

Looks good, but could you create a few tests?

What kind of tests do you want to see here? For TestAlertProvider_buildRequestBody a new scenario for the new JSON attributes, or add them to an existing one?

There does not seem to be a test for the request headers there yet. So either a test with a server receiving the request is needed or a refactor of the Send method into a buildRequest to be able to test for the headers.

Which of these options do you think are best?

@TwiN
Copy link
Owner

TwiN commented Jul 1, 2024

@stendler A new case for TestAlertProvider_buildRequestBody should suffice!

@stendler
Copy link
Author

stendler commented Jul 3, 2024

@TwiN I've now added tests for the json properties and also tests for verifying the request headers from the Send function using httptest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Related to alerting feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants