From 9bf316e28fc47d847f14774cbe866c321b9cb173 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Mon, 21 Apr 2025 19:19:36 -0700 Subject: [PATCH] If the exact same "everything" command hook is present in multiple configuration files, borgmatic only runs it once (#1080). --- NEWS | 2 + borgmatic/commands/borgmatic.py | 60 +++++++++++----- ...reparation-and-cleanup-steps-to-backups.md | 4 ++ tests/unit/commands/test_borgmatic.py | 70 +++++++++++++++---- 4 files changed, 105 insertions(+), 31 deletions(-) diff --git a/NEWS b/NEWS index 9b492b6e..62ae7d9f 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,8 @@ differs from other hooks: https://torsion.org/borgmatic/docs/how-to/add-preparation-and-cleanup-steps-to-backups/ * #1075: Fix an incorrect warning about Borg placeholders being unsupported in a command hook. + * #1080: If the exact same "everything" command hook is present in multiple configuration files, + borgmatic only runs it once. 2.0.3 * #1065: Fix a regression in monitoring hooks in which an error pinged the finish state instead of diff --git a/borgmatic/commands/borgmatic.py b/borgmatic/commands/borgmatic.py index 593a8873..7fadc263 100644 --- a/borgmatic/commands/borgmatic.py +++ b/borgmatic/commands/borgmatic.py @@ -893,17 +893,29 @@ def collect_configuration_run_summary_logs(configs, config_paths, arguments, log return try: + seen_command_hooks = [] + for config_filename, config in configs.items(): - command.execute_hooks( - command.filter_hooks( - config.get('commands'), before='everything', action_names=arguments.keys() + command_hooks = command.filter_hooks( + tuple( + command_hook + for command_hook in config.get('commands', ()) + if command_hook not in seen_command_hooks ), - config.get('umask'), - borgmatic.config.paths.get_working_directory(config), - arguments['global'].dry_run, - configuration_filename=config_filename, - log_file=log_file_path or '', + before='everything', + action_names=arguments.keys(), ) + + if command_hooks: + command.execute_hooks( + command_hooks, + config.get('umask'), + borgmatic.config.paths.get_working_directory(config), + arguments['global'].dry_run, + configuration_filename=config_filename, + log_file=log_file_path or '', + ) + seen_command_hooks += list(command_hooks) except (CalledProcessError, ValueError, OSError) as error: yield from log_error_records('Error running before everything hook', error) return @@ -951,20 +963,30 @@ def collect_configuration_run_summary_logs(configs, config_paths, arguments, log sys.stdout.write(json.dumps(json_results)) try: + seen_command_hooks = [] + for config_filename, config in configs.items(): - command.execute_hooks( - command.filter_hooks( - config.get('commands'), - after='everything', - action_names=arguments.keys(), - state_names=['fail' if encountered_error else 'finish'], + command_hooks = command.filter_hooks( + tuple( + command_hook + for command_hook in config.get('commands', ()) + if command_hook not in seen_command_hooks ), - config.get('umask'), - borgmatic.config.paths.get_working_directory(config), - arguments['global'].dry_run, - configuration_filename=config_filename, - log_file=log_file_path or '', + after='everything', + action_names=arguments.keys(), + state_names=['fail' if encountered_error else 'finish'], ) + + if command_hooks: + command.execute_hooks( + command_hooks, + config.get('umask'), + borgmatic.config.paths.get_working_directory(config), + arguments['global'].dry_run, + configuration_filename=config_filename, + log_file=log_file_path or '', + ) + seen_command_hooks += list(command_hooks) except (CalledProcessError, ValueError, OSError) as error: yield from log_error_records('Error running after everything hook', error) diff --git a/docs/how-to/add-preparation-and-cleanup-steps-to-backups.md b/docs/how-to/add-preparation-and-cleanup-steps-to-backups.md index 8b44123b..2f9c1482 100644 --- a/docs/how-to/add-preparation-and-cleanup-steps-to-backups.md +++ b/docs/how-to/add-preparation-and-cleanup-steps-to-backups.md @@ -91,6 +91,10 @@ Each command in the `commands:` list has the following options: When command hooks run, they respect the `working_directory` option if it is configured, meaning that the hook commands are run in that directory. +New in version 2.0.4If the exact +same `everything` command hook is present in multiple configuration files, +borgmatic only runs it once. + ### Order of execution diff --git a/tests/unit/commands/test_borgmatic.py b/tests/unit/commands/test_borgmatic.py index b2525076..e488a9ba 100644 --- a/tests/unit/commands/test_borgmatic.py +++ b/tests/unit/commands/test_borgmatic.py @@ -1956,14 +1956,17 @@ def test_collect_configuration_run_summary_logs_info_for_success(): def test_collect_configuration_run_summary_executes_hooks_for_create(): + before_everything_hook = {'before': 'everything', 'run': ['echo hi']} + after_everything_hook = {'after': 'everything', 'run': ['echo hi']} + command_hooks = (before_everything_hook, after_everything_hook) flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module.command).should_receive('filter_hooks').with_args( - object, before='everything', action_names=object - ) + command_hooks, before='everything', action_names=object + ).and_return([before_everything_hook]) flexmock(module.command).should_receive('filter_hooks').with_args( - object, after='everything', action_names=object, state_names=['finish'] - ) - flexmock(module.command).should_receive('execute_hooks') + command_hooks, after='everything', action_names=object, state_names=['finish'] + ).and_return([after_everything_hook]) + flexmock(module.command).should_receive('execute_hooks').twice() flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return([]) arguments = { @@ -1973,7 +1976,7 @@ def test_collect_configuration_run_summary_executes_hooks_for_create(): logs = tuple( module.collect_configuration_run_summary_logs( - {'test.yaml': {}}, + {'test.yaml': {'commands': command_hooks}}, config_paths=['/tmp/test.yaml'], arguments=arguments, log_file_path=None, @@ -1983,6 +1986,43 @@ def test_collect_configuration_run_summary_executes_hooks_for_create(): assert {log.levelno for log in logs} == {logging.INFO} +def test_collect_configuration_run_summary_deduplicates_everything_hooks_across_config_files(): + before_everything_hook = {'before': 'everything', 'run': ['echo hi']} + after_everything_hook = {'after': 'everything', 'run': ['echo hi']} + command_hooks = (before_everything_hook, after_everything_hook) + flexmock(module.validate).should_receive('guard_configuration_contains_repository') + flexmock(module.command).should_receive('filter_hooks').with_args( + command_hooks, before='everything', action_names=object + ).and_return([before_everything_hook]).once() + flexmock(module.command).should_receive('filter_hooks').with_args( + (after_everything_hook,), before='everything', action_names=object + ).and_return([]).once() + flexmock(module.command).should_receive('filter_hooks').with_args( + command_hooks, after='everything', action_names=object, state_names=['finish'] + ).and_return([after_everything_hook]).once() + flexmock(module.command).should_receive('filter_hooks').with_args( + (before_everything_hook,), after='everything', action_names=object, state_names=['finish'] + ).and_return([]).once() + flexmock(module.command).should_receive('execute_hooks').twice() + flexmock(module).should_receive('Log_prefix').and_return(flexmock()) + flexmock(module).should_receive('run_configuration').and_return([]) + arguments = { + 'create': flexmock(), + 'global': flexmock(monitoring_verbosity=1, dry_run=False), + } + + logs = tuple( + module.collect_configuration_run_summary_logs( + {'test.yaml': {'commands': command_hooks}, 'other.yaml': {'commands': command_hooks}}, + config_paths=['/tmp/test.yaml', '/tmp/other.yaml'], + arguments=arguments, + log_file_path=None, + ) + ) + + assert {log.levelno for log in logs} == {logging.INFO} + + def test_collect_configuration_run_summary_logs_info_for_success_with_extract(): flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module.command).should_receive('filter_hooks').with_args( @@ -2105,13 +2145,16 @@ def test_collect_configuration_run_summary_logs_missing_configs_error(): def test_collect_configuration_run_summary_logs_before_hook_error(): + before_everything_hook = {'before': 'everything', 'run': ['echo hi']} + after_everything_hook = {'after': 'everything', 'run': ['echo hi']} + command_hooks = (before_everything_hook, after_everything_hook) flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module.command).should_receive('filter_hooks').with_args( object, before='everything', action_names=object - ) + ).and_return([before_everything_hook]) flexmock(module.command).should_receive('filter_hooks').with_args( object, after='everything', action_names=object, state_names=['fail'] - ) + ).and_return([after_everything_hook]) flexmock(module.command).should_receive('execute_hooks').and_raise(ValueError) expected_logs = (flexmock(),) flexmock(module).should_receive('log_error_records').and_return(expected_logs) @@ -2122,7 +2165,7 @@ def test_collect_configuration_run_summary_logs_before_hook_error(): logs = tuple( module.collect_configuration_run_summary_logs( - {'test.yaml': {}}, + {'test.yaml': {'commands': command_hooks}}, config_paths=['/tmp/test.yaml'], arguments=arguments, log_file_path=None, @@ -2133,13 +2176,16 @@ def test_collect_configuration_run_summary_logs_before_hook_error(): def test_collect_configuration_run_summary_logs_after_hook_error(): + before_everything_hook = {'before': 'everything', 'run': ['echo hi']} + after_everything_hook = {'after': 'everything', 'run': ['echo hi']} + command_hooks = (before_everything_hook, after_everything_hook) flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module.command).should_receive('filter_hooks').with_args( object, before='everything', action_names=object - ) + ).and_return([before_everything_hook]) flexmock(module.command).should_receive('filter_hooks').with_args( object, after='everything', action_names=object, state_names=['finish'] - ) + ).and_return([after_everything_hook]) flexmock(module.command).should_receive('execute_hooks').and_return(None).and_raise(ValueError) flexmock(module).should_receive('Log_prefix').and_return(flexmock()) flexmock(module).should_receive('run_configuration').and_return([]) @@ -2152,7 +2198,7 @@ def test_collect_configuration_run_summary_logs_after_hook_error(): logs = tuple( module.collect_configuration_run_summary_logs( - {'test.yaml': {}}, + {'test.yaml': {'commands': command_hooks}}, config_paths=['/tmp/test.yaml'], arguments=arguments, log_file_path=None,