From 9aece3936a7828166479145c0d31455014ff4882 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Mon, 25 Jul 2022 11:30:02 -0700 Subject: [PATCH] Modify "mount" and "extract" actions to require the "--repository" flag when multiple repositories are configured (#566). --- NEWS | 2 ++ borgmatic/commands/borgmatic.py | 28 +++++++++--------- borgmatic/config/validate.py | 41 ++++++++++++++++----------- tests/unit/commands/test_borgmatic.py | 7 +++++ tests/unit/config/test_validate.py | 35 +++++++++++++++++------ 5 files changed, 75 insertions(+), 38 deletions(-) diff --git a/NEWS b/NEWS index e18e130..f7002a3 100644 --- a/NEWS +++ b/NEWS @@ -1,5 +1,7 @@ 1.6.7.dev0 * #565: Fix handling of "repository" and "data" consistency checks to prevent invalid Borg flags. + * #566: Modify "mount" and "extract" actions to require the "--repository" flag when multiple + repositories are configured. * Add support for disabling TLS verification in Healthchecks monitoring hook with "verify_tls" option. diff --git a/borgmatic/commands/borgmatic.py b/borgmatic/commands/borgmatic.py index 1ec250f..2e11c97 100644 --- a/borgmatic/commands/borgmatic.py +++ b/borgmatic/commands/borgmatic.py @@ -768,21 +768,21 @@ def collect_configuration_run_summary_logs(configs, arguments): any, to stdout. ''' # Run cross-file validation checks. - if 'extract' in arguments: - repository = arguments['extract'].repository - elif 'list' in arguments and arguments['list'].archive: - repository = arguments['list'].repository - elif 'mount' in arguments: - repository = arguments['mount'].repository - else: - repository = None + repository = None - if repository: - try: - validate.guard_configuration_contains_repository(repository, configs) - except ValueError as error: - yield from log_error_records(str(error)) - return + for action_name, action_arguments in arguments.items(): + if hasattr(action_arguments, 'repository'): + repository = getattr(action_arguments, 'repository') + break + + try: + if 'extract' in arguments or 'mount' in arguments: + validate.guard_single_repository_selected(repository, configs) + + validate.guard_configuration_contains_repository(repository, configs) + except ValueError as error: + yield from log_error_records(str(error)) + return if not configs: yield from log_error_records( diff --git a/borgmatic/config/validate.py b/borgmatic/config/validate.py index a782d7b..b0e89cd 100644 --- a/borgmatic/config/validate.py +++ b/borgmatic/config/validate.py @@ -140,27 +140,13 @@ def repositories_match(first, second): def guard_configuration_contains_repository(repository, configurations): ''' Given a repository path and a dict mapping from config filename to corresponding parsed config - dict, ensure that the repository is declared exactly once in all of the configurations. - - If no repository is given, then error if there are multiple configured repositories. + dict, ensure that the repository is declared exactly once in all of the configurations. If no + repository is given, skip this check. Raise ValueError if the repository is not found in a configuration, or is declared multiple times. ''' if not repository: - count = len( - tuple( - config_repository - for config in configurations.values() - for config_repository in config['location']['repositories'] - ) - ) - - if count > 1: - raise ValueError( - 'Can\'t determine which repository to use. Use --repository option to disambiguate' - ) - return count = len( @@ -176,3 +162,26 @@ def guard_configuration_contains_repository(repository, configurations): raise ValueError('Repository {} not found in configuration files'.format(repository)) if count > 1: raise ValueError('Repository {} found in multiple configuration files'.format(repository)) + + +def guard_single_repository_selected(repository, configurations): + ''' + Given a repository path and a dict mapping from config filename to corresponding parsed config + dict, ensure either a single repository exists across all configuration files or a repository + path was given. + ''' + if repository: + return + + count = len( + tuple( + config_repository + for config in configurations.values() + for config_repository in config['location']['repositories'] + ) + ) + + if count != 1: + raise ValueError( + 'Can\'t determine which repository to use. Use --repository to disambiguate' + ) diff --git a/tests/unit/commands/test_borgmatic.py b/tests/unit/commands/test_borgmatic.py index bc62d7a..66c5abe 100644 --- a/tests/unit/commands/test_borgmatic.py +++ b/tests/unit/commands/test_borgmatic.py @@ -746,6 +746,7 @@ def test_get_local_path_without_local_path_defaults_to_borg(): def test_collect_configuration_run_summary_logs_info_for_success(): flexmock(module.command).should_receive('execute_hook').never() + flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module).should_receive('run_configuration').and_return([]) arguments = {} @@ -757,6 +758,7 @@ def test_collect_configuration_run_summary_logs_info_for_success(): def test_collect_configuration_run_summary_executes_hooks_for_create(): + flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module).should_receive('run_configuration').and_return([]) arguments = {'create': flexmock(), 'global': flexmock(monitoring_verbosity=1, dry_run=False)} @@ -768,6 +770,7 @@ def test_collect_configuration_run_summary_executes_hooks_for_create(): def test_collect_configuration_run_summary_logs_info_for_success_with_extract(): + flexmock(module.validate).should_receive('guard_single_repository_selected') flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module).should_receive('run_configuration').and_return([]) arguments = {'extract': flexmock(repository='repo')} @@ -795,6 +798,7 @@ def test_collect_configuration_run_summary_logs_extract_with_repository_error(): def test_collect_configuration_run_summary_logs_info_for_success_with_mount(): + flexmock(module.validate).should_receive('guard_single_repository_selected') flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module).should_receive('run_configuration').and_return([]) arguments = {'mount': flexmock(repository='repo')} @@ -846,6 +850,7 @@ def test_collect_configuration_run_summary_logs_pre_hook_error(): def test_collect_configuration_run_summary_logs_post_hook_error(): flexmock(module.command).should_receive('execute_hook').and_return(None).and_raise(ValueError) + flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module).should_receive('run_configuration').and_return([]) expected_logs = (flexmock(),) flexmock(module).should_receive('log_error_records').and_return(expected_logs) @@ -874,6 +879,7 @@ def test_collect_configuration_run_summary_logs_for_list_with_archive_and_reposi def test_collect_configuration_run_summary_logs_info_for_success_with_list(): + flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module).should_receive('run_configuration').and_return([]) arguments = {'list': flexmock(repository='repo', archive=None)} @@ -916,6 +922,7 @@ def test_collect_configuration_run_summary_logs_run_umount_error(): def test_collect_configuration_run_summary_logs_outputs_merged_json_results(): + flexmock(module.validate).should_receive('guard_configuration_contains_repository') flexmock(module).should_receive('run_configuration').and_return(['foo', 'bar']).and_return( ['baz'] ) diff --git a/tests/unit/config/test_validate.py b/tests/unit/config/test_validate.py index a858899..713ecc7 100644 --- a/tests/unit/config/test_validate.py +++ b/tests/unit/config/test_validate.py @@ -120,14 +120,6 @@ def test_guard_configuration_contains_repository_does_not_raise_when_repository_ ) -def test_guard_configuration_contains_repository_errors_when_repository_assumed_to_match_config_twice(): - with pytest.raises(ValueError): - module.guard_configuration_contains_repository( - repository=None, - configurations={'config.yaml': {'location': {'repositories': ['repo', 'repo2']}}}, - ) - - def test_guard_configuration_contains_repository_errors_when_repository_missing_from_config(): flexmock(module).should_receive('repositories_match').replace_with( lambda first, second: first == second @@ -153,3 +145,30 @@ def test_guard_configuration_contains_repository_errors_when_repository_matches_ 'other.yaml': {'location': {'repositories': ['repo']}}, }, ) + + +def test_guard_single_repository_selected_raises_when_multiple_repositories_configured_and_none_selected(): + with pytest.raises(ValueError): + module.guard_single_repository_selected( + repository=None, + configurations={'config.yaml': {'location': {'repositories': ['repo', 'repo2']}}}, + ) + + +def test_guard_single_repository_selected_does_not_raise_when_single_repository_configured_and_none_selected(): + module.guard_single_repository_selected( + repository=None, configurations={'config.yaml': {'location': {'repositories': ['repo']}}}, + ) + + +def test_guard_single_repository_selected_does_not_raise_when_no_repositories_configured_and_one_selected(): + module.guard_single_repository_selected( + repository='repo', configurations={'config.yaml': {'location': {'repositories': []}}}, + ) + + +def test_guard_single_repository_selected_does_not_raise_when_repositories_configured_and_one_selected(): + module.guard_single_repository_selected( + repository='repo', + configurations={'config.yaml': {'location': {'repositories': ['repo', 'repo2']}}}, + )