Support for Apprise #759
No reviewers
Labels
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
No Milestone
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: borgmatic-collective/borgmatic#759
Loading…
Reference in New Issue
No description provided.
Delete Branch "pizzapim/borgmatic:main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This merge request is to implement Apprise for notifications as per my issue #715
I have two questions/comments:
Let me know what you think!
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/"
Note that borgmatic already has a "native" PagerDuty integration, so a different example may make more sense here.
@ -1309,0 +1332,4 @@
type: string
description: |
Specify the message body.
exampe: Your backups have failed.
Typo.
Also, maybe this example body should be
start
-specific?@ -1309,0 +1336,4 @@
notification_type:
type: string
description: |
The Apprise message type.
Does this even need to be configurable at the borgmatic config level? For instance, I'd imagine that
start
would always be of notification typeinfo
,finish
would always besuccess
, andfail
would always befailure
. Or am I just missing other use cases that would deviate from those?@ -1309,0 +1356,4 @@
type: string
description: |
Specify the message body.
exampe: Your backups have failed.
Typo.
Also, maybe this example body should be
finish
-specific?@ -1309,0 +1380,4 @@
type: string
description: |
Specify the message body.
exampe: Your backups have failed.
Typo.
@ -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.
Minor nit: borgmatic comments wrap text by character width (100) instead of by sentence boundary.
@ -0,0 +36,4 @@
)
# TODO: Currently not very meaningful message.
# However, the Apprise service URLs can contain sensitive info.
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. Seerepositories
for an example of such an approach.@ -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')
Why is this
debug
log needed? The previousinfo
log seems like it covers things..@ -0,0 +45,4 @@
body = state_config.get('body')
notify_type = state_config.get('notification_type', 'success')
apobj = apprise.Apprise()
Please spell this (
apobj
) out. Thank you!@ -0,0 +55,4 @@
title=title,
body=body,
body_format=NotifyFormat.TEXT,
notify_type=get_notify_type(notify_type)
My proposal is instead of allowing the user to specify the notification type, just use your
default_notify_type()
mapping all the time.@ -0,0 +62,4 @@
logger.warning(f'{config_filename}: error sending some apprise notifications')
def get_notify_type(s):
Please spell out variable names. Yes, I know it's obnoxious. 😄
@ -0,0 +70,4 @@
if s == 'warning':
return NotifyType.WARNING
if s == 'failure':
return NotifyType.FAILURE
It would be a little more idiomatic Python IMO to use a dict to perform this mapping.
@ -0,0 +81,4 @@
if state == 'fail':
return NotifyType.FAILURE
if state == 'log':
return NotifyType.INFO
You could also perform this mapping with a dict!
@ -35,6 +35,7 @@ setup(
'requests',
'ruamel.yaml>0.15.0,<0.18.0',
'setuptools',
'apprise'
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.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 justimport apprise
now. Should I wrap it in a try-catch?@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 :)
Looks like you have a typo in
setup.py
:extra_require
should beextras_require
. Once I make that change:Yeah, my recommendation would be
import apprise
in functions where it's used (instead of globally), wrap with atry
/catch
, and then if anImportError
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.One way would be something like this:
That should show you all borgmatic + Apprise dependencies and their versions that got installed by pipx.
Thanks for the comments, I think the current PR is ready to be merged then or do you have additional comments?
WIP: Support for Appriseto Support for AppriseThis 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:
@ -1309,0 +1341,4 @@
title:
type: string
description: |
Specify the message title.
Is it worth saying what the defaults are for
title
orbody
if not specified?@ -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')))
An alternative:
Or even just:
Do not feel strongly.
@ -0,0 +54,4 @@
title = state_config.get('title', '')
body = state_config.get('body')
notify_type = state_to_notify_type[state.name.lower()]
These could be moved inline to the
notify()
call below, as they're not used elsewhere.Do not feel strongly.
@ -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'))))
Similar here to the comment above.
@ -0,0 +66,4 @@
title=title,
body=body,
body_format=NotifyFormat.TEXT,
notify_type=notify_type)
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
.@ -0,0 +68,4 @@
body_format=NotifyFormat.TEXT,
notify_type=notify_type)
if result is False:
if not result:
would be more idiomatic Python IMO. Unless you're trying to distinguish fromresult
beingNone
that is...Exactly like you said, the function returns either a Boolean or None
@ -0,0 +69,4 @@
notify_type=notify_type)
if result is False:
logger.warning(f'{config_filename}: error sending some apprise notifications')
Convention nit: Capital E in "error". And maybe "A" in "apprise" for consistency with other logs?
@ -33,1 +33,4 @@
zipp==3.15.0; python_version < '3.8'
certifi==2022.9.24
PyYAML==6.0
Markdown==3.4.1
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?
c3678f10a6
to4763c323d0
@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.
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 fakeImportError
, 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
Thanks so much for following testing convention in this code base!
@ -0,0 +5,4 @@
import borgmatic.hooks.monitor
from borgmatic.hooks import apprise as module
topic = 'borgmatic-unit-testing'
Super minor, but constant names at the global scope are usually all caps (
logger
being a prominent exception to that).@ -0,0 +9,4 @@
def test_ping_monitor_adheres_dry_run():
flexmock(apprise.Apprise).should_receive('notify').never()
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:
This probably also goes for all the other tests of this unit (
ping_monitor()
) that get far enough to instantiateApprise
.@ -0,0 +34,4 @@
)
def test_ping_monitor_hits_fail_by_default():
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?@ -0,0 +120,4 @@
)
def test_ping_monitor_with_custom_message_title():
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.
6856c3e465
to7a9625cd44
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 whenstates
is an empty list.Thanks so much for sticking with this! This looks great to me.