borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
15
57
Fork 40
Code Issues 54 Pull Requests 2 Actions Packages Releases 121 Activity

add a hook for sending push notifications via ntfy.sh #543

Merged
witten merged 1 commits from g-a-c/borgmatic:feat/ntfy into master 2022-06-09 20:26:06 +00:00
Conversation 10 Commits 1 Files Changed 6 +371 -6
g-a-c commented 2022-06-08 21:26:38 +00:00
Contributor
Copy Link

Out of the current monitoring options (Healthchecks.io, Cronhub, Cronitor, and PagerDuty) I didn't see any free options which would provide real-time push notifications. Healthchecks.io allows 5 SMS or WhatsApp messages per month but this is kind of limiting.

So I decided since I had a use case, and felt like a little bit of hacking on something, I'd add a hook for Ntfy that I would personally find useful, and hopefully others might too.

It can be configured as simply as providing a topic, which will be enough to publish a message on failure, or there are more (documented) options to allow more configuration.

Out of the current monitoring options (Healthchecks.io, Cronhub, Cronitor, and PagerDuty) I didn't see any free options which would provide real-time push notifications. Healthchecks.io allows 5 SMS or WhatsApp messages per month but this is kind of limiting. So I decided since I had a use case, and felt like a little bit of hacking on something, I'd add a hook for [Ntfy](https://ntfy.sh) that I would personally find useful, and hopefully others might too. It can be configured as simply as providing a `topic`, which will be enough to publish a message on failure, or there are more (documented) options to allow more configuration.
IMG_899D646EDBA6-1.jpeg
351 KiB
IMG_899D646EDBA6-1.jpeg
g-a-c force-pushed feat/ntfy from 506907e6b6 to 0ff5c6192c 2022-06-08 21:31:45 +00:00 Compare
witten commented 2022-06-08 22:10:19 +00:00
Owner
Copy Link

OMG this is great. And documentation, too! Thanks so much for taking the time to add this functionality. The only thing I see potentially missing is tests for ping_monitor(). Do you want to take a stab at that, or would you prefer I do them? You can grep for test_ping_monitor if you want inspiration.

OMG this is great. And documentation, too! Thanks so much for taking the time to add this functionality. The only thing I see potentially missing is tests for `ping_monitor()`. Do you want to take a stab at that, or would you prefer I do them? You can grep for `test_ping_monitor` if you want inspiration.
g-a-c commented 2022-06-09 09:25:25 +00:00
Author
Contributor
Copy Link

I'm happy to take a look and finish the job, somehow I totally missed seeing the tests/unit/hooks directory and assumed that only the core Borgmatic functionality was tested and not external services. I'll do that this evening and push another commit

I'm happy to take a look and finish the job, somehow I totally missed seeing the `tests/unit/hooks` directory and assumed that only the core Borgmatic functionality was tested and not external services. I'll do that this evening and push another commit
g-a-c force-pushed feat/ntfy from e218aec8ce to 4a91f6a741 2022-06-09 11:25:09 +00:00 Compare
g-a-c force-pushed feat/ntfy from 4a91f6a741 to a7a99799fc 2022-06-09 11:27:19 +00:00 Compare
g-a-c reviewed 2022-06-09 11:29:49 +00:00
borgmatic/hooks/ntfy.py Outdated
@ -0,0 +6,4 @@
logger = logging.getLogger(__name__)
MONITOR_STATE_TO_NTFY = {
g-a-c commented 2022-06-09 11:29:49 +00:00
Author
Contributor
Copy Link

This is not really necessary, but had to be done to keep flake8 happy because otherwise borgmatic.hooks.monitor was imported due to being required by the tests, even though it was not referenced within this hook directly.

This is not really necessary, but had to be done to keep `flake8` happy because otherwise `borgmatic.hooks.monitor` was imported due to being required by the tests, even though it was not referenced within this hook directly.
witten commented 2022-06-09 17:43:30 +00:00
Owner
Copy Link

That seems unnecessary to me, given that it's never used...? Your tests can always import directly from borgmatic.hooks.monitor rather than relying on module.monitor. That's just a little convention in borgmatic tests to make sure you're using the exact same import that the code under test is importing. But no need to bend over backwards to do it!

That seems unnecessary to me, given that it's never used...? Your tests can always import directly from `borgmatic.hooks.monitor` rather than relying on `module.monitor`. That's just a little convention in borgmatic tests to make sure you're using the exact same import that the code under test is importing. But no need to bend over backwards to do it!
g-a-c force-pushed feat/ntfy from a7a99799fc to d0bb740171 2022-06-09 11:36:44 +00:00 Compare
g-a-c commented 2022-06-09 11:54:07 +00:00
Author
Contributor
Copy Link

Too much multi-tasking with my day job and a couple of force-pushed later, and this should be reviewable...the tests don't exhaustively cover every possible scenario but they do cover:

  • minimal config (topic set only)
  • self-hosted config (topic and server set)
  • custom fail messages (topic and fail set)
  • custom states with the default message (topic and states set)
  • dry run (topic set, with dryrun set on the test)
Too much multi-tasking with my day job and a couple of force-pushed later, and this should be reviewable...the tests don't exhaustively cover every possible scenario but they do cover: - minimal config (`topic` set only) - self-hosted config (`topic` and `server` set) - custom fail messages (`topic` and `fail` set) - custom states with the default message (`topic` and `states` set) - dry run (`topic` set, with `dryrun` set on the test)
g-a-c commented 2022-06-09 14:02:46 +00:00
Author
Contributor
Copy Link

So I realised I was missing one last test to get 100% code coverage, which is the "connection error" test. I stole the one from one of the other tests as this seemed a relatively simple one, but as it is in the most recent commit, it fails, and it seems to be not necessarily related to my hook but related to logging:

self = <Logger borgmatic.hooks.ntfy (WARNING)>, record = <LogRecord: borgmatic.hooks.ntfy, 30, /Users/g-a-c/Development/borgmatic/borgmatic/hooks/ntfy.py, 70, "config.yaml: Ntfy error: ">

    def callHandlers(self, record):
        """
        Pass a record to all relevant handlers.

        Loop through all handlers for this logger and its parents in the
        logger hierarchy. If no handler was found, output a one-off error
        message to sys.stderr. Stop searching up the hierarchy whenever a
        logger with the "propagate" attribute set to zero is found - that
        will be the last logger whose handlers are called.
        """
        c = self
        found = 0
        while c:
            for hdlr in c.handlers:
                found = found + 1
>               if record.levelno >= hdlr.level:
E               AttributeError: 'NoneType' object has no attribute 'level'

/usr/local/Cellar/python@3.9/3.9.13_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/logging/__init__.py:1660: AttributeError

So this raises a couple of questions I had:

  • what is the purpose of the logging.getLogger('urllib3').setLevel() call? I don't see anywhere that a logger called urllib3 is either created or used (unless this happens transparently through Requests?) it is transparently used by Requests, and urllib3 as the underlying library - answered my own question there...
  • why does the Cronitor hook change the log level of that logger, when the except block is going to output through the __name__ logger anyway?
  • why does test_cronitor.py (for example) pass this same test when the logger object is instantiated identically in both cronitor.py and ntfy.py?
So I realised I was missing one last test to get 100% code coverage, which is the "connection error" test. I stole the one from one of the other tests as this seemed a relatively simple one, but as it is in the most recent commit, it fails, and it seems to be not necessarily related to my hook but related to logging: ``` self = <Logger borgmatic.hooks.ntfy (WARNING)>, record = <LogRecord: borgmatic.hooks.ntfy, 30, /Users/g-a-c/Development/borgmatic/borgmatic/hooks/ntfy.py, 70, "config.yaml: Ntfy error: "> def callHandlers(self, record): """ Pass a record to all relevant handlers. Loop through all handlers for this logger and its parents in the logger hierarchy. If no handler was found, output a one-off error message to sys.stderr. Stop searching up the hierarchy whenever a logger with the "propagate" attribute set to zero is found - that will be the last logger whose handlers are called. """ c = self found = 0 while c: for hdlr in c.handlers: found = found + 1 > if record.levelno >= hdlr.level: E AttributeError: 'NoneType' object has no attribute 'level' /usr/local/Cellar/python@3.9/3.9.13_1/Frameworks/Python.framework/Versions/3.9/lib/python3.9/logging/__init__.py:1660: AttributeError ``` So this raises a couple of questions I had: - ~~what is the purpose of the `logging.getLogger('urllib3').setLevel()` call? I don't see anywhere that a logger called `urllib3` is either created or used (unless this happens transparently through Requests?)~~ it is transparently used by Requests, and `urllib3` as the underlying library - answered my own question there... - why does the Cronitor hook change the log level of **that** logger, when the `except` block is going to output through the `__name__` logger anyway? - why does `test_cronitor.py` (for example) pass this same test when the `logger` object is instantiated identically in both `cronitor.py` and `ntfy.py`?
witten commented 2022-06-09 17:50:09 +00:00
Owner
Copy Link
  • what is the purpose of the logging.getLogger('urllib3').setLevel() call? I don't see anywhere that a logger called urllib3 is either created or used (unless this happens transparently through Requests?) it is transparently used by Requests, and urllib3 as the underlying library - answered my own question there...
  • why does the Cronitor hook change the log level of that logger, when the except block is going to output through the name logger anyway?

Yeah, IIRC, requests' use of urllib3 had some pretty verbose logging by default—and it uses its own urllib3 logger rather than the global logger—so this setLevel() call is just to suppress (most of) its logging.

  • why does test_cronitor.py (for example) pass this same test when the logger object is instantiated identically in both cronitor.py and ntfy.py?

So, about that. This one is pretty bad, but I was able to track it down. I ended up having to hack Python logging's addHandler() and raising when hdlr was None so I could see where a None handler was getting set. Turned out, it was in the tests for Healthchecks! Apparently those tests were (accidentally, implicitly) adding a None handler and then not cleaning it up, thereby impacting downstream tests. Your Ntfy tests were lucky enough to be after the Healthchecks tests, while the Cronitor tests came before both. I think the fix is to make the Healthchecks tests not change any global state—a pretty clear no-no in any testing.

I'll take on this task in borgmatic master, and let you know when that's fixed!

> * what is the purpose of the logging.getLogger('urllib3').setLevel() call? I don't see anywhere that a logger called urllib3 is either created or used (unless this happens transparently through Requests?) it is transparently used by Requests, and urllib3 as the underlying library - answered my own question there... > * why does the Cronitor hook change the log level of that logger, when the except block is going to output through the __name__ logger anyway? Yeah, IIRC, requests' use of urllib3 had some pretty verbose logging by default—and it uses its own urllib3 logger rather than the global logger—so this `setLevel()` call is just to suppress (most of) its logging. > * why does test_cronitor.py (for example) pass this same test when the logger object is instantiated identically in both cronitor.py and ntfy.py? So, about that. This one is pretty bad, but I was able to track it down. I ended up having to hack Python logging's `addHandler()` and raising when `hdlr` was None so I could see where a None handler was getting set. Turned out, it was in the tests for Healthchecks! Apparently those tests were (accidentally, implicitly) adding a None handler and then *not cleaning it up*, thereby impacting downstream tests. Your Ntfy tests were lucky enough to be *after* the Healthchecks tests, while the Cronitor tests came before both. I think the fix is to make the Healthchecks tests not change any global state—a pretty clear no-no in any testing. I'll take on this task in borgmatic master, and let you know when that's fixed!
witten commented 2022-06-09 18:08:22 +00:00
Owner
Copy Link

Okay, this is fixed in master! Feel free to pull that changeset in. Sorry you had to discover this the hard way.

Okay, this is [fixed in master](https://projects.torsion.org/borgmatic-collective/borgmatic/commit/3561c93d74e7bafb3308c83788684761338775e3)! Feel free to pull that changeset in. Sorry you had to discover this the hard way.
witten reviewed 2022-06-09 18:22:00 +00:00
witten left a comment
Owner
Copy Link

Lookin' really good. As far as I'm concerned, this is ready to go once tests are passing!

Lookin' really good. As far as I'm concerned, this is ready to go once tests are passing!
borgmatic/hooks/ntfy.py Outdated
@ -0,0 +30,4 @@
run_states = hook_config.get('states', ['fail'])
if state.name.lower() in run_states:
witten commented 2022-06-09 18:12:36 +00:00
Owner
Copy Link

Might not need to .lower() this, since states in configuration is an enum? Meaning there's no way to make those values uppercase..

Similar below.

Might not need to `.lower()` this, since `states` in configuration is an `enum`? Meaning there's no way to make those values uppercase.. Similar below.
g-a-c commented 2022-06-09 19:56:09 +00:00
Author
Contributor
Copy Link

It is an enum, but my understanding (which appears to be backed up by my testing) is that when it's retrieved, because the enum is capitalised on the left hand side the .name is uppercase, and it feels clunky making the user fill out this state list in the config in uppercase when nothing else is?

It is an `enum`, but my understanding (which _appears_ to be backed up by my testing) is that when it's retrieved, because the `enum` is capitalised on the left hand side the `.name` is uppercase, and it feels clunky making the user fill out this state list in the config in uppercase when nothing else is?
witten commented 2022-06-09 20:00:02 +00:00
Owner
Copy Link

Right you are! My bad.. I didn't remember that the state passed into ping_monitor() didn't come from the configuration schema enum.

Right you are! My bad.. I didn't remember that the `state` passed into `ping_monitor()` didn't come from the configuration schema enum.
witten marked this conversation as resolved
borgmatic/hooks/ntfy.py Outdated
@ -0,0 +36,4 @@
state_config = hook_config.get(
state.name.lower(),
{
'title': 'A Borgmatic event happened',
witten commented 2022-06-09 18:13:32 +00:00
Owner
Copy Link

You could interpolate the state name into the title/message here. (Do not feel strongly.)

You *could* interpolate the state name into the title/message here. (Do not feel strongly.)
👍 1
borgmatic/hooks/ntfy.py Outdated
@ -0,0 +46,4 @@
base_url = hook_config.get('server', 'https://ntfy.sh')
topic = hook_config.get('topic')
logger.info('{}: Pinging ntfy topic {}{}'.format(config_filename, topic, dry_run_label))
witten commented 2022-06-09 18:14:25 +00:00
Owner
Copy Link

You could use a Python f-string here and elsewhere. (Do not feel strongly.)

You could use a Python f-string here and elsewhere. (Do not feel strongly.)
g-a-c commented 2022-06-09 19:24:52 +00:00
Author
Contributor
Copy Link

Yeah, I actually wasn't aware of the f-string until I was poking around in other modules to try and work out that test issue, but I do find that cleaner to read so I think I'll refactor to use those.

Yeah, I actually wasn't aware of the f-string until I was poking around in other modules to try and work out that test issue, but I do find that cleaner to read so I think I'll refactor to use those.
👍 1
docs/how-to/monitor-your-backups.md Outdated
@ -273,0 +308,4 @@
fail:
title: A Borgmatic backup failed
message: You should probably fix it
tags: borgmatic,-1,skull
witten commented 2022-06-09 18:15:51 +00:00
Owner
Copy Link

Love it. (The skull.)

Love it. (The skull.)
👍 1
tests/unit/hooks/test_ntfy.py Outdated
@ -0,0 +45,4 @@
def test_ping_monitor_minimal_config_does_not_hit_hosted_ntfy_on_start():
hook_config = {
'topic': topic,
}
witten commented 2022-06-09 18:18:45 +00:00
Owner
Copy Link

Could all be on one line. (Do not feel strongly.)

Similar elsewhere.

Could all be on one line. (Do not feel strongly.) Similar elsewhere.
g-a-c commented 2022-06-09 19:26:12 +00:00
Author
Contributor
Copy Link

I think one of the auto-formatters may have done this, as I don't remember doing it for these simple single-element dicts.

I'll clean it up and see if it comes back when black or flake8 get run.

I _think_ one of the auto-formatters may have done this, as I don't remember doing it for these simple single-element `dict`s. I'll clean it up and see if it comes back when `black` or `flake8` get run.
👍 1
tests/unit/hooks/test_ntfy.py Outdated
@ -0,0 +134,4 @@
module.monitor.State.FAIL,
monitoring_log_level=1,
dry_run=False,
)
witten commented 2022-06-09 18:21:07 +00:00
Owner
Copy Link

These tests look great! I appreciate you taking the time to add them. IMO not every possible permutation needs testing as long as most of the code paths are hit. (Which they are here.)

These tests look great! I appreciate you taking the time to add them. IMO not every possible permutation needs testing as long as most of the code paths are hit. (Which they are here.)
👍 1
g-a-c commented 2022-06-09 19:44:10 +00:00
Author
Contributor
Copy Link

Sorry you had to discover this the hard way.

💩 happens! Sounds like it was an experience for both of us.

That does make a ton of sense, because when I ran just my ntfy test to get it to go a little quicker, it passed in isolation. I guess if I'd done some more messing around I might have worked out that when I ran all of the hook tests together the problem would have come back and isolated it that way - but I don't think I'd have come any closer to tracking down the root cause myself

> Sorry you had to discover this the hard way. :poop: happens! Sounds like it was an experience for both of us. That does make a ton of sense, because when I ran **just** my `ntfy` test to get it to go a little quicker, it passed in isolation. I guess if I'd done some more messing around I might have worked out that when I ran all of the `hook` tests together the problem would have come back and isolated it that way - but I don't think I'd have come any closer to tracking down the root cause myself
👍 1
g-a-c force-pushed feat/ntfy from e0be85cbcb to a8b8d507b6 2022-06-09 20:11:10 +00:00 Compare
g-a-c commented 2022-06-09 20:13:05 +00:00
Author
Contributor
Copy Link

I've squashed everything down to a single commit instead of my chopping and hacking around, 100% code coverage, black, isort and flake8 are all happy so I think this is ready for reviewing again...

I've squashed everything down to a single commit instead of my chopping and hacking around, 100% code coverage, `black`, `isort` and `flake8` are all happy so I think this is ready for reviewing again...
witten commented 2022-06-09 20:25:23 +00:00
Owner
Copy Link

Awesome! Thanks again for doing this—and slogging through the tests. This will be part of the next release.

Awesome! Thanks again for doing this—and slogging through the tests. This will be part of the next release.
🎉 1
witten merged commit 7648bcff39 into master 2022-06-09 20:26:06 +00:00
g-a-c deleted branch feat/ntfy 2022-06-09 21:04:23 +00:00
witten commented 2022-06-09 23:41:08 +00:00
Owner
Copy Link

Released in borgmatic 1.6.3!

Released in borgmatic 1.6.3!
🎉 1
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

   
data loss

   
design finalized

   
good first issue

   
new feature area

   
question / support

   
security

   
waiting for response
No Label
bug
data loss
design finalized
good first issue
new feature area
question / support
security
waiting for response
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
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#543
No description provided.
Delete Branch "g-a-c/borgmatic:feat/ntfy"

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?