From da789294157d77ca0e7f1961bc01e76dd9bf43a9 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 13 Jul 2023 19:25:14 -0700 Subject: [PATCH] To prevent argument parsing errors on ambiguous commands, drop support for multiple consecutive flag values. --- NEWS | 3 + borgmatic/commands/arguments.py | 74 ++++++-------------- borgmatic/config/normalize.py | 2 +- docs/how-to/backup-your-databases.md | 2 +- docs/how-to/extract-a-backup.md | 2 +- docs/how-to/make-per-application-backups.md | 6 -- tests/end-to-end/test_borgmatic.py | 7 -- tests/integration/commands/test_arguments.py | 17 ++--- tests/unit/hooks/test_mysql.py | 23 ++++++ tests/unit/hooks/test_postgresql.py | 24 +++++++ 10 files changed, 79 insertions(+), 81 deletions(-) diff --git a/NEWS b/NEWS index 23f4737..dd882d9 100644 --- a/NEWS +++ b/NEWS @@ -17,6 +17,9 @@ values (unless one is not set). * #721: BREAKING: The storage umask and the hooks umask can no longer have different values (unless one is not set). + * BREAKING: Flags like "--config" that previously took multiple values now need to be given once + per value, e.g. "--config first.yaml --config second.yaml" instead of "--config first.yaml + second.yaml". This prevents argument parsing errors on ambiguous commands. * BREAKING: Remove the deprecated (and silently ignored) "--successful" flag on the "list" action, as newer versions of Borg list successful (non-checkpoint) archives by default. * All deprecated configuration option values now generate warning logs. diff --git a/borgmatic/commands/arguments.py b/borgmatic/commands/arguments.py index ce246e1..0b4b259 100644 --- a/borgmatic/commands/arguments.py +++ b/borgmatic/commands/arguments.py @@ -1,7 +1,7 @@ import collections import itertools import sys -from argparse import Action, ArgumentParser +from argparse import ArgumentParser from borgmatic.config import collect @@ -216,42 +216,12 @@ def parse_arguments_for_actions(unparsed_arguments, action_parsers, global_parse arguments['global'], remaining = global_parser.parse_known_args(unparsed_arguments) remaining_action_arguments.append(remaining) - # Prevent action names and arguments that follow "--config" paths from being considered as - # additional paths. - for argument_name in arguments.keys(): - if argument_name == 'global': - continue - - for action_name in [argument_name] + ACTION_ALIASES.get(argument_name, []): - try: - action_name_index = arguments['global'].config_paths.index(action_name) - arguments['global'].config_paths = arguments['global'].config_paths[ - :action_name_index - ] - break - except ValueError: - pass - return ( arguments, tuple(remaining_action_arguments) if arguments else unparsed_arguments, ) -class Extend_action(Action): - ''' - An argparse action to support Python 3.8's "extend" action in older versions of Python. - ''' - - def __call__(self, parser, namespace, values, option_string=None): - items = getattr(namespace, self.dest, None) - - if items: - items.extend(values) # pragma: no cover - else: - setattr(namespace, self.dest, list(values)) - - def make_parsers(): ''' Build a global arguments parser, individual action parsers, and a combined parser containing @@ -263,16 +233,14 @@ def make_parsers(): unexpanded_config_paths = collect.get_default_config_paths(expand_home=False) global_parser = ArgumentParser(add_help=False) - global_parser.register('action', 'extend', Extend_action) global_group = global_parser.add_argument_group('global arguments') global_group.add_argument( '-c', '--config', - nargs='*', dest='config_paths', - default=config_paths, - help=f"Configuration filenames or directories, defaults to: {' '.join(unexpanded_config_paths)}", + action='append', + help=f"Configuration filename or directory, can specify flag multiple times, defaults to: {' '.join(unexpanded_config_paths)}", ) global_group.add_argument( '-n', @@ -331,10 +299,9 @@ def make_parsers(): global_group.add_argument( '--override', metavar='OPTION.SUBOPTION=VALUE', - nargs='+', dest='overrides', - action='extend', - help='One or more configuration file options to override with specified values', + action='append', + help='Configuration file option to override with specified value, can specify flag multiple times', ) global_group.add_argument( '--no-environment-interpolation', @@ -672,9 +639,9 @@ def make_parsers(): '--path', '--restore-path', metavar='PATH', - nargs='+', dest='paths', - help='Paths to extract from archive, defaults to the entire archive', + action='append', + help='Path to extract from archive, can specify flag multiple times, defaults to the entire archive', ) extract_group.add_argument( '--destination', @@ -826,9 +793,9 @@ def make_parsers(): export_tar_group.add_argument( '--path', metavar='PATH', - nargs='+', dest='paths', - help='Paths to export from archive, defaults to the entire archive', + action='append', + help='Path to export from archive, can specify flag multiple times, defaults to the entire archive', ) export_tar_group.add_argument( '--destination', @@ -877,9 +844,9 @@ def make_parsers(): mount_group.add_argument( '--path', metavar='PATH', - nargs='+', dest='paths', - help='Paths to mount from archive, defaults to the entire archive', + action='append', + help='Path to mount from archive, can specify multiple times, defaults to the entire archive', ) mount_group.add_argument( '--foreground', @@ -954,16 +921,16 @@ def make_parsers(): restore_group.add_argument( '--database', metavar='NAME', - nargs='+', dest='databases', - help="Names of databases to restore from archive, defaults to all databases. Note that any databases to restore must be defined in borgmatic's configuration", + action='append', + help="Name of database to restore from archive, must be defined in borgmatic's configuration, can specify flag multiple times, defaults to all databases", ) restore_group.add_argument( '--schema', metavar='NAME', - nargs='+', dest='schemas', - help='Names of schemas to restore from the database, defaults to all schemas. Schemas are only supported for PostgreSQL and MongoDB databases', + action='append', + help='Name of schema to restore from the database, can specify flag multiple times, defaults to all schemas. Schemas are only supported for PostgreSQL and MongoDB databases', ) restore_group.add_argument( '--hostname', @@ -1065,16 +1032,16 @@ def make_parsers(): list_group.add_argument( '--path', metavar='PATH', - nargs='+', dest='paths', - help='Paths or patterns to list from a single selected archive (via "--archive"), defaults to listing the entire archive', + action='append', + help='Path or pattern to list from a single selected archive (via "--archive"), can specify flag multiple times, defaults to listing the entire archive', ) list_group.add_argument( '--find', metavar='PATH', - nargs='+', dest='find_paths', - help='Partial paths or patterns to search for and list across multiple archives', + action='append', + help='Partial path or pattern to search for and list across multiple archives, can specify flag multiple times', ) list_group.add_argument( '--short', default=False, action='store_true', help='Output only path names' @@ -1248,6 +1215,9 @@ def parse_arguments(*unparsed_arguments): unparsed_arguments, action_parsers.choices, global_parser ) + if not arguments['global'].config_paths: + arguments['global'].config_paths = collect.get_default_config_paths(expand_home=True) + for action_name in ('bootstrap', 'generate', 'validate'): if ( action_name in arguments.keys() and len(arguments.keys()) > 2 diff --git a/borgmatic/config/normalize.py b/borgmatic/config/normalize.py index 83f2a20..7ca7913 100644 --- a/borgmatic/config/normalize.py +++ b/borgmatic/config/normalize.py @@ -46,7 +46,7 @@ def normalize_sections(config_filename, config): dict( levelno=logging.WARNING, levelname='WARNING', - msg=f'{config_filename}: Configuration sections like location: and storage: are deprecated and support will be removed from a future release. Move all of your options out of sections to the global scope.', + msg=f'{config_filename}: Configuration sections like location: and storage: are deprecated and support will be removed from a future release. To prepare for this, move your options out of sections to the global scope.', ) ) ] diff --git a/docs/how-to/backup-your-databases.md b/docs/how-to/backup-your-databases.md index 4708e0a..f1f0087 100644 --- a/docs/how-to/backup-your-databases.md +++ b/docs/how-to/backup-your-databases.md @@ -295,7 +295,7 @@ restore one of them, use the `--database` flag to select one or more databases. For instance: ```bash -borgmatic restore --archive host-2023-... --database users +borgmatic restore --archive host-2023-... --database users --database orders ``` New in version 1.7.6 You can diff --git a/docs/how-to/extract-a-backup.md b/docs/how-to/extract-a-backup.md index 164fc13..5a4aa6f 100644 --- a/docs/how-to/extract-a-backup.md +++ b/docs/how-to/extract-a-backup.md @@ -65,7 +65,7 @@ everything from an archive. To do that, tack on one or more `--path` values. For instance: ```bash -borgmatic extract --archive latest --path path/1 path/2 +borgmatic extract --archive latest --path path/1 --path path/2 ``` Note that the specified restore paths should not have a leading slash. Like a diff --git a/docs/how-to/make-per-application-backups.md b/docs/how-to/make-per-application-backups.md index 62a1c3e..60e0b22 100644 --- a/docs/how-to/make-per-application-backups.md +++ b/docs/how-to/make-per-application-backups.md @@ -448,12 +448,6 @@ the configured value for the `remote_path` option, and use the value of You can even override nested values or multiple values at once. For instance: -```bash -borgmatic create --override parent_option.option1=value1 parent_option.option2=value2 -``` - -This will accomplish the same thing: - ```bash borgmatic create --override parent_option.option1=value1 --override parent_option.option2=value2 ``` diff --git a/tests/end-to-end/test_borgmatic.py b/tests/end-to-end/test_borgmatic.py index d4d8629..7eac73a 100644 --- a/tests/end-to-end/test_borgmatic.py +++ b/tests/end-to-end/test_borgmatic.py @@ -74,13 +74,6 @@ def test_borgmatic_command(): assert len(parsed_output) == 1 assert 'repository' in parsed_output[0] - - # Exercise the bootstrap action. - output = subprocess.check_output( - f'borgmatic --config {config_path} bootstrap --repository {repository_path}'.split(' '), - ).decode(sys.stdout.encoding) - - assert 'successful' in output finally: os.chdir(original_working_directory) shutil.rmtree(temporary_directory) diff --git a/tests/integration/commands/test_arguments.py b/tests/integration/commands/test_arguments.py index db8db63..74d056a 100644 --- a/tests/integration/commands/test_arguments.py +++ b/tests/integration/commands/test_arguments.py @@ -17,10 +17,10 @@ def test_parse_arguments_with_no_arguments_uses_defaults(): assert global_arguments.log_file_verbosity == 0 -def test_parse_arguments_with_multiple_config_paths_parses_as_list(): +def test_parse_arguments_with_multiple_config_flags_parses_as_list(): flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default']) - arguments = module.parse_arguments('--config', 'myconfig', 'otherconfig') + arguments = module.parse_arguments('--config', 'myconfig', '--config', 'otherconfig') global_arguments = arguments['global'] assert global_arguments.config_paths == ['myconfig', 'otherconfig'] @@ -109,20 +109,11 @@ def test_parse_arguments_with_single_override_parses(): assert global_arguments.overrides == ['foo.bar=baz'] -def test_parse_arguments_with_multiple_overrides_parses(): - flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default']) - - arguments = module.parse_arguments('--override', 'foo.bar=baz', 'foo.quux=7') - - global_arguments = arguments['global'] - assert global_arguments.overrides == ['foo.bar=baz', 'foo.quux=7'] - - -def test_parse_arguments_with_multiple_overrides_and_flags_parses(): +def test_parse_arguments_with_multiple_overrides_flags_parses(): flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default']) arguments = module.parse_arguments( - '--override', 'foo.bar=baz', '--override', 'foo.quux=7', 'this.that=8' + '--override', 'foo.bar=baz', '--override', 'foo.quux=7', '--override', 'this.that=8' ) global_arguments = arguments['global'] diff --git a/tests/unit/hooks/test_mysql.py b/tests/unit/hooks/test_mysql.py index 19ab3cc..55abebb 100644 --- a/tests/unit/hooks/test_mysql.py +++ b/tests/unit/hooks/test_mysql.py @@ -407,6 +407,29 @@ def test_restore_database_dump_runs_mysql_to_restore(): ) +def test_restore_database_dump_errors_when_database_missing_from_configuration(): + databases_config = [{'name': 'foo'}, {'name': 'bar'}] + extract_process = flexmock(stdout=flexmock()) + + flexmock(module).should_receive('execute_command_with_processes').never() + + with pytest.raises(ValueError): + module.restore_database_dump( + databases_config, + {}, + 'test.yaml', + database_name='other', + dry_run=False, + extract_process=extract_process, + connection_params={ + 'hostname': None, + 'port': None, + 'username': None, + 'password': None, + }, + ) + + def test_restore_database_dump_runs_mysql_with_options(): databases_config = [{'name': 'foo', 'restore_options': '--harder'}] extract_process = flexmock(stdout=flexmock()) diff --git a/tests/unit/hooks/test_postgresql.py b/tests/unit/hooks/test_postgresql.py index 51235ff..73d67da 100644 --- a/tests/unit/hooks/test_postgresql.py +++ b/tests/unit/hooks/test_postgresql.py @@ -515,6 +515,30 @@ def test_restore_database_dump_runs_pg_restore(): ) +def test_restore_database_dump_errors_when_database_missing_from_configuration(): + databases_config = [{'name': 'foo', 'schemas': None}, {'name': 'bar'}] + extract_process = flexmock(stdout=flexmock()) + + flexmock(module).should_receive('execute_command_with_processes').never() + flexmock(module).should_receive('execute_command').never() + + with pytest.raises(ValueError): + module.restore_database_dump( + databases_config, + {}, + 'test.yaml', + database_name='other', + dry_run=False, + extract_process=extract_process, + connection_params={ + 'hostname': None, + 'port': None, + 'username': None, + 'password': None, + }, + ) + + def test_restore_database_dump_runs_pg_restore_with_hostname_and_port(): databases_config = [ {'name': 'foo', 'hostname': 'database.example.org', 'port': 5433, 'schemas': None}