Support for Apprise #759
|
@ -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
|
||||
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
witten
commented
Typo. Also, maybe this example body should be 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
witten
commented
Does this even need to be configurable at the borgmatic config level? For instance, I'd imagine that 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
witten
commented
Is it worth saying what the defaults are for 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
witten
commented
Typo. Also, maybe this example body should be 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
witten
commented
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']
|
||||
|
|
|
@ -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
witten
commented
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
witten
commented
What do you think of making 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
witten
commented
Why is this 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
witten
commented
Please spell this ( 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
witten
commented
An alternative:
Or even just:
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
witten
commented
These could be moved inline to the 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
witten
commented
My proposal is instead of allowing the user to specify the notification type, just use your 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
witten
commented
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
witten
commented
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
witten
commented
Code style nit: Closing paren on new line, lined up with start of You may be able to get this reformatted for you automatically by running 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
witten
commented
`if not result:` would be more idiomatic Python IMO. Unless you're trying to distinguish from `result` being `None` that is...
pizzapim
commented
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
witten
commented
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
witten
commented
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
|
|
@ -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,
|
||||
|
|
|
@ -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):
|
||||
|
|
1
setup.py
|
@ -36,6 +36,7 @@ setup(
|
|||
'ruamel.yaml>0.15.0,<0.18.0',
|
||||
'setuptools',
|
||||
),
|
||||
pizzapim marked this conversation as resolved
Outdated
witten
commented
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 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',
|
||||
)
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
witten
commented
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
witten
commented
Super minor, but constant names at the global scope are usually all caps ( 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
witten
commented
Nice mocking, but IMO Mocking it might be as easy as something like:
This probably also goes for all the other tests of this unit ( 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
witten
commented
This description suggests it's testing that the 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
witten
commented
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: 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,
|
||||
)
|
Note that borgmatic already has a "native" PagerDuty integration, so a different example may make more sense here.