Support for Apprise #759
|
@ -1311,15 +1311,28 @@ properties:
|
|||
required: ['service_urls']
|
||||
additionalProperties: false
|
||||
properties:
|
||||
service_urls:
|
||||
type: array
|
||||
items:
|
||||
type: string
|
||||
services:
|
||||
type: object
|
||||
required:
|
||||
- url
|
||||
- label
|
||||
properties:
|
||||
url:
|
||||
type: string
|
||||
example: "mastodon://accesskey/host/?visibility=direct"
|
||||
pizzapim marked this conversation as resolved
Outdated
|
||||
label:
|
||||
type: string
|
||||
example: mastodon
|
||||
description: |
|
||||
List of Apprise service URLs to publish to.
|
||||
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:
|
||||
- "mastodon://accesskey/host/?visibility=direct"
|
||||
- "pagerduty://A1BRTD4JD@TIiajkdnlazkcOXrIdevi7F/node01.local/drive_sda/"
|
||||
- url: "slack://xoxb-1234-1234-4ddbaae6f3523ada2d/#backups"
|
||||
label: slackbackups
|
||||
- url: "matrixs://nuxref:abc123@matrix.example.com/#general/#backups"
|
||||
label: matrix
|
||||
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?
|
||||
start:
|
||||
type: object
|
||||
properties:
|
||||
|
@ -1332,18 +1345,7 @@ properties:
|
|||
type: string
|
||||
description: |
|
||||
Specify the message body.
|
||||
exampe: Your backups have failed.
|
||||
notification_type:
|
||||
type: string
|
||||
description: |
|
||||
The Apprise message type.
|
||||
enum:
|
||||
- info
|
||||
- success
|
||||
- failure
|
||||
- warning
|
||||
example:
|
||||
- failure
|
||||
example: Starting backup process.
|
||||
finish:
|
||||
type: object
|
||||
properties:
|
||||
|
@ -1356,18 +1358,7 @@ properties:
|
|||
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 body.
|
||||
exampe: Your backups have failed.
|
||||
notification_type:
|
||||
type: string
|
||||
description: |
|
||||
The Apprise message type.
|
||||
enum:
|
||||
- info
|
||||
- success
|
||||
- failure
|
||||
- warning
|
||||
example:
|
||||
- failure
|
||||
example: Backups successfully made.
|
||||
fail:
|
||||
type: object
|
||||
properties:
|
||||
|
@ -1380,18 +1371,7 @@ properties:
|
|||
type: string
|
||||
description: |
|
||||
Specify the message body.
|
||||
exampe: Your backups have failed.
|
||||
notification_type:
|
||||
type: string
|
||||
description: |
|
||||
The Apprise message type.
|
||||
enum:
|
||||
- info
|
||||
- success
|
||||
- failure
|
||||
- warning
|
||||
example:
|
||||
- failure
|
||||
example: Your backups have failed.
|
||||
states:
|
||||
type: array
|
||||
items:
|
||||
|
|
|
@ -5,6 +5,13 @@ from apprise import NotifyType, NotifyFormat
|
|||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
state_to_notify_type = {
|
||||
'start': NotifyType.INFO,
|
||||
'finish': NotifyType.SUCCESS,
|
||||
'fail': NotifyType.FAILURE,
|
||||
'log': NotifyType.INFO
|
||||
}
|
||||
|
||||
|
||||
def initialize_monitor(
|
||||
ping_url, config, config_filename, monitoring_log_level, dry_run
|
||||
|
@ -17,9 +24,8 @@ def initialize_monitor(
|
|||
|
||||
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.
|
||||
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.
|
||||
'''
|
||||
run_states = hook_config.get('states', ['fail'])
|
||||
|
||||
|
@ -30,60 +36,38 @@ def ping_monitor(hook_config, config, config_filename, state, monitoring_log_lev
|
|||
state.name.lower(),
|
||||
{
|
||||
'title': f'A borgmatic {state.name} event happened',
|
||||
'body': f'A borgmatic {state.name} event happened',
|
||||
'notification_type': default_notify_type(state.name.lower()),
|
||||
'body': f'A borgmatic {state.name} event happened'
|
||||
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.
|
||||
},
|
||||
)
|
||||
|
||||
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..
|
||||
# TODO: Currently not very meaningful message.
|
||||
# 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')
|
||||
if not hook_config.get('services'):
|
||||
logger.info(f'{config_filename}: No Apprise services to ping')
|
||||
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')))
|
||||
pizzapim marked this conversation as resolved
Outdated
witten
commented
Please spell this ( Please spell this (`apobj`) out. Thank you!
|
||||
logger.info(f'{config_filename}: Pinging Apprise services: {labels_string}{dry_run_string}')
|
||||
|
||||
title = state_config.get('title', '')
|
||||
body = state_config.get('body')
|
||||
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.
|
||||
notify_type = state_config.get('notification_type', 'success')
|
||||
notify_type = state_to_notify_type[state.name.lower()]
|
||||
|
||||
apobj = apprise.Apprise()
|
||||
apobj.add(hook_config.get('service_urls'))
|
||||
apprise_object = apprise.Apprise()
|
||||
apprise_object.add(map(lambda service: service['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.
|
||||
if dry_run:
|
||||
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.
|
||||
return
|
||||
|
||||
pizzapim marked this conversation as resolved
Outdated
witten
commented
Similar here to the comment above. Similar here to the comment above.
|
||||
result = apobj.notify(
|
||||
result = apprise_object.notify(
|
||||
title=title,
|
||||
body=body,
|
||||
body_format=NotifyFormat.TEXT,
|
||||
notify_type=get_notify_type(notify_type)
|
||||
)
|
||||
notify_type=notify_type)
|
||||
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. 😄
|
||||
|
||||
if result is False:
|
||||
logger.warning(f'{config_filename}: error sending some apprise notifications')
|
||||
|
||||
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).
|
||||
|
||||
def get_notify_type(s):
|
||||
if s == 'info':
|
||||
return NotifyType.INFO
|
||||
if s == 'success':
|
||||
return NotifyType.SUCCESS
|
||||
if s == 'warning':
|
||||
return NotifyType.WARNING
|
||||
if s == 'failure':
|
||||
return NotifyType.FAILURE
|
||||
|
||||
|
||||
def default_notify_type(state):
|
||||
if state == 'start':
|
||||
return NotifyType.INFO
|
||||
if state == 'finish':
|
||||
return NotifyType.SUCCESS
|
||||
if state == 'fail':
|
||||
return NotifyType.FAILURE
|
||||
if state == 'log':
|
||||
return NotifyType.INFO
|
||||
|
||||
|
||||
def destroy_monitor(
|
||||
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
|
||||
ping_url_or_uuid, config, config_filename, monitoring_log_level, dry_run
|
||||
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?
|
||||
): # pragma: no cover
|
||||
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.
|
||||
|
|
3
setup.py
|
@ -37,6 +37,9 @@ setup(
|
|||
'setuptools',
|
||||
'apprise'
|
||||
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.
|
||||
),
|
||||
extra_require={
|
||||
"Apprise": ["apprise"]
|
||||
},
|
||||
include_package_data=True,
|
||||
python_requires='>=3.7',
|
||||
)
|
||||
|
|
Note that borgmatic already has a "native" PagerDuty integration, so a different example may make more sense here.