Support for Apprise #759

Merged
witten merged 10 commits from pizzapim/borgmatic:main into main 2023-10-04 21:58:21 +00:00
7 changed files with 390 additions and 3 deletions

View File

@ -1306,6 +1306,99 @@ properties:
example:
- start
- finish
apprise:
type: object
required: ['services']
additionalProperties: false
properties:
services:
type: array
items:
type: object
required:
- url
- label
properties:
url:
pizzapim marked this conversation as resolved Outdated

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.
type: string
example: "gotify://hostname/token"
label:
type: string
example: mastodon
description: |
A list of Apprise services to publish to with URLs
and labels. The labels are used for logging.
A full list of services and their configuration can be found
at https://github.com/caronc/apprise/wiki.
example:
- url: "kodi://user@hostname"
label: kodi
pizzapim marked this conversation as resolved Outdated

Typo.

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

Typo. Also, maybe this example body should be `start`-specific?
- url: "line://Token@User"
label: line
start:
type: object
pizzapim marked this conversation as resolved Outdated

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?
required: ['body']
properties:
title:
type: string
description: |
pizzapim marked this conversation as resolved Outdated

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?
Specify the message title. If left unspecified, no
title is sent.
example: Ping!
body:
type: string
description: |
Specify the message body.
example: Starting backup process.
finish:
type: object
required: ['body']
properties:
title:
type: string
description: |
pizzapim marked this conversation as resolved Outdated

Typo.

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

Typo. Also, maybe this example body should be `finish`-specific?
Specify the message title. If left unspecified, no
title is sent.
example: Ping!
body:
type: string
description: |
Specify the message body.
example: Backups successfully made.
fail:
type: object
required: ['body']
properties:
title:
type: string
description: |
Specify the message title. If left unspecified, no
title is sent.
example: Ping!
body:
type: string
description: |
Specify the message body.
example: Your backups have failed.
states:
pizzapim marked this conversation as resolved Outdated

Typo.

Typo.
type: array
items:
type: string
enum:
- start
- finish
- fail
uniqueItems: true
description: |
List of one or more monitoring states to ping for: "start",
"finish", and/or "fail". Defaults to pinging for failure
only. For each selected state, corresponding configuration
for the message title and body should be given. If any is
left unspecified, a generic message is emitted instead.
example:
- start
- finish
healthchecks:
type: object
required: ['ping_url']

View File

@ -0,0 +1,79 @@
import logging
import operator
logger = logging.getLogger(__name__)
def initialize_monitor(
ping_url, config, config_filename, monitoring_log_level, dry_run
): # pragma: no cover
'''
No initialization is necessary for this monitor.
'''
pass
def ping_monitor(hook_config, config, config_filename, state, monitoring_log_level, dry_run):
'''
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.
'''
try:
import apprise
pizzapim marked this conversation as resolved Outdated

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.
from apprise import NotifyFormat, NotifyType
except ImportError: # pragma: no cover
logger.warning('Unable to import Apprise in monitoring hook')
return
state_to_notify_type = {
'start': NotifyType.INFO,
'finish': NotifyType.SUCCESS,
'fail': NotifyType.FAILURE,
'log': NotifyType.INFO,
}
run_states = hook_config.get('states', ['fail'])
if state.name.lower() not in run_states:
return
pizzapim marked this conversation as resolved Outdated

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.
state_config = hook_config.get(
state.name.lower(),
{
pizzapim marked this conversation as resolved Outdated

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..
'title': f'A borgmatic {state.name} event happened',
'body': f'A borgmatic {state.name} event happened',
},
)
if not hook_config.get('services'):
pizzapim marked this conversation as resolved Outdated

Please spell this (apobj) out. Thank you!

Please spell this (`apobj`) out. Thank you!
logger.info(f'{config_filename}: No Apprise services to ping')
return
dry_run_string = ' (dry run; not actually pinging)' if dry_run else ''
pizzapim marked this conversation as resolved Outdated

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.
labels_string = ', '.join(map(operator.itemgetter('label'), hook_config.get('services')))
logger.info(f'{config_filename}: Pinging Apprise services: {labels_string}{dry_run_string}')
apprise_object = apprise.Apprise()
apprise_object.add(list(map(operator.itemgetter('url'), hook_config.get('services'))))
pizzapim marked this conversation as resolved Outdated

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 Outdated

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.
if dry_run:
return
pizzapim marked this conversation as resolved Outdated

Similar here to the comment above.

Similar here to the comment above.
result = apprise_object.notify(
title=state_config.get('title', ''),
body=state_config.get('body'),
body_format=NotifyFormat.TEXT,
pizzapim marked this conversation as resolved Outdated

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

Please spell out variable names. Yes, I know it's obnoxious. 😄
notify_type=state_to_notify_type[state.name.lower()],
)
if result is False:
pizzapim marked this conversation as resolved Outdated

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).
logger.warning(f'{config_filename}: Error sending some Apprise notifications')
witten marked this conversation as resolved Outdated

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...

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

Exactly like you said, the function returns either a Boolean or None
pizzapim marked this conversation as resolved Outdated

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?
def destroy_monitor(
pizzapim marked this conversation as resolved Outdated

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.
ping_url_or_uuid, config, config_filename, monitoring_log_level, dry_run
): # pragma: no cover
'''
No destruction is necessary for this monitor.
'''
pass

View File

@ -1,6 +1,7 @@
import logging
from borgmatic.hooks import (
apprise,
cronhub,
cronitor,
healthchecks,
@ -17,6 +18,7 @@ from borgmatic.hooks import (
logger = logging.getLogger(__name__)
HOOK_NAME_TO_MODULE = {
'apprise': apprise,
'cronhub': cronhub,
'cronitor': cronitor,
'healthchecks': healthchecks,

View File

@ -1,6 +1,6 @@
from enum import Enum
MONITOR_HOOK_NAMES = ('healthchecks', 'cronitor', 'cronhub', 'pagerduty', 'ntfy', 'loki')
MONITOR_HOOK_NAMES = ('apprise', 'healthchecks', 'cronitor', 'cronhub', 'pagerduty', 'ntfy', 'loki')
class State(Enum):

View File

@ -36,6 +36,7 @@ setup(
'ruamel.yaml>0.15.0,<0.18.0',
'setuptools',
),
pizzapim marked this conversation as resolved Outdated

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.
extras_require={"Apprise": ["apprise"]},
include_package_data=True,
python_requires='>=3.7',
)

View File

@ -1,6 +1,8 @@
appdirs==1.4.4; python_version >= '3.8'
apprise==1.3.0
attrs==22.2.0; python_version >= '3.8'
black==23.3.0; python_version >= '3.8'
certifi==2022.9.24
chardet==5.1.0
click==8.1.3; python_version >= '3.8'
codespell==2.2.4
@ -14,16 +16,18 @@ flexmock==0.11.3
idna==3.4
importlib_metadata==6.3.0; python_version < '3.8'
isort==5.12.0
jsonschema==4.17.3
Markdown==3.4.1
mccabe==0.7.0
packaging==23.1
pluggy==1.0.0
pathspec==0.11.1; python_version >= '3.8'
pluggy==1.0.0
py==1.11.0
pycodestyle==2.10.0
pyflakes==3.0.1
jsonschema==4.17.3
pytest==7.3.0
pytest-cov==4.0.0
PyYAML==6.0
regex; python_version >= '3.8'
requests==2.31.0
ruamel.yaml>0.15.0,<0.18.0

View File

@ -0,0 +1,208 @@
import apprise
from apprise import NotifyFormat, NotifyType
from flexmock import flexmock
import borgmatic.hooks.monitor
from borgmatic.hooks import apprise as module
pizzapim marked this conversation as resolved
Review

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

Thanks so much for following testing convention in this code base!
TOPIC = 'borgmatic-unit-testing'
pizzapim marked this conversation as resolved Outdated

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).
def mock_apprise():
apprise_mock = flexmock(
pizzapim marked this conversation as resolved Outdated

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`.
add=lambda servers: None, notify=lambda title, body, body_format, notify_type: None
)
flexmock(apprise.Apprise).new_instances(apprise_mock)
return apprise_mock
def test_ping_monitor_adheres_dry_run():
mock_apprise().should_receive('notify').never()
module.ping_monitor(
{'services': [{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'}]},
{},
'config.yaml',
borgmatic.hooks.monitor.State.FAIL,
monitoring_log_level=1,
dry_run=True,
)
def test_ping_monitor_does_not_hit_with_no_states():
mock_apprise().should_receive('notify').never()
module.ping_monitor(
{'services': [{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'}], 'states': []},
{},
pizzapim marked this conversation as resolved
Review

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?
'config.yaml',
borgmatic.hooks.monitor.State.FAIL,
monitoring_log_level=1,
dry_run=True,
)
def test_ping_monitor_hits_fail_by_default():
mock_apprise().should_receive('notify').with_args(
title='A borgmatic FAIL event happened',
body='A borgmatic FAIL event happened',
body_format=NotifyFormat.TEXT,
notify_type=NotifyType.FAILURE,
).once()
for state in borgmatic.hooks.monitor.State:
module.ping_monitor(
{'services': [{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'}]},
{},
'config.yaml',
state,
monitoring_log_level=1,
dry_run=False,
)
def test_ping_monitor_hits_with_finish_default_config():
mock_apprise().should_receive('notify').with_args(
title='A borgmatic FINISH event happened',
body='A borgmatic FINISH event happened',
body_format=NotifyFormat.TEXT,
notify_type=NotifyType.SUCCESS,
).once()
module.ping_monitor(
{'services': [{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'}], 'states': ['finish']},
{},
'config.yaml',
borgmatic.hooks.monitor.State.FINISH,
monitoring_log_level=1,
dry_run=False,
)
def test_ping_monitor_hits_with_start_default_config():
mock_apprise().should_receive('notify').with_args(
title='A borgmatic START event happened',
body='A borgmatic START event happened',
body_format=NotifyFormat.TEXT,
notify_type=NotifyType.INFO,
).once()
module.ping_monitor(
{'services': [{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'}], 'states': ['start']},
{},
'config.yaml',
borgmatic.hooks.monitor.State.START,
monitoring_log_level=1,
dry_run=False,
)
def test_ping_monitor_hits_with_fail_default_config():
mock_apprise().should_receive('notify').with_args(
title='A borgmatic FAIL event happened',
body='A borgmatic FAIL event happened',
body_format=NotifyFormat.TEXT,
notify_type=NotifyType.FAILURE,
).once()
module.ping_monitor(
{'services': [{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'}], 'states': ['fail']},
{},
'config.yaml',
borgmatic.hooks.monitor.State.FAIL,
monitoring_log_level=1,
dry_run=False,
)
def test_ping_monitor_hits_with_log_default_config():
mock_apprise().should_receive('notify').with_args(
title='A borgmatic LOG event happened',
body='A borgmatic LOG event happened',
body_format=NotifyFormat.TEXT,
notify_type=NotifyType.INFO,
pizzapim marked this conversation as resolved
Review

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.
).once()
module.ping_monitor(
{'services': [{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'}], 'states': ['log']},
{},
'config.yaml',
borgmatic.hooks.monitor.State.LOG,
monitoring_log_level=1,
dry_run=False,
)
def test_ping_monitor_passes_through_custom_message_title():
mock_apprise().should_receive('notify').with_args(
title='foo',
body='bar',
body_format=NotifyFormat.TEXT,
notify_type=NotifyType.FAILURE,
).once()
module.ping_monitor(
{
'services': [{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'}],
'states': ['fail'],
'fail': {'title': 'foo', 'body': 'bar'},
},
{},
'config.yaml',
borgmatic.hooks.monitor.State.FAIL,
monitoring_log_level=1,
dry_run=False,
)
def test_ping_monitor_passes_through_custom_message_body():
mock_apprise().should_receive('notify').with_args(
title='',
body='baz',
body_format=NotifyFormat.TEXT,
notify_type=NotifyType.FAILURE,
).once()
module.ping_monitor(
{
'services': [{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'}],
'states': ['fail'],
'fail': {'body': 'baz'},
},
{},
'config.yaml',
borgmatic.hooks.monitor.State.FAIL,
monitoring_log_level=1,
dry_run=False,
)
def test_ping_monitor_pings_multiple_services():
mock_apprise().should_receive('add').with_args([f'ntfys://{TOPIC}', f'ntfy://{TOPIC}']).once()
module.ping_monitor(
{
'services': [
{'url': f'ntfys://{TOPIC}', 'label': 'ntfys'},
{'url': f'ntfy://{TOPIC}', 'label': 'ntfy'},
]
},
{},
'config.yaml',
borgmatic.hooks.monitor.State.FAIL,
monitoring_log_level=1,
dry_run=False,
)
def test_ping_monitor_warning_for_no_services():
flexmock(module.logger).should_receive('info').once()
module.ping_monitor(
{'services': []},
{},
'config.yaml',
borgmatic.hooks.monitor.State.FAIL,
monitoring_log_level=1,
dry_run=False,
)