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

Added support for Pushover Hook for notifications #86

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

tony1661
Copy link
Contributor

Hi,

This PR aims to add support for Pushover notifications.

I've updated the documentation, list of supported integrations on borgmatic website homepage and have included various tests.

Pushover uses two keys. A user key and an application key. User keys are how you are authenticated to your Pushover account. Application keys are how Pushover knows which application to post the message to. Applications can be though of as threads, or topics.

I've included support for all of the API options that Pushover has.

Please let me know if there are any changes I should make.

@witten
Copy link
Collaborator

witten commented Nov 1, 2024

Thanks for taking the time to implement yet another monitoring hook! I'll have a look at this after the 1.9.0 release—which I'm currently trying to avoid scope creep on so that it's released sometime this century. 😄

Copy link
Collaborator

@witten witten left a comment

Choose a reason for hiding this comment

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

Initial review (not including tests). This looks like a great start! Most of my comments are on smaller stuff, so hopefully there shouldn't be too many changes. Thanks!

borgmatic/hooks/pushover.py Outdated Show resolved Hide resolved
borgmatic/config/schema.yaml Outdated Show resolved Hide resolved
borgmatic/config/schema.yaml Outdated Show resolved Hide resolved
borgmatic/config/schema.yaml Outdated Show resolved Hide resolved
borgmatic/config/schema.yaml Outdated Show resolved Hide resolved
docs/how-to/monitor-your-backups.md Outdated Show resolved Hide resolved
docs/how-to/monitor-your-backups.md Outdated Show resolved Hide resolved
docs/how-to/monitor-your-backups.md Outdated Show resolved Hide resolved
docs/how-to/monitor-your-backups.md Outdated Show resolved Hide resolved
docs/how-to/monitor-your-backups.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@witten witten left a comment

Choose a reason for hiding this comment

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

Tests look great! Thank you so much for doing them (and the docs too!). I bet the tests were easier to write this time around.

docs/how-to/monitor-your-backups.md Outdated Show resolved Hide resolved
tests/unit/hooks/test_pushover.py Show resolved Hide resolved
tests/unit/hooks/test_pushover.py Show resolved Hide resolved
tests/unit/hooks/test_pushover.py Outdated Show resolved Hide resolved
tests/unit/hooks/test_pushover.py Show resolved Hide resolved
tests/unit/hooks/test_pushover.py Outdated Show resolved Hide resolved
tests/unit/hooks/test_pushover.py Outdated Show resolved Hide resolved
)


def test_ping_monitor_start_state_backup_default_message_with_priority_high_declared_expire_and_retry_delared_success():
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one too: IMO it's not just the success you care about but the fact that expire and retry are ignored.

borgmatic/hooks/pushover.py Show resolved Hide resolved
borgmatic/hooks/pushover.py Show resolved Hide resolved
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.

2 participants