WIP: Add ntfy warning #664

phfn · 2023-03-31T11:11:44Z

phfn commented

2023-03-31 11:11:44 +00:00

First-time contributor

Add a ntfy warn hook. See: borgmatic-collective/borgmatic#606

phfn added 1 commit 2023-03-31 11:11:46 +00:00

ee1beb9b5b Add ntfy warning

phfn commented

2023-03-31 11:14:09 +00:00

First-time contributor

Still needs documentation. Also, if it'n not to complicated, i would add a on_warning hook.

Still needs documentation. Also, if it'n not to complicated, i would add a `on_warning` hook.

witten reviewed 2023-03-31 17:37:19 +00:00

witten left a comment

Thanks for diving into a PR for this! I really appreciate it.

The changes you've made to ntfy schema look like they would totally work. But if I'm reading the rest of the code correctly, I'm not sure the approach works for a couple reasons—one pretty easily surmountable and one more thorny:

If you look at the exit_code_indicates_error() section within log_outputs(), it does a bunch of cleanup stuff before raising. This includes gathering log output to stuff into the exception, killing other processes to prevent hangs, etc. So you'd presumably need to do at least some of that if raising for warnings as well. Also, don't forget the captured_outputs return at the bottom of log_outputs().
The thornier issue: Raising for a warning seems like it'll stop execution of all borgmatic actions before the exception gets caught way up in command/borgmatic.py. That means if you run borgmatic create check, for instance, and Borg issues a warning during create, that'll prevent the check from happening entirely.

So my reaction is that using a raised exception to signal a warning state won't achieve what you're looking for—finding out about Borg warnings without interrupting borgmatic. If you wanted to address this issue, I think it would have to be by doing something like: Let borgmatic run its course normally, and then after all actions run, check if any of them exited with warnings. That information is unfortunately not readily available now, but it could potentially be bubbled up through successive calls and collected at the commands/borgmatic.py level. Then you could confidently ping monitors including ntfy with the warning.

I'm of course open to other approaches, especially if I'm missing something here!

Thanks for diving into a PR for this! I really appreciate it. The changes you've made to `ntfy` schema look like they would totally work. But if I'm reading the rest of the code correctly, I'm not sure the approach works for a couple reasons—one pretty easily surmountable and one more thorny: 1. If you look at the `exit_code_indicates_error()` section within `log_outputs()`, it does a bunch of cleanup stuff before raising. This includes gathering log output to stuff into the exception, killing other processes to prevent hangs, etc. So you'd presumably need to do at least some of that if raising for warnings as well. Also, don't forget the `captured_outputs` return at the bottom of `log_outputs()`. 2. The thornier issue: Raising for a warning seems like it'll stop execution of all borgmatic actions before the exception gets caught way up in `command/borgmatic.py`. That means if you run `borgmatic create check`, for instance, and Borg issues a warning during `create`, that'll prevent the `check` from happening entirely. So my reaction is that using a raised exception to signal a warning state won't achieve what you're looking for—finding out about Borg warnings without interrupting borgmatic. If you wanted to address this issue, I think it would have to be by doing something like: Let borgmatic run its course normally, and then after all actions run, check if any of them exited with warnings. That information is unfortunately not readily available now, but it could potentially be bubbled up through successive calls and collected at the `commands/borgmatic.py` level. Then you could confidently ping monitors including `ntfy` with the warning. I'm of course open to other approaches, especially if I'm missing something here!

witten commented

2023-06-27 22:03:09 +00:00

Hey @phfn, I'm just checking in on this PR. Do you intend to follow up when you get a chance? I realize the work item may have gotten a little bigger in scope than you originally expected.

phfn commented

2023-07-03 09:43:39 +00:00

First-time contributor

Hey, yeah the work actually was a bit bigger than i thought. I'm still interested in, but currently i'm working on my Thesis, so i don't have much time left. You can close this PR and i'll open one, when i have more time.