Support for Apprise #759

Merged
witten merged 10 commits from pizzapim/borgmatic:main into main 2023-10-04 21:58:21 +00:00
Contributor

This merge request is to implement Apprise for notifications as per my issue #715

I have two questions/comments:

  • As per my comment, not sure how to improve logging of the Apprise ping as we only have the service URL which can contain sensitive data.
  • Apprise has a feature called "tags" where you can filter which URLs are pinged. I am not sure whether it makes sense to implement this here as well.

Let me know what you think!

This merge request is to implement Apprise for notifications as per my issue #715 I have two questions/comments: - As per my comment, not sure how to improve logging of the Apprise ping as we only have the service URL which can contain sensitive data. - Apprise has a feature called "tags" where you can filter which URLs are pinged. I am not sure whether it makes sense to implement this here as well. Let me know what you think!
pizzapim added 3 commits 2023-09-21 20:26:44 +00:00
witten reviewed 2023-09-22 21:15:43 +00:00
witten left a comment
Owner

Thanks so much for diving in and implementing this! The general approach looks good to me.. It's nice to see that the apprise library makes this relatively easy. I left one idea in the review comments about how to potentially deal with the sensitive data issue in logging. As for the tags feature, I'd say whether you implement it as part of this PR depends on: 1. Whether it's a feature you need/want to use now, and 2. How much complexity it takes to add. You can always do a basic initial pass without it and then iterate with subsequent PRs later on. In fact it may be easier to review and release that way.

Anyway, please let me know if you have any thoughts or questions about any of my PR comments.

Thanks so much for diving in and implementing this! The general approach looks good to me.. It's nice to see that the `apprise` library makes this relatively easy. I left one idea in the review comments about how to potentially deal with the sensitive data issue in logging. As for the tags feature, I'd say whether you implement it as part of this PR depends on: 1. Whether it's a feature you need/want to use now, and 2. How much complexity it takes to add. You can always do a basic initial pass without it and then iterate with subsequent PRs later on. In fact it may be easier to review and release that way. Anyway, please let me know if you have any thoughts or questions about any of my PR comments.
@ -1309,0 +1319,4 @@
List of Apprise service URLs to publish to.
example:
- "mastodon://accesskey/host/?visibility=direct"
- "pagerduty://A1BRTD4JD@TIiajkdnlazkcOXrIdevi7F/node01.local/drive_sda/"
Owner

Note that borgmatic already has a "native" PagerDuty integration, so a different example may make more sense here.

Note that borgmatic already has a "native" PagerDuty integration, so a different example may make more sense here.
pizzapim marked this conversation as resolved
@ -1309,0 +1332,4 @@
type: string
description: |
Specify the message body.
exampe: Your backups have failed.
Owner

Typo.

Also, maybe this example body should be start-specific?

Typo. Also, maybe this example body should be `start`-specific?
pizzapim marked this conversation as resolved
@ -1309,0 +1336,4 @@
notification_type:
type: string
description: |
The Apprise message type.
Owner

Does this even need to be configurable at the borgmatic config level? For instance, I'd imagine that start would always be of notification type info, finish would always be success, and fail would always be failure. Or am I just missing other use cases that would deviate from those?

Does this even need to be configurable at the borgmatic config level? For instance, I'd imagine that `start` would always be of notification type `info`, `finish` would always be `success`, and `fail` would always be `failure`. Or am I just missing other use cases that would deviate from those?
pizzapim marked this conversation as resolved
@ -1309,0 +1356,4 @@
type: string
description: |
Specify the message body.
exampe: Your backups have failed.
Owner

Typo.

Also, maybe this example body should be finish-specific?

Typo. Also, maybe this example body should be `finish`-specific?
pizzapim marked this conversation as resolved
@ -1309,0 +1380,4 @@
type: string
description: |
Specify the message body.
exampe: Your backups have failed.
Owner

Typo.

Typo.
pizzapim marked this conversation as resolved
@ -0,0 +19,4 @@
'''
Ping the configured Apprise service URLs.
Use the given configuration filename in any log entries.
If this is a dry run, then don't actually ping anything.
Owner

Minor nit: borgmatic comments wrap text by character width (100) instead of by sentence boundary.

Minor nit: borgmatic comments wrap text by character width (100) instead of by sentence boundary.
pizzapim marked this conversation as resolved
@ -0,0 +36,4 @@
)
# TODO: Currently not very meaningful message.
# However, the Apprise service URLs can contain sensitive info.
Owner

What do you think of making service_urls a list of dicts/objects instead of a list of strings? Then each entry could have both a URL and an optional or required label.. And you could log the label without fear of leaking sensitive info. See repositories for an example of such an approach.

What do you think of making `service_urls` a list of dicts/objects instead of a list of strings? Then each entry could have both a URL and an optional or required label.. And you could log the label without fear of leaking sensitive info. See `repositories` for an example of such an approach.
pizzapim marked this conversation as resolved
@ -0,0 +39,4 @@
# However, the Apprise service URLs can contain sensitive info.
dry_run_label = ' (dry run; not actually pinging)' if dry_run else ''
logger.info(f'{config_filename}: Pinging Apprise {dry_run_label}')
logger.debug(f'{config_filename}: Using Apprise ping')
Owner

Why is this debug log needed? The previous info log seems like it covers things..

Why is this `debug` log needed? The previous `info` log seems like it covers things..
pizzapim marked this conversation as resolved
@ -0,0 +45,4 @@
body = state_config.get('body')
notify_type = state_config.get('notification_type', 'success')
apobj = apprise.Apprise()
Owner

Please spell this (apobj) out. Thank you!

Please spell this (`apobj`) out. Thank you!
pizzapim marked this conversation as resolved
@ -0,0 +55,4 @@
title=title,
body=body,
body_format=NotifyFormat.TEXT,
notify_type=get_notify_type(notify_type)
Owner

My proposal is instead of allowing the user to specify the notification type, just use your default_notify_type() mapping all the time.

My proposal is instead of allowing the user to specify the notification type, just use your `default_notify_type()` mapping all the time.
pizzapim marked this conversation as resolved
@ -0,0 +62,4 @@
logger.warning(f'{config_filename}: error sending some apprise notifications')
def get_notify_type(s):
Owner

Please spell out variable names. Yes, I know it's obnoxious. 😄

Please spell out variable names. Yes, I know it's obnoxious. 😄
pizzapim marked this conversation as resolved
@ -0,0 +70,4 @@
if s == 'warning':
return NotifyType.WARNING
if s == 'failure':
return NotifyType.FAILURE
Owner

It would be a little more idiomatic Python IMO to use a dict to perform this mapping.

It would be a little more idiomatic Python IMO to use a dict to perform this mapping.
pizzapim marked this conversation as resolved
@ -0,0 +81,4 @@
if state == 'fail':
return NotifyType.FAILURE
if state == 'log':
return NotifyType.INFO
Owner

You could also perform this mapping with a dict!

You could also perform this mapping with a dict!
pizzapim marked this conversation as resolved
setup.py Outdated
@ -35,6 +35,7 @@ setup(
'requests',
'ruamel.yaml>0.15.0,<0.18.0',
'setuptools',
'apprise'
Owner

One thing I'm not wild about with the apprise library is its dependency on PyYAML, given that: 1. PyYAML is often hard to build, and 2. borgmatic already has a dependency on a different (actively maintained, more modern) YAML library: ruamel.yaml.

So what do you think of making the apprise dependency an optional requirement, so only those users that want to use the Apprise hook need to install it?

Also, could you add apprise and any of its dependencies that aren't already there to test_requirements.txt with appropriate pinned versions? I'm fine with extra test dependencies.. I'm just trying to cut down on general install dependencies for most users.

One thing I'm not wild about with the apprise library is its dependency on PyYAML, given that: 1. PyYAML is often hard to build, and 2. borgmatic already has a dependency on a different (actively maintained, more modern) YAML library: ruamel.yaml. So what do you think of making the apprise dependency an [optional requirement](https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#optional-dependencies), so only those users that want to use the Apprise hook need to install it? Also, could you add apprise and any of [its dependencies](https://github.com/caronc/apprise/blob/master/requirements.txt) that aren't already there to `test_requirements.txt` with appropriate pinned versions? I'm fine with extra test dependencies.. I'm just trying to cut down on general install dependencies for most users.
pizzapim marked this conversation as resolved
pizzapim added 1 commit 2023-09-23 19:11:41 +00:00
pizzapim added 1 commit 2023-09-23 19:34:49 +00:00
eba4e57a29 convert map to list for apprise function call
fix apprise config schema
remove apprise from required dependencies
Author
Contributor

Thanks for your invaluable comments :) I have amended my code.
I agree, let's start with having only default notification types. If necessary, it can be easily added back in.
Regarding the possibility of adding 'tags', I have no immediate use of it so let's add it if the need arises. I think that shouldn't be very difficult.

I agree with your reasoning to make apprise an optional dependency. However, I'm not sure how to use pipx to install this variant and test this setup. I tried: pipx install --editable --force .[Apprise], but this does not install the apprise package. I'm also not sure how to deal with an optional dependency in my code, as the entire application does not run if I use just import apprise now. Should I wrap it in a try-catch?

Thanks for your invaluable comments :) I have amended my code. I agree, let's start with having only default notification types. If necessary, it can be easily added back in. Regarding the possibility of adding 'tags', I have no immediate use of it so let's add it if the need arises. I think that shouldn't be very difficult. I agree with your reasoning to make apprise an optional dependency. However, I'm not sure how to use pipx to install this variant and test this setup. I tried: `pipx install --editable --force .[Apprise]`, but this does not install the apprise package. I'm also not sure how to deal with an optional dependency in my code, as the entire application does not run if I use just `import apprise` now. Should I wrap it in a try-catch?
Author
Contributor

@witten One more question, how should I pin the versions of Apprise's dependencies? Sorry for the newb questions, never worked at a Python project outside of university :)

@witten One more question, how should I pin the versions of Apprise's dependencies? Sorry for the newb questions, never worked at a Python project outside of university :)
Owner

I agree with your reasoning to make apprise an optional dependency. However, I'm not sure how to use pipx to install this variant and test this setup. I tried: pipx install --editable --force .[Apprise], but this does not install the apprise package.

Looks like you have a typo in setup.py: extra_require should be extras_require. Once I make that change:

$ pipx install --editable --force .[Apprise]
Installing to existing venv 'borgmatic'
  installed package borgmatic 1.8.3.dev0, installed using Python 3.10.10
  These apps are now globally available
    - borgmatic
    - generate-borgmatic-config
    - validate-borgmatic-config
done! ✨ 🌟 ✨
$ find ~/.local/pipx/venvs/ -name apprise
/home/witten/.local/pipx/venvs/borgmatic/bin/apprise
/home/witten/.local/pipx/venvs/borgmatic/lib/python3.10/site-packages/apprise

I'm also not sure how to deal with an optional dependency in my code, as the entire application does not run if I use just import apprise now. Should I wrap it in a try-catch?

Yeah, my recommendation would be import apprise in functions where it's used (instead of globally), wrap with a try/catch, and then if an ImportError is caught, maybe log a warning and bail. In general, borgmatic takes the approach that if a monitoring hook fails, it shouldn't cause the whole backup to fail. See the other monitoring hooks for examples.

@witten One more question, how should I pin the versions of Apprise's dependencies? Sorry for the newb questions, never worked at a Python project outside of university :)

One way would be something like this:

. ~/.local/pipx/venvs/borgmatic/bin/activate
pip freeze

That should show you all borgmatic + Apprise dependencies and their versions that got installed by pipx.

> I agree with your reasoning to make apprise an optional dependency. However, I'm not sure how to use pipx to install this variant and test this setup. I tried: `pipx install --editable --force .[Apprise]`, but this does not install the apprise package. Looks like you have a typo in `setup.py`: `extra_require` should be `extras_require`. Once I make that change: ``` $ pipx install --editable --force .[Apprise] Installing to existing venv 'borgmatic' installed package borgmatic 1.8.3.dev0, installed using Python 3.10.10 These apps are now globally available - borgmatic - generate-borgmatic-config - validate-borgmatic-config done! ✨ 🌟 ✨ $ find ~/.local/pipx/venvs/ -name apprise /home/witten/.local/pipx/venvs/borgmatic/bin/apprise /home/witten/.local/pipx/venvs/borgmatic/lib/python3.10/site-packages/apprise ``` > I'm also not sure how to deal with an optional dependency in my code, as the entire application does not run if I use just import apprise now. Should I wrap it in a try-catch? Yeah, my recommendation would be `import apprise` in functions where it's used (instead of globally), wrap with a `try`/`catch`, and then if an `ImportError` is caught, maybe log a warning and bail. In general, borgmatic takes the approach that if a monitoring hook fails, it shouldn't cause the whole backup to fail. See the other monitoring hooks for examples. > @witten One more question, how should I pin the versions of Apprise's dependencies? Sorry for the newb questions, never worked at a Python project outside of university :) One way would be something like this: ```bash . ~/.local/pipx/venvs/borgmatic/bin/activate pip freeze ``` That should show you _all_ borgmatic + Apprise dependencies and their versions that got installed by pipx.
pizzapim added 1 commit 2023-09-25 08:19:14 +00:00
1795a78c1c fix typo in setup.py
handle if apprise cannot be imported
pizzapim added 1 commit 2023-09-25 08:23:47 +00:00
Author
Contributor

Thanks for the comments, I think the current PR is ready to be merged then or do you have additional comments?

Thanks for the comments, I think the current PR is ready to be merged then or do you have additional comments?
pizzapim changed title from WIP: Support for Apprise to Support for Apprise 2023-09-25 08:47:53 +00:00
witten requested changes 2023-09-25 18:42:54 +00:00
witten left a comment
Owner

This hook is looking really good! I left some minor comments, but as far as I'm concerned, the hook itself is just about ready to merge. The main things missing to my eye are:

  • automated tests: IMO these need to be implemented ahead of merging the PR. The good news is I think they should be pretty easy. I'd recommend doing unit tests rather than integration or end-to-end tests for this particular feature. You can take a look at some of the existing tests for other monitoring hooks for inspiration. And to set up your test environment, check out these developer docs. If you get stuck anywhere, I'd be happy to help.
  • documentation: I'm totally fine doing docs for you so you can focus on the code, but I wanted to give you the opportunity in case you're at all interested. (I usually do docs for most features.)
This hook is looking really good! I left some minor comments, but as far as I'm concerned, the hook itself is just about ready to merge. The main things missing to my eye are: - automated tests: IMO these need to be implemented ahead of merging the PR. The good news is I think they should be pretty easy. I'd recommend doing unit tests rather than integration or end-to-end tests for this particular feature. You can take a look at some of the existing tests for other monitoring hooks for inspiration. And to set up your test environment, check out [these developer docs](https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/#automated-tests). If you get stuck anywhere, I'd be happy to help. - documentation: I'm totally fine doing docs for you so you can focus on the code, but I wanted to give you the opportunity in case you're at all interested. (I usually do docs for most features.)
@ -1309,0 +1341,4 @@
title:
type: string
description: |
Specify the message title.
Owner

Is it worth saying what the defaults are for title or body if not specified?

Is it worth saying what the defaults are for `title` or `body` if not specified?
pizzapim marked this conversation as resolved
@ -0,0 +49,4 @@
return
dry_run_string = ' (dry run; not actually pinging)' if dry_run else ''
labels_string = ', '.join(map(lambda service: service['label'], hook_config.get('services')))
Owner

An alternative:

    labels_string = ', '.join(map(operator.itemgetter('label'), hook_config.get('services'))) 

Or even just:

    labels_string = ', '.join(service['label'] for service in hook_config.get('services'))

Do not feel strongly.

An alternative: ```python labels_string = ', '.join(map(operator.itemgetter('label'), hook_config.get('services'))) ``` Or even just: ```python labels_string = ', '.join(service['label'] for service in hook_config.get('services')) ``` Do not feel strongly.
pizzapim marked this conversation as resolved
@ -0,0 +54,4 @@
title = state_config.get('title', '')
body = state_config.get('body')
notify_type = state_to_notify_type[state.name.lower()]
Owner

These could be moved inline to the notify() call below, as they're not used elsewhere.

Do not feel strongly.

These could be moved inline to the `notify()` call below, as they're not used elsewhere. Do not feel strongly.
pizzapim marked this conversation as resolved
@ -0,0 +57,4 @@
notify_type = state_to_notify_type[state.name.lower()]
apprise_object = apprise.Apprise()
apprise_object.add(list(map(lambda service: service['url'], hook_config.get('services'))))
Owner

Similar here to the comment above.

Similar here to the comment above.
pizzapim marked this conversation as resolved
@ -0,0 +66,4 @@
title=title,
body=body,
body_format=NotifyFormat.TEXT,
notify_type=notify_type)
Owner

Code style nit: Closing paren on new line, lined up with start of result. Also a trailing comma here would be good.

You may be able to get this reformatted for you automatically by running tox -e black.

Code style nit: Closing paren on new line, lined up with start of `result`. Also a trailing comma here would be good. You may be able to get this reformatted for you automatically by running [`tox -e black`](https://torsion.org/borgmatic/docs/how-to/develop-on-borgmatic/#code-formatting).
pizzapim marked this conversation as resolved
@ -0,0 +68,4 @@
body_format=NotifyFormat.TEXT,
notify_type=notify_type)
if result is False:
Owner

if not result: would be more idiomatic Python IMO. Unless you're trying to distinguish from result being None that is...

`if not result:` would be more idiomatic Python IMO. Unless you're trying to distinguish from `result` being `None` that is...
Author
Contributor

Exactly like you said, the function returns either a Boolean or None

Exactly like you said, the function returns either a Boolean or None
witten marked this conversation as resolved
@ -0,0 +69,4 @@
notify_type=notify_type)
if result is False:
logger.warning(f'{config_filename}: error sending some apprise notifications')
Owner

Convention nit: Capital E in "error". And maybe "A" in "apprise" for consistency with other logs?

Convention nit: Capital E in "error". And maybe "A" in "apprise" for consistency with other logs?
pizzapim marked this conversation as resolved
@ -33,1 +33,4 @@
zipp==3.15.0; python_version < '3.8'
certifi==2022.9.24
PyYAML==6.0
Markdown==3.4.1
Owner

I'm not seeing apprise itself in the test dependencies...? I think it needs to be there for it to be available in tests. I'm totally fine if it's not an optional dependency for tests themselves.

Also would you mind alpha ordering these new dependencies into the rest?

I'm not seeing `apprise` itself in the test dependencies...? I think it needs to be there for it to be available in tests. I'm totally fine if it's not an optional dependency for tests themselves. Also would you mind alpha ordering these new dependencies into the rest?
pizzapim marked this conversation as resolved
pizzapim added 1 commit 2023-10-01 15:00:48 +00:00
pizzapim added 1 commit 2023-10-01 16:52:46 +00:00
pizzapim force-pushed main from c3678f10a6 to 4763c323d0 2023-10-01 17:00:01 +00:00 Compare
Author
Contributor

@witten I added some unit tests, I hope they are sufficient! Tox reports no failure (except a spelling error somewhere else). I also rebased my branch to master. I'm fine with you doing the documentation part.

@witten I added some unit tests, I hope they are sufficient! Tox reports no failure (except a spelling error somewhere else). I also rebased my branch to master. I'm fine with you doing the documentation part.
witten requested changes 2023-10-01 23:20:07 +00:00
witten left a comment
Owner

Really nice tests! I left some minor notes, but I think this is almost ready to merge.

One test case I didn't see though is for the ImportError case. I'm not exactly sure how to test that given that there's not a function you can mock out to trigger a fake ImportError, so maybe just flag that exception handler with # pragma: no cover..?

Additionally, is there a test for the if state.name.lower() not in run_states: case?

Really nice tests! I left some minor notes, but I think this is almost ready to merge. One test case I didn't see though is for the `ImportError` case. I'm not exactly sure how to test that given that there's not a function you can mock out to trigger a fake `ImportError`, so maybe just flag that exception handler with `# pragma: no cover`..? Additionally, is there a test for the `if state.name.lower() not in run_states:` case?
@ -0,0 +3,4 @@
from flexmock import flexmock
import borgmatic.hooks.monitor
from borgmatic.hooks import apprise as module
Owner

Thanks so much for following testing convention in this code base!

Thanks so much for following testing convention in this code base!
pizzapim marked this conversation as resolved
@ -0,0 +5,4 @@
import borgmatic.hooks.monitor
from borgmatic.hooks import apprise as module
topic = 'borgmatic-unit-testing'
Owner

Super minor, but constant names at the global scope are usually all caps (logger being a prominent exception to that).

Super minor, but constant names at the global scope are usually all caps (`logger` being a prominent exception to that).
pizzapim marked this conversation as resolved
@ -0,0 +9,4 @@
def test_ping_monitor_adheres_dry_run():
flexmock(apprise.Apprise).should_receive('notify').never()
Owner

Nice mocking, but IMO apprise.Apprise itself should also be mocked out too. My rationale is that without that additional mock, your unit under test is integrating with a third-party library (apprise) and thus your test becomes an integration test.

Mocking it might be as easy as something like:

flexmock(apprise).should_receive('Apprise').and_return(flexmock(add=lambda: None))

This probably also goes for all the other tests of this unit (ping_monitor()) that get far enough to instantiate Apprise.

Nice mocking, but IMO `apprise.Apprise` itself should also be mocked out too. My rationale is that without that additional mock, your unit under test is integrating with a third-party library (`apprise`) and thus your test becomes an integration test. Mocking it might be as easy as something like: ```python flexmock(apprise).should_receive('Apprise').and_return(flexmock(add=lambda: None)) ``` This probably also goes for all the other tests of this unit (`ping_monitor()`) that get far enough to instantiate `Apprise`.
pizzapim marked this conversation as resolved
@ -0,0 +34,4 @@
)
def test_ping_monitor_hits_fail_by_default():
Owner

This description suggests it's testing that the fail state is notified, but it doesn't look like that's actually tested anywhere.. So maybe either rename the test to reflect reality or change the asserts/expectations to reflect the test name?

This description suggests it's testing that the `fail` state is notified, but it doesn't look like that's actually tested anywhere.. So maybe either rename the test to reflect reality or change the asserts/expectations to reflect the test name?
pizzapim marked this conversation as resolved
@ -0,0 +120,4 @@
)
def test_ping_monitor_with_custom_message_title():
Owner

Minor nit: You could encode the general expectation of the test in this test name (and some of the following tests too). In other words, include a verb about what the unit under test is expected to do, not just a noun about what's different with the input given in the current test. Example: test_ping_monitor_passes_through_custom_message_title()

Do not feel strongly.

Minor nit: You could encode the general expectation of the test in this test name (and some of the following tests too). In other words, include a verb about what the unit under test is expected to _do_, not just a noun about what's different with the input given in the current test. Example: `test_ping_monitor_passes_through_custom_message_title()` Do not feel strongly.
pizzapim marked this conversation as resolved
pizzapim added 1 commit 2023-10-04 11:18:29 +00:00
pizzapim force-pushed main from 6856c3e465 to 7a9625cd44 2023-10-04 11:19:50 +00:00 Compare
Author
Contributor

I think I fixed the tests with your comments.

I played around with this StackOverflow answer but ultimately couldn't get the import mock to work: https://stackoverflow.com/questions/2481511/mocking-importerror-in-python
The test test_ping_monitor_does_not_hit_with_no_states does check that no service is called when states is an empty list.

I think I fixed the tests with your comments. I played around with this StackOverflow answer but ultimately couldn't get the import mock to work: https://stackoverflow.com/questions/2481511/mocking-importerror-in-python The test `test_ping_monitor_does_not_hit_with_no_states` does check that no service is called when `states` is an empty list.
Owner

Thanks so much for sticking with this! This looks great to me.

Thanks so much for sticking with this! This looks great to me.
witten merged commit 9d34d2eec5 into main 2023-10-04 21:58:21 +00:00
witten referenced this issue from a commit 2023-10-04 21:58:21 +00:00
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: borgmatic-collective/borgmatic#759
No description provided.