To prevent argument parsing errors on ambiguous commands, drop support for multiple consecutive flag values.

This commit is contained in:
Dan Helfman 2023-07-13 19:25:14 -07:00
parent 18b3b569d0
commit da78929415
10 changed files with 79 additions and 81 deletions

3
NEWS
View file

@ -17,6 +17,9 @@
values (unless one is not set). values (unless one is not set).
* #721: BREAKING: The storage umask and the hooks umask can no longer have different values (unless * #721: BREAKING: The storage umask and the hooks umask can no longer have different values (unless
one is not set). 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, * 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. as newer versions of Borg list successful (non-checkpoint) archives by default.
* All deprecated configuration option values now generate warning logs. * All deprecated configuration option values now generate warning logs.

View file

@ -1,7 +1,7 @@
import collections import collections
import itertools import itertools
import sys import sys
from argparse import Action, ArgumentParser from argparse import ArgumentParser
from borgmatic.config import collect 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) arguments['global'], remaining = global_parser.parse_known_args(unparsed_arguments)
remaining_action_arguments.append(remaining) 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 ( return (
arguments, arguments,
tuple(remaining_action_arguments) if arguments else unparsed_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(): def make_parsers():
''' '''
Build a global arguments parser, individual action parsers, and a combined parser containing 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) unexpanded_config_paths = collect.get_default_config_paths(expand_home=False)
global_parser = ArgumentParser(add_help=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 = global_parser.add_argument_group('global arguments')
global_group.add_argument( global_group.add_argument(
'-c', '-c',
'--config', '--config',
nargs='*',
dest='config_paths', dest='config_paths',
default=config_paths, action='append',
help=f"Configuration filenames or directories, defaults to: {' '.join(unexpanded_config_paths)}", help=f"Configuration filename or directory, can specify flag multiple times, defaults to: {' '.join(unexpanded_config_paths)}",
) )
global_group.add_argument( global_group.add_argument(
'-n', '-n',
@ -331,10 +299,9 @@ def make_parsers():
global_group.add_argument( global_group.add_argument(
'--override', '--override',
metavar='OPTION.SUBOPTION=VALUE', metavar='OPTION.SUBOPTION=VALUE',
nargs='+',
dest='overrides', dest='overrides',
action='extend', action='append',
help='One or more configuration file options to override with specified values', help='Configuration file option to override with specified value, can specify flag multiple times',
) )
global_group.add_argument( global_group.add_argument(
'--no-environment-interpolation', '--no-environment-interpolation',
@ -672,9 +639,9 @@ def make_parsers():
'--path', '--path',
'--restore-path', '--restore-path',
metavar='PATH', metavar='PATH',
nargs='+',
dest='paths', 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( extract_group.add_argument(
'--destination', '--destination',
@ -826,9 +793,9 @@ def make_parsers():
export_tar_group.add_argument( export_tar_group.add_argument(
'--path', '--path',
metavar='PATH', metavar='PATH',
nargs='+',
dest='paths', 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( export_tar_group.add_argument(
'--destination', '--destination',
@ -877,9 +844,9 @@ def make_parsers():
mount_group.add_argument( mount_group.add_argument(
'--path', '--path',
metavar='PATH', metavar='PATH',
nargs='+',
dest='paths', 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( mount_group.add_argument(
'--foreground', '--foreground',
@ -954,16 +921,16 @@ def make_parsers():
restore_group.add_argument( restore_group.add_argument(
'--database', '--database',
metavar='NAME', metavar='NAME',
nargs='+',
dest='databases', 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( restore_group.add_argument(
'--schema', '--schema',
metavar='NAME', metavar='NAME',
nargs='+',
dest='schemas', 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( restore_group.add_argument(
'--hostname', '--hostname',
@ -1065,16 +1032,16 @@ def make_parsers():
list_group.add_argument( list_group.add_argument(
'--path', '--path',
metavar='PATH', metavar='PATH',
nargs='+',
dest='paths', 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( list_group.add_argument(
'--find', '--find',
metavar='PATH', metavar='PATH',
nargs='+',
dest='find_paths', 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( list_group.add_argument(
'--short', default=False, action='store_true', help='Output only path names' '--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 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'): for action_name in ('bootstrap', 'generate', 'validate'):
if ( if (
action_name in arguments.keys() and len(arguments.keys()) > 2 action_name in arguments.keys() and len(arguments.keys()) > 2

View file

@ -46,7 +46,7 @@ def normalize_sections(config_filename, config):
dict( dict(
levelno=logging.WARNING, levelno=logging.WARNING,
levelname='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.',
) )
) )
] ]

View file

@ -295,7 +295,7 @@ restore one of them, use the `--database` flag to select one or more
databases. For instance: databases. For instance:
```bash ```bash
borgmatic restore --archive host-2023-... --database users borgmatic restore --archive host-2023-... --database users --database orders
``` ```
<span class="minilink minilink-addedin">New in version 1.7.6</span> You can <span class="minilink minilink-addedin">New in version 1.7.6</span> You can

View file

@ -65,7 +65,7 @@ everything from an archive. To do that, tack on one or more `--path` values.
For instance: For instance:
```bash ```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 Note that the specified restore paths should not have a leading slash. Like a

View file

@ -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: 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 ```bash
borgmatic create --override parent_option.option1=value1 --override parent_option.option2=value2 borgmatic create --override parent_option.option1=value1 --override parent_option.option2=value2
``` ```

View file

@ -74,13 +74,6 @@ def test_borgmatic_command():
assert len(parsed_output) == 1 assert len(parsed_output) == 1
assert 'repository' in parsed_output[0] 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: finally:
os.chdir(original_working_directory) os.chdir(original_working_directory)
shutil.rmtree(temporary_directory) shutil.rmtree(temporary_directory)

View file

@ -17,10 +17,10 @@ def test_parse_arguments_with_no_arguments_uses_defaults():
assert global_arguments.log_file_verbosity == 0 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']) 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'] global_arguments = arguments['global']
assert global_arguments.config_paths == ['myconfig', 'otherconfig'] 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'] assert global_arguments.overrides == ['foo.bar=baz']
def test_parse_arguments_with_multiple_overrides_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', '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():
flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default']) flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default'])
arguments = module.parse_arguments( 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'] global_arguments = arguments['global']

View file

@ -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(): def test_restore_database_dump_runs_mysql_with_options():
databases_config = [{'name': 'foo', 'restore_options': '--harder'}] databases_config = [{'name': 'foo', 'restore_options': '--harder'}]
extract_process = flexmock(stdout=flexmock()) extract_process = flexmock(stdout=flexmock())

View file

@ -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(): def test_restore_database_dump_runs_pg_restore_with_hostname_and_port():
databases_config = [ databases_config = [
{'name': 'foo', 'hostname': 'database.example.org', 'port': 5433, 'schemas': None} {'name': 'foo', 'hostname': 'database.example.org', 'port': 5433, 'schemas': None}