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 Pushover provider #405

Merged
merged 5 commits into from
Jan 29, 2023
Merged

feat(alerting): Add Pushover provider #405

merged 5 commits into from
Jan 29, 2023

Conversation

Kovah
Copy link
Contributor

@Kovah Kovah commented Jan 19, 2023

Summary

I am not sure if I missed something. There are a lot of files where providers are referenced, but I think I got all of them.

  • Adds new provider named Pushover with corresponding tests
  • Adds Pushover as a provider to the configuration and adjusts test accordingly
  • Adds Pushover to alerting_test.go, provider.go and type.go
  • Updates the readme with configuration details

Before merging: what would be the best way to properly test this with real API calls? Is there any process defined or should I just try to build it and check if a config works?

Additional thoughts/ideas that could be implemented later:

  • Add support for other configuration options, like device support, the Gatus URL in it or setting a sound
  • Allow overriding of title, priority,... per group like the PagerDuty provider does.
  • Allow usage of placeholders in title and description like the custom provider does.

Related: ##129

Checklist

  • Tested and/or added tests to validate that the changes work as intended, if applicable.
  • Added the documentation in README.md, if applicable.

- Adds new provider named Pushover with corresponding tests
- Adds Pushover as a provider to the configuration and adjusts test accordingly
- Adds Pushover to alerting_test.go, provider.go and type.go
- Updates the readme with configuration details
README.md Outdated Show resolved Hide resolved
alerting/config.go Outdated Show resolved Hide resolved
alerting/provider/pushover/pushover.go Outdated Show resolved Hide resolved
alerting/provider/pushover/pushover.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
alerting/config.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 25, 2023

Codecov Report

Base: 82.00% // Head: 81.98% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (b99d14d) compared to base (90bb8f7).
Patch coverage: 86.66% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #405      +/-   ##
==========================================
- Coverage   82.00%   81.98%   -0.02%     
==========================================
  Files          55       56       +1     
  Lines        4178     4225      +47     
==========================================
+ Hits         3426     3464      +38     
- Misses        586      592       +6     
- Partials      166      169       +3     
Impacted Files Coverage Δ
alerting/provider/provider.go 100.00% <ø> (ø)
alerting/provider/pushover/pushover.go 86.36% <86.36%> (ø)
config/config.go 76.06% <100.00%> (+0.10%) ⬆️
config/maintenance/maintenance.go 96.20% <0.00%> (-3.80%) ⬇️
controller/handler/handler.go 79.41% <0.00%> (+1.28%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Kovah
Copy link
Contributor Author

Kovah commented Jan 25, 2023

I have built the Docker container with the latest changes and tested it with this config:

---
debug: true

alerting:
  pushover:
    application-token: ar5k...wkg4
    user-key: 2ZHm...cWed
    title: "Pushover Gatus Test"
    default-alert:
      enabled: true
      description: "Healthcheck failed for [ENDPOINT_URL]"
      failure-threshold: 1
      success-threshold: 1
      send-on-resolved: true

endpoints:
  - name: dashboard
    url: https://my-private-domain.com
    interval: 30s
    alerts:
      - type: pushover
    conditions:
      - "[STATUS] == 200"

Working as expected.

What I would like to add before merging this is to add support for placeholders in title and description like the custom provider does. I think this should be handled by a helper function so that there's no code duplication, because currently it's just hardcoded into the custom provider. Any thoughts an that?

@TwiN
Copy link
Owner

TwiN commented Jan 29, 2023

@Kovah Support for placeholders will be handled separately.

See #287

@TwiN TwiN changed the title Add a new Pushover provider feat(alerting): Add Pushover provider Jan 29, 2023
@TwiN
Copy link
Owner

TwiN commented Jan 29, 2023

Before merging: what would be the best way to properly test this with real API calls? Is there any process defined or should I just try to build it and check if a config works?

Using the MockRoundTripper just like in PagerDuty was the right approach, and you've already done it.

There's a couple of integration tests that send real requests, but I try to avoid them as much as possible, and when it's unavoidable, I either use a website that I own or a stable public service (e.g. example.com, example.org, a public DNS, etc.), because they have predictable outputs or I can control the outputs myself.

As such, I wouldn't feel really comfortable having an integration test for something like pushover unless they explicitly provide permission.

@TwiN TwiN merged commit 21f62f3 into TwiN:master Jan 29, 2023
@TwiN
Copy link
Owner

TwiN commented Jan 29, 2023

@Kovah Excellent work!
Thank you for the contribution!

@Kovah
Copy link
Contributor Author

Kovah commented Jan 29, 2023

Great, thanks for merging!

@TwiN TwiN added feature New feature or request area/alerting Related to alerting labels Feb 5, 2023
@TwiN TwiN mentioned this pull request Feb 5, 2023
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.

3 participants