borgmatic-collective/borgmatic
borgmatic-collective
/
borgmatic
14
58
Fork 42
Code Issues 62 Pull Requests 2 Actions Packages Releases 122 Activity

WIP: Separate endpoint for each primary action for monitor hooks #660

Closed
jetchirag wants to merge 8 commits from jetchirag/borgmatic:action-based-monitor-endpoints into main
pull from: jetchirag/borgmatic:action-based-monitor-endpoints
merge into: borgmatic-collective:main
Conversation 9 Commits 8 Files Changed 15 +17 -4
2 changed files with 17 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 6ce6367a26 - Show all commits

7
borgmatic/commands/arguments.py
View File

@ -145,6 +145,13 @@ def make_parsers():
action='store_true',
help='Go through the motions, but do not actually write to any repositories',
)
global_group.add_argument(
'-nm',
jetchirag marked this conversation as resolved Outdated
witten commented 2023-04-02 22:01:19 +00:00
Outdated
Review
Copy Link

I'd prefer not including short flags for an option like this one. I generally only add short flags for the more frequently used options.

But if it's just too annoying to type --skip-monitoring all the time, you could remove the option entirely and do what borgmatic does for logging: If borgmatic is run interactively, it implicitly disables syslog logging. So it could also disable monitoring when at an interactive console. Up to you! (Although, a related issue: #665.)

I'd prefer not including short flags for an option like this one. I generally only add short flags for the more frequently used options. But if it's just too annoying to type `--skip-monitoring` all the time, you could remove the option entirely and do what borgmatic does for logging: If borgmatic is run interactively, it implicitly disables syslog logging. So it could also disable monitoring when at an interactive console. Up to you! (Although, a related issue: #665.)
jetchirag commented 2023-07-05 23:56:17 +00:00
Outdated
Review
Copy Link

Noted. I would personally prefer the skip-monitoring flag if I am configuring monitoring. I've removed the short flag but let me know if you rather want to go through your alternate suggestion.

Noted. I would personally prefer the skip-monitoring flag if I am configuring monitoring. I've removed the short flag but let me know if you rather want to go through your alternate suggestion.
witten commented 2023-07-05 23:57:35 +00:00
Outdated
Review
Copy Link

Nope, it's fine as-is IMO.

Nope, it's fine as-is IMO.
'--skip-monitoring',
dest='skip_monitoring',
jetchirag marked this conversation as resolved Outdated
witten commented 2023-04-02 22:02:01 +00:00
Outdated
Review
Copy Link

I think you can omit this dest line, because Python will automatically use this name based on the flag name.

I think you can omit this `dest` line, because Python will automatically use this name based on the flag name.
action='store_true',
help='Skip reporting any data to the configured monitoring services',
)
global_group.add_argument(
'-nc', '--no-color', dest='no_color', action='store_true', help='Disable colored output'
)

14
borgmatic/commands/borgmatic.py
View File

@ -67,6 +67,12 @@ def run_configuration(config_filename, config, arguments):
using_primary_action = {'create', 'prune', 'compact', 'check'}.intersection(arguments)
monitoring_log_level = verbosity_to_log_level(global_arguments.monitoring_verbosity)
action_name = next(iter(arguments))
jetchirag marked this conversation as resolved Outdated
diivi commented 2023-03-25 14:00:32 +00:00
Outdated
Review
Copy Link

nit(pythonic): next(iter(arguments))

nit(pythonic): `next(iter(arguments))`
diivi commented 2023-03-25 14:30:38 +00:00
Outdated
Review
Copy Link

Also, if we are only using this variable when using_primary_action, it might make sense to rename it to primary_aciton_name and/or move it where it's needed too.

Also, if we are only using this variable when `using_primary_action`, it might make sense to rename it to `primary_aciton_name` and/or move it where it's needed too.
jetchirag commented 2023-03-25 14:32:02 +00:00
Outdated
Review
Copy Link

Thanks, that's definitely more effecient.

Thanks, that's definitely more effecient.
witten commented 2023-03-26 20:58:16 +00:00
Outdated
Review
Copy Link

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).

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).
jetchirag commented 2023-03-26 21:28:23 +00:00
Outdated
Review
Copy Link

@witten I unexpectedly missed that multiple actions. Will fix it.

@witten I unexpectedly missed that multiple actions. Will fix it.
👍 1
witten commented 2023-04-02 22:04:22 +00:00
Outdated
Review
Copy Link

Why getattr()? Doesn't the attribute always exist now that you've added it to the global arguments? So would just global_arguments.skip_monitoring work? I don't even think you need the False here because action='store_true' above defaults to False if not specified.

Why `getattr()`? Doesn't the attribute always exist now that you've added it to the global arguments? So would just `global_arguments.skip_monitoring` work? I don't even think you need the `False` here because `action='store_true'` above defaults to False if not specified.
skip_monitoring = global_arguments.skip_monitoring
jetchirag marked this conversation as resolved Outdated
witten commented 2023-04-02 22:23:41 +00:00
Outdated
Review
Copy Link

On the isinstance() call: Shouldn't they always be strings? Or is something else sneaking in there?

On the `isinstance()` call: Shouldn't they always be strings? Or is something else sneaking in there?
# action_names = [action for action in arguments.keys() if action != 'global' and isinstance(action, str)]
# print (action_names)
# return
try:
local_borg_version = borg_version.local_borg_version(storage, local_path)
@ -75,7 +81,7 @@ def run_configuration(config_filename, config, arguments):
return
try:
if using_primary_action:
if using_primary_action and not skip_monitoring:
dispatch.call_hooks(
'initialize_monitor',
hooks,
@ -84,7 +90,7 @@ def run_configuration(config_filename, config, arguments):
monitoring_log_level,
global_arguments.dry_run,
)
if using_primary_action:
if using_primary_action and not skip_monitoring:
dispatch.call_hooks(
'ping_monitor',
hooks,
@ -153,7 +159,7 @@ def run_configuration(config_filename, config, arguments):
error_repository = repository_path
try:
if using_primary_action:
if using_primary_action and not skip_monitoring:
# send logs irrespective of error
dispatch.call_hooks(
'ping_monitor',
@ -174,7 +180,7 @@ def run_configuration(config_filename, config, arguments):
if not encountered_error:
try:
if using_primary_action:
if using_primary_action and not skip_monitoring:
dispatch.call_hooks(
'ping_monitor',
hooks,