WIP: Separate endpoint for each primary action for monitor hooks #660
|
@ -66,6 +66,7 @@ def run_configuration(config_filename, config, arguments):
|
|||
error_repository = ''
|
||||
using_primary_action = {'create', 'prune', 'compact', 'check'}.intersection(arguments)
|
||||
monitoring_log_level = verbosity_to_log_level(global_arguments.monitoring_verbosity)
|
||||
action_name = list(arguments.keys())[0]
|
||||
jetchirag marked this conversation as resolved
Outdated
|
||||
|
||||
try:
|
||||
jetchirag marked this conversation as resolved
Outdated
witten
commented
On the On the `isinstance()` call: Shouldn't they always be strings? Or is something else sneaking in there?
|
||||
local_borg_version = borg_version.local_borg_version(storage, local_path)
|
||||
|
@ -94,6 +95,7 @@ def run_configuration(config_filename, config, arguments):
|
|||
monitor.State.START,
|
||||
monitoring_log_level,
|
||||
global_arguments.dry_run,
|
||||
action_name,
|
||||
)
|
||||
except (OSError, CalledProcessError) as error:
|
||||
if command.considered_soft_failure(config_filename, error):
|
||||
witten
commented
I know we've going back and forth on the subject of looping over
I think that means with the actions loop in place and So, the question is what to do about this. One idea is to remove the above normalization code so that you can retain the prior behavior when I know we've going back and forth on the subject of looping over `ping_monitor` calls, but something occurs to me: Are there cases where this will ping the monitor multiple times even if it's only configured with a single `ping_url` option? I'm thinking of this code in `normalize.py`:
```
config['hooks']['cronitor'] = {
'create': cronitor['ping_url'],
'prune': cronitor['ping_url'],
'compact': cronitor['ping_url'],
'check': cronitor['ping_url'],
}
```
I think that means with the actions loop in place and `ping_url` set, Cronitor will get pinged *four* times for a standard borgmatic run. But shouldn't it ideally only get pinged *once* on start—unless the user explicitly configures ping endpoints for multiple actions?
So, the question is what to do about this. One idea is to remove the above normalization code so that you can retain the prior behavior when `ping_url` is set: Only ping *once* on start. Another idea is to drop support for `ping_url` entirely. There are probably other options as well. I'm open to discussion on this point of course! I didn't realize this would be such a thorny ticket.
jetchirag
commented
@witten I had this concern too since I started working on this. I was also wondering if someone wants to receive notification for a failed event in any of 4 actions, he will have to set the endpoints in each of them and same for future update. And suppose someone wants to get notified for failure in all of these actions, it will send alert 4 times if all of them fail, where probability of other act So what if we keep the ping_url but add a boolean configuration so user can select which action to receive notification on. I don't think people will use different cronitor endpoint for different actions of same job. This should still fulfil the problem raised in this PR's issue. It's been a while since I worked on this so I might be misunderstanding something here. So these are just the thoughts I gathered going over the code again. Let me know what you think. @witten I had this concern too since I started working on this. I was also wondering if someone wants to receive notification for a failed event in any of 4 actions, he will have to set the endpoints in each of them and same for future update.
And suppose someone wants to get notified for failure in all of these actions, it will send alert 4 times if all of them fail, where probability of other act
So what if we keep the ping_url but add a boolean configuration so user can select which action to receive notification on. I don't think people will use different cronitor endpoint for different actions of same job.
This should still fulfil the problem raised in this PR's issue.
It's been a while since I worked on this so I might be misunderstanding something here. So these are just the thoughts I gathered going over the code again. Let me know what you think.
witten
commented
It's also been a while since I've looked at this code! Could you maybe provide a configuration example to illustrate what you're thinking here? For instance, I'm unsure how an additional boolean configuration option would allow a user to select which borgmatic action to receive notifications on. I would think that any action with a Cronitor URL set would receive a notification. Or is the distinction you're drawing that, for a given action, URLs might only be notified for some of begin/start/fail and not all three?
Looking at #518, I think a common case is probably where the user wants one URL for, say, the For the current (released borgmatic) use case of pinging Cronitor only once for begin/start/fail when the action is create, prune, compact, or check.. What do you think of including some logic such that if a URL is specified for multiple different actions (whether explicitly.. or implicitly via I've given no thought to how that might be implemented, but such de-duplication seems nice from a user perspective—and it lines up with the currently released behavior. Let me know though if this doesn't sound feasible or desirable. It's also been a while since I've looked at this code! Could you maybe provide a configuration example to illustrate what you're thinking here? For instance, I'm unsure how an additional boolean configuration option would allow a user to select which borgmatic action to receive notifications on. I would think that any action with a Cronitor URL set would receive a notification. Or is the distinction you're drawing that, for a given action, URLs might only be notified for some of begin/start/fail and not all three?
> I don't think people will use different cronitor endpoint for different actions of same job.
Looking at #518, I think a common case *is* probably where the user wants one URL for, say, the `create` action and a completely different URL if any for, say, the `check` action. (See #366 as well.) In that case it wouldn't be a problem that each such URL would get pinged once upon failure, as each action is hitting a different Cronitor endpoint.
For the current (released borgmatic) use case of pinging Cronitor only once for begin/start/fail when the action is create, prune, compact, or check.. What do you think of including some logic such that if a URL is specified for multiple different actions (whether explicitly.. or implicitly via `ping_url`), then borgmatic is smart enough to only ping Cronitor once before/after all the actions and once on failure.
I've given no thought to how that might be implemented, but such de-duplication seems nice from a user perspective—and it lines up with the currently released behavior. Let me know though if this doesn't sound feasible or desirable.
jetchirag
commented
It makes sense to include logic to remove duplicate endpoints. I'll add that. I've fixed the latest merge conflicts and will start working on the rest of suggestions. Development might still be slow unfortunately since classes are starting but I'll keep at it. Since merge conflicts are fixed, I can solve the rest in small pieces. Thank you for the help and patience! It makes sense to include logic to remove duplicate endpoints. I'll add that.
I've fixed the latest merge conflicts and will start working on the rest of suggestions. Development might still be slow unfortunately since classes are starting but I'll keep at it. Since merge conflicts are fixed, I can solve the rest in small pieces.
Thank you for the help and patience!
|
||||
|
@ -163,6 +165,7 @@ def run_configuration(config_filename, config, arguments):
|
|||
monitor.State.LOG,
|
||||
monitoring_log_level,
|
||||
global_arguments.dry_run,
|
||||
action_name,
|
||||
)
|
||||
except (OSError, CalledProcessError) as error:
|
||||
if command.considered_soft_failure(config_filename, error):
|
||||
|
@ -182,6 +185,7 @@ def run_configuration(config_filename, config, arguments):
|
|||
monitor.State.FINISH,
|
||||
monitoring_log_level,
|
||||
global_arguments.dry_run,
|
||||
action_name,
|
||||
)
|
||||
dispatch.call_hooks(
|
||||
'destroy_monitor',
|
||||
|
@ -218,6 +222,7 @@ def run_configuration(config_filename, config, arguments):
|
|||
monitor.State.FAIL,
|
||||
monitoring_log_level,
|
||||
global_arguments.dry_run,
|
||||
action_name,
|
||||
)
|
||||
dispatch.call_hooks(
|
||||
'destroy_monitor',
|
||||
|
|
|
@ -27,6 +27,13 @@ def normalize(config_filename, config):
|
|||
cronitor = hooks.get('cronitor')
|
||||
if isinstance(cronitor, str):
|
||||
config['hooks']['cronitor'] = {'ping_url': cronitor}
|
||||
if isinstance(cronitor, dict) and 'ping_url' in cronitor:
|
||||
config['hooks']['cronitor'] = {
|
||||
'create': cronitor['ping_url'],
|
||||
'prune': cronitor['ping_url'],
|
||||
'compact': cronitor['ping_url'],
|
||||
'check': cronitor['ping_url'],
|
||||
}
|
||||
jetchirag marked this conversation as resolved
Outdated
witten
commented
Makes sense to me. You could even do something like this if you wanted to:
In any case, I'd expect to see a unit test covering this new normalization. Makes sense to me. You could even do something like this if you wanted to:
```python
config['hooks']['cronitor'] = {
action_name: config['hooks']['cronitor']
for action_name in ('create', 'prune', 'compact', 'check')
}
```
In any case, I'd expect to see a unit test covering this new normalization.
jetchirag
commented
Added unit test Added unit test
|
||||
|
||||
pagerduty = hooks.get('pagerduty')
|
||||
if isinstance(pagerduty, str):
|
||||
|
|
|
@ -1227,20 +1227,38 @@ properties:
|
|||
service. See borgmatic monitoring documentation for details.
|
||||
cronitor:
|
||||
type: object
|
||||
required: ['ping_url']
|
||||
jetchirag marked this conversation as resolved
Outdated
diivi
commented
Not sure about this, but now that we don't have the
Because even in Not sure about this, but now that we don't have the `required` validation for anything, what will happen when I do this:
```yaml
cronitor:
next_item:
```
Because even in `normalize.py` we only do checks for (cronitor == str and cronitor == dict).
jetchirag
commented
I think that should be covered by default yaml configuration. It should error out cause cronitor is now None. I think that should be covered by default yaml configuration. It should error out cause cronitor is now None.
jetchirag
commented
An error occurred while parsing a configuration file at /XXX/borgmatic/config.yaml: An error occurred while parsing a configuration file at /XXX/borgmatic/config.yaml:
At 'hooks.cronitor': None is not of type 'object'
|
||||
additionalProperties: false
|
||||
properties:
|
||||
create:
|
||||
type: string
|
||||
description: |
|
||||
Cronitor ping URL to notify when a backup
|
||||
begins, ends, or errors.
|
||||
example: https://cronitor.link/d3x0c1
|
||||
prune:
|
||||
type: string
|
||||
description: |
|
||||
Cronitor ping URL to notify when a prune action
|
||||
begins, ends, or errors.
|
||||
example: https://cronitor.link/d3x0c1
|
||||
compact:
|
||||
type: string
|
||||
description: |
|
||||
Cronitor ping URL to notify when a compact action
|
||||
begins, ends, or errors.
|
||||
example: https://cronitor.link/d3x0c1
|
||||
check:
|
||||
type: string
|
||||
description: |
|
||||
Cronitor ping URL to notify when a check action
|
||||
begins, ends, or errors.
|
||||
example: https://cronitor.link/d3x0c1
|
||||
ping_url:
|
||||
type: string
|
||||
description: |
|
||||
Cronitor ping URL to notify when a backup begins,
|
||||
ends, or errors.
|
||||
If this is set, other properties will be ignored
|
||||
diivi
commented
A description should probably still be here (followed by what you added). somethine like A description should probably still be here (followed by what you added). somethine like `Default ping url for cronitor...`
|
||||
and replaced by this value.
|
||||
example: https://cronitor.link/d3x0c1
|
||||
description: |
|
||||
Configuration for a monitoring integration with Cronitor.
|
||||
Create an account at https://cronitor.io if you'd
|
||||
like to use this service. See borgmatic monitoring
|
||||
documentation for details.
|
||||
pagerduty:
|
||||
type: object
|
||||
required: ['integration_key']
|
||||
|
|
|
@ -22,7 +22,7 @@ def initialize_monitor(
|
|||
pass
|
||||
|
||||
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run):
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run, action_name):
|
||||
jetchirag marked this conversation as resolved
Outdated
witten
commented
To support the multiple actions thing mentioned above, a couple of options I can think of:
To support the multiple actions thing mentioned above, a couple of options I can think of:
* Change this to take multiple action names and internally loop accordingly.
* Or, call `ping_monitor()` in a loop over the action names.
jetchirag
commented
I decided to go with calling ping_monitor in a loop. I decided to go with calling ping_monitor in a loop.
witten
commented
Sounds good. Please let me know when this is ready for me to have another look! Sounds good. Please let me know when this is ready for me to have another look!
jetchirag
commented
I think it's ready for a review. Only tests should be missing. I think it's ready for a review. Only tests should be missing.
|
||||
'''
|
||||
Ping the configured Cronhub URL, modified with the monitor.State. Use the given configuration
|
||||
filename in any log entries. If this is a dry run, then don't actually ping anything.
|
||||
|
|
|
@ -22,7 +22,7 @@ def initialize_monitor(
|
|||
pass
|
||||
|
||||
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run):
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run, action_name):
|
||||
'''
|
||||
Ping the configured Cronitor URL, modified with the monitor.State. Use the given configuration
|
||||
filename in any log entries. If this is a dry run, then don't actually ping anything.
|
||||
|
@ -34,7 +34,11 @@ def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_
|
|||
return
|
||||
|
||||
dry_run_label = ' (dry run; not actually pinging)' if dry_run else ''
|
||||
ping_url = '{}/{}'.format(hook_config['ping_url'], MONITOR_STATE_TO_CRONITOR[state])
|
||||
try:
|
||||
ping_url = '{}/{}'.format(hook_config[action_name], MONITOR_STATE_TO_CRONITOR[state])
|
||||
except KeyError:
|
||||
jetchirag marked this conversation as resolved
witten
commented
Nice, succinct way to skip unconfigured actions. You could optionally Nice, succinct way to skip unconfigured actions. You could optionally `logger.debug()` in the `except` so that it's clear what's being skipped and why.
|
||||
print('KeyError')
|
||||
jetchirag marked this conversation as resolved
Outdated
diivi
commented
Missed a debug log here 😄 Missed a debug log here 😄
jetchirag
commented
Thanks! :) Thanks! :)
|
||||
return
|
||||
|
||||
logger.info(
|
||||
'{}: Pinging Cronitor {}{}'.format(config_filename, state.name.lower(), dry_run_label)
|
||||
|
|
|
@ -90,7 +90,7 @@ def initialize_monitor(hook_config, config_filename, monitoring_log_level, dry_r
|
|||
)
|
||||
|
||||
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run):
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run, action_name):
|
||||
'''
|
||||
Ping the configured Healthchecks URL or UUID, modified with the monitor.State. Use the given
|
||||
configuration filename in any log entries, and log to Healthchecks with the giving log level.
|
||||
|
|
|
@ -14,7 +14,7 @@ def initialize_monitor(
|
|||
pass
|
||||
|
||||
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run):
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run, action_name):
|
||||
'''
|
||||
Ping the configured Ntfy topic. Use the given configuration filename in any log entries.
|
||||
If this is a dry run, then don't actually ping anything.
|
||||
|
|
|
@ -21,7 +21,7 @@ def initialize_monitor(
|
|||
pass
|
||||
|
||||
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run):
|
||||
def ping_monitor(hook_config, config_filename, state, monitoring_log_level, dry_run, action_name):
|
||||
'''
|
||||
If this is an error state, create a PagerDuty event with the configured integration key. Use
|
||||
the given configuration filename in any log entries. If this is a dry run, then don't actually
|
||||
|
|
|
@ -15,6 +15,7 @@ def test_ping_monitor_rewrites_ping_url_for_start_state():
|
|||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -30,6 +31,7 @@ def test_ping_monitor_rewrites_ping_url_and_state_for_start_state():
|
|||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -45,6 +47,7 @@ def test_ping_monitor_rewrites_ping_url_for_finish_state():
|
|||
module.monitor.State.FINISH,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -55,7 +58,12 @@ def test_ping_monitor_rewrites_ping_url_for_fail_state():
|
|||
).and_return(flexmock(ok=True))
|
||||
|
||||
module.ping_monitor(
|
||||
hook_config, 'config.yaml', module.monitor.State.FAIL, monitoring_log_level=1, dry_run=False
|
||||
hook_config,
|
||||
'config.yaml',
|
||||
module.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -64,7 +72,12 @@ def test_ping_monitor_dry_run_does_not_hit_ping_url():
|
|||
flexmock(module.requests).should_receive('get').never()
|
||||
|
||||
module.ping_monitor(
|
||||
hook_config, 'config.yaml', module.monitor.State.START, monitoring_log_level=1, dry_run=True
|
||||
hook_config,
|
||||
'config.yaml',
|
||||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=True,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -81,6 +94,7 @@ def test_ping_monitor_with_connection_error_logs_warning():
|
|||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -101,6 +115,7 @@ def test_ping_monitor_with_other_error_logs_warning():
|
|||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -108,5 +123,10 @@ def test_ping_monitor_with_unsupported_monitoring_state():
|
|||
hook_config = {'ping_url': 'https://example.com'}
|
||||
flexmock(module.requests).should_receive('get').never()
|
||||
module.ping_monitor(
|
||||
hook_config, 'config.yaml', module.monitor.State.LOG, monitoring_log_level=1, dry_run=False,
|
||||
hook_config,
|
||||
'config.yaml',
|
||||
module.monitor.State.LOG,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
|
|
@ -4,7 +4,7 @@ from borgmatic.hooks import cronitor as module
|
|||
|
||||
|
||||
def test_ping_monitor_hits_ping_url_for_start_state():
|
||||
hook_config = {'ping_url': 'https://example.com'}
|
||||
hook_config = {'create': 'https://example.com'}
|
||||
flexmock(module.requests).should_receive('get').with_args('https://example.com/run').and_return(
|
||||
flexmock(ok=True)
|
||||
)
|
||||
|
@ -15,11 +15,12 @@ def test_ping_monitor_hits_ping_url_for_start_state():
|
|||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
def test_ping_monitor_hits_ping_url_for_finish_state():
|
||||
hook_config = {'ping_url': 'https://example.com'}
|
||||
hook_config = {'create': 'https://example.com'}
|
||||
flexmock(module.requests).should_receive('get').with_args(
|
||||
'https://example.com/complete'
|
||||
).and_return(flexmock(ok=True))
|
||||
|
@ -30,31 +31,42 @@ def test_ping_monitor_hits_ping_url_for_finish_state():
|
|||
module.monitor.State.FINISH,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
def test_ping_monitor_hits_ping_url_for_fail_state():
|
||||
hook_config = {'ping_url': 'https://example.com'}
|
||||
hook_config = {'create': 'https://example.com'}
|
||||
flexmock(module.requests).should_receive('get').with_args(
|
||||
'https://example.com/fail'
|
||||
).and_return(flexmock(ok=True))
|
||||
|
||||
module.ping_monitor(
|
||||
hook_config, 'config.yaml', module.monitor.State.FAIL, monitoring_log_level=1, dry_run=False
|
||||
hook_config,
|
||||
'config.yaml',
|
||||
module.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
def test_ping_monitor_dry_run_does_not_hit_ping_url():
|
||||
hook_config = {'ping_url': 'https://example.com'}
|
||||
hook_config = {'create': 'https://example.com'}
|
||||
flexmock(module.requests).should_receive('get').never()
|
||||
|
||||
module.ping_monitor(
|
||||
hook_config, 'config.yaml', module.monitor.State.START, monitoring_log_level=1, dry_run=True
|
||||
hook_config,
|
||||
'config.yaml',
|
||||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=True,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
def test_ping_monitor_with_connection_error_logs_warning():
|
||||
hook_config = {'ping_url': 'https://example.com'}
|
||||
hook_config = {'create': 'https://example.com'}
|
||||
flexmock(module.requests).should_receive('get').and_raise(
|
||||
module.requests.exceptions.ConnectionError
|
||||
)
|
||||
|
@ -66,11 +78,12 @@ def test_ping_monitor_with_connection_error_logs_warning():
|
|||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
def test_ping_monitor_with_other_error_logs_warning():
|
||||
hook_config = {'ping_url': 'https://example.com'}
|
||||
hook_config = {'create': 'https://example.com'}
|
||||
response = flexmock(ok=False)
|
||||
response.should_receive('raise_for_status').and_raise(
|
||||
module.requests.exceptions.RequestException
|
||||
|
@ -86,12 +99,18 @@ def test_ping_monitor_with_other_error_logs_warning():
|
|||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
def test_ping_monitor_with_unsupported_monitoring_state():
|
||||
hook_config = {'ping_url': 'https://example.com'}
|
||||
jetchirag marked this conversation as resolved
Outdated
diivi
commented
Maybe one of these tests (or an additional one) can have the Maybe one of these tests (or an additional one) can have the `ping_url` instead of `create` as a key, now that we support it.
jetchirag
commented
Yes, would be working on tests once I complete basic functions. Yes, would be working on tests once I complete basic functions.
diivi
commented
My bad, you can mark the PR as draft on gitea too btw. My bad, you can mark the PR as draft on gitea too btw.
jetchirag
commented
Just found it. Marked! Just found it. Marked!
|
||||
hook_config = {'create': 'https://example.com'}
|
||||
flexmock(module.requests).should_receive('get').never()
|
||||
module.ping_monitor(
|
||||
hook_config, 'config.yaml', module.monitor.State.LOG, monitoring_log_level=1, dry_run=False,
|
||||
hook_config,
|
||||
'config.yaml',
|
||||
module.monitor.State.LOG,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
|
|
@ -147,6 +147,7 @@ def test_ping_monitor_hits_ping_url_for_start_state():
|
|||
state=module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -164,6 +165,7 @@ def test_ping_monitor_hits_ping_url_for_finish_state():
|
|||
state=module.monitor.State.FINISH,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -181,6 +183,7 @@ def test_ping_monitor_hits_ping_url_for_fail_state():
|
|||
state=module.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -198,6 +201,7 @@ def test_ping_monitor_hits_ping_url_for_log_state():
|
|||
state=module.monitor.State.LOG,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -217,6 +221,7 @@ def test_ping_monitor_with_ping_uuid_hits_corresponding_url():
|
|||
state=module.monitor.State.FINISH,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -234,6 +239,7 @@ def test_ping_monitor_skips_ssl_verification_when_verify_tls_false():
|
|||
state=module.monitor.State.FINISH,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -251,6 +257,7 @@ def test_ping_monitor_executes_ssl_verification_when_verify_tls_true():
|
|||
state=module.monitor.State.FINISH,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -265,6 +272,7 @@ def test_ping_monitor_dry_run_does_not_hit_ping_url():
|
|||
state=module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=True,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -279,6 +287,7 @@ def test_ping_monitor_does_not_hit_ping_url_when_states_not_matching():
|
|||
state=module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=True,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -295,6 +304,7 @@ def test_ping_monitor_hits_ping_url_when_states_matching():
|
|||
state=module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -312,6 +322,7 @@ def test_ping_monitor_with_connection_error_logs_warning():
|
|||
state=module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -333,4 +344,5 @@ def test_ping_monitor_with_other_error_logs_warning():
|
|||
state=module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
|
|
@ -48,6 +48,7 @@ def test_ping_monitor_minimal_config_hits_hosted_ntfy_on_fail():
|
|||
borgmatic.hooks.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -69,6 +70,7 @@ def test_ping_monitor_with_auth_hits_hosted_ntfy_on_fail():
|
|||
borgmatic.hooks.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -87,6 +89,7 @@ def test_ping_monitor_auth_with_no_username_warning():
|
|||
borgmatic.hooks.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -105,6 +108,7 @@ def test_ping_monitor_auth_with_no_password_warning():
|
|||
borgmatic.hooks.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -118,6 +122,7 @@ def test_ping_monitor_minimal_config_does_not_hit_hosted_ntfy_on_start():
|
|||
borgmatic.hooks.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -131,6 +136,7 @@ def test_ping_monitor_minimal_config_does_not_hit_hosted_ntfy_on_finish():
|
|||
borgmatic.hooks.monitor.State.FINISH,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -148,6 +154,7 @@ def test_ping_monitor_minimal_config_hits_selfhosted_ntfy_on_fail():
|
|||
borgmatic.hooks.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -161,6 +168,7 @@ def test_ping_monitor_minimal_config_does_not_hit_hosted_ntfy_on_fail_dry_run():
|
|||
borgmatic.hooks.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=True,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -176,6 +184,7 @@ def test_ping_monitor_custom_message_hits_hosted_ntfy_on_fail():
|
|||
borgmatic.hooks.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -193,6 +202,7 @@ def test_ping_monitor_custom_state_hits_hosted_ntfy_on_start():
|
|||
borgmatic.hooks.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -211,6 +221,7 @@ def test_ping_monitor_with_connection_error_logs_warning():
|
|||
borgmatic.hooks.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -233,4 +244,5 @@ def test_ping_monitor_with_other_error_logs_warning():
|
|||
borgmatic.hooks.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
|
|
@ -12,6 +12,7 @@ def test_ping_monitor_ignores_start_state():
|
|||
module.monitor.State.START,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -24,6 +25,7 @@ def test_ping_monitor_ignores_finish_state():
|
|||
module.monitor.State.FINISH,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -36,6 +38,7 @@ def test_ping_monitor_calls_api_for_fail_state():
|
|||
module.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -48,6 +51,7 @@ def test_ping_monitor_dry_run_does_not_call_api():
|
|||
module.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=True,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -63,6 +67,7 @@ def test_ping_monitor_with_connection_error_logs_warning():
|
|||
module.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
||||
|
||||
|
@ -80,4 +85,5 @@ def test_ping_monitor_with_other_error_logs_warning():
|
|||
module.monitor.State.FAIL,
|
||||
monitoring_log_level=1,
|
||||
dry_run=False,
|
||||
action_name='create',
|
||||
)
|
||||
|
|
nit(pythonic):
next(iter(arguments))
Also, if we are only using this variable when
using_primary_action
, it might make sense to rename it toprimary_aciton_name
and/or move it where it's needed too.Thanks, that's definitely more effecient.
If I'm reading this correctly, this will just grab the first action. But there could very well be multiple actions at once, and I'd expect Cronitor to be pinged for each one (if configured for each one).
@witten I unexpectedly missed that multiple actions. Will fix it.
Why
getattr()
? Doesn't the attribute always exist now that you've added it to the global arguments? So would justglobal_arguments.skip_monitoring
work? I don't even think you need theFalse
here becauseaction='store_true'
above defaults to False if not specified.