From e4e455ee457f2429b52287144618e1879bc3b234 Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Fri, 23 Jun 2023 10:11:41 -0700 Subject: [PATCH] Deprecate validate-borgmatic-config in favor of new "config validate" action (#529). --- NEWS | 3 +- borgmatic/actions/config/generate.py | 7 ++ borgmatic/actions/config/validate.py | 22 ++++++ borgmatic/commands/arguments.py | 21 +++++- borgmatic/commands/borgmatic.py | 72 +++++++++++++++--- borgmatic/commands/validate_config.py | 71 +++--------------- docs/Dockerfile | 2 +- docs/how-to/make-per-application-backups.md | 11 ++- docs/how-to/set-up-backups.md | 7 ++ tests/integration/actions/__init__.py | 0 tests/integration/actions/config/__init__.py | 0 .../actions/config/test_validate.py | 37 +++++++++ .../commands/test_validate_config.py | 26 +------ tests/unit/actions/config/test_generate.py | 6 +- tests/unit/actions/config/test_validate.py | 17 +++++ tests/unit/commands/test_borgmatic.py | 75 +++++++++++++++++-- 16 files changed, 266 insertions(+), 111 deletions(-) create mode 100644 borgmatic/actions/config/validate.py create mode 100644 tests/integration/actions/__init__.py create mode 100644 tests/integration/actions/config/__init__.py create mode 100644 tests/integration/actions/config/test_validate.py create mode 100644 tests/unit/actions/config/test_validate.py diff --git a/NEWS b/NEWS index 7e78c131..781e30dd 100644 --- a/NEWS +++ b/NEWS @@ -4,7 +4,8 @@ * #399: Add a documentation troubleshooting note for MySQL/MariaDB authentication errors. * #529: Remove upgrade-borgmatic-config command for upgrading borgmatic 1.1.0 INI-style configuration. - * #529: Deprecate generate-borgmatic-config in favor if new "config generate" action. + * #529: Deprecate generate-borgmatic-config in favor of new "config generate" action. + * #529: Deprecate validate-borgmatic-config in favor of new "config validate" action. * #697, #712: Extract borgmatic configuration from backup via new "config bootstrap" action—even when borgmatic has no configuration yet! * #669: Add sample systemd user service for running borgmatic as a non-root user. diff --git a/borgmatic/actions/config/generate.py b/borgmatic/actions/config/generate.py index 5f430383..1943ea74 100644 --- a/borgmatic/actions/config/generate.py +++ b/borgmatic/actions/config/generate.py @@ -7,6 +7,13 @@ logger = logging.getLogger(__name__) def run_generate(generate_arguments, global_arguments): + ''' + Given the generate arguments and the global arguments, each as an argparse.Namespace instance, + run the "generate" action. + + Raise FileExistsError if a file already exists at the destination path and the generate + arguments do not have overwrite set. + ''' dry_run_label = ' (dry run; not actually writing anything)' if global_arguments.dry_run else '' logger.answer( diff --git a/borgmatic/actions/config/validate.py b/borgmatic/actions/config/validate.py new file mode 100644 index 00000000..2cec6135 --- /dev/null +++ b/borgmatic/actions/config/validate.py @@ -0,0 +1,22 @@ +import logging + +import borgmatic.config.generate + +logger = logging.getLogger(__name__) + + +def run_validate(validate_arguments, configs): + ''' + Given the validate arguments as an argparse.Namespace instance and a dict of configuration + filename to corresponding parsed configuration, run the "validate" action. + + Most of the validation is actually performed implicitly by the standard borgmatic configuration + loading machinery prior to here, so this function mainly exists to support additional validate + flags like "--show". + ''' + if validate_arguments.show: + for config_path, config in configs.items(): + if len(configs) > 1: + logger.answer('---') + + logger.answer(borgmatic.config.generate.render_configuration(config)) diff --git a/borgmatic/commands/arguments.py b/borgmatic/commands/arguments.py index ae6056a2..fe8c1dd1 100644 --- a/borgmatic/commands/arguments.py +++ b/borgmatic/commands/arguments.py @@ -699,8 +699,8 @@ def make_parsers(): config_bootstrap_parser = config_parsers.add_parser( 'bootstrap', - help='Extract the borgmatic config files from a named archive', - description='Extract the borgmatic config files from a named archive', + help='Extract the borgmatic configuration files from a named archive', + description='Extract the borgmatic configuration files from a named archive', add_help=False, ) config_bootstrap_group = config_bootstrap_parser.add_argument_group( @@ -774,6 +774,23 @@ def make_parsers(): '-h', '--help', action='help', help='Show this help message and exit' ) + config_validate_parser = config_parsers.add_parser( + 'validate', + help='Validate that borgmatic configuration files specified with --config (see borgmatic --help)', + description='Validate borgmatic configuration files specified with --config (see borgmatic --help)', + add_help=False, + ) + config_validate_group = config_validate_parser.add_argument_group('config validate arguments') + config_validate_group.add_argument( + '-s', + '--show', + action='store_true', + help='Show the validated configuration after all include merging has occurred', + ) + config_validate_group.add_argument( + '-h', '--help', action='help', help='Show this help message and exit' + ) + export_tar_parser = action_parsers.add_parser( 'export-tar', aliases=ACTION_ALIASES['export-tar'], diff --git a/borgmatic/commands/borgmatic.py b/borgmatic/commands/borgmatic.py index e04a785a..7d31ddee 100644 --- a/borgmatic/commands/borgmatic.py +++ b/borgmatic/commands/borgmatic.py @@ -20,6 +20,7 @@ import borgmatic.actions.check import borgmatic.actions.compact import borgmatic.actions.config.bootstrap import borgmatic.actions.config.generate +import borgmatic.actions.config.validate import borgmatic.actions.create import borgmatic.actions.export_tar import borgmatic.actions.extract @@ -500,6 +501,9 @@ def load_configurations(config_filenames, overrides=None, resolve_env=True): Given a sequence of configuration filenames, load and validate each configuration file. Return the results as a tuple of: dict of configuration filename to corresponding parsed configuration, and sequence of logging.LogRecord instances containing any parse errors. + + Log records are returned here instead of being logged directly because logging isn't yet + initialized at this point! ''' # Dict mapping from config filename to corresponding parsed config dict. configs = collections.OrderedDict() @@ -507,6 +511,17 @@ def load_configurations(config_filenames, overrides=None, resolve_env=True): # Parse and load each configuration file. for config_filename in config_filenames: + logs.extend( + [ + logging.makeLogRecord( + dict( + levelno=logging.DEBUG, + levelname='DEBUG', + msg=f'{config_filename}: Loading configuration file', + ) + ), + ] + ) try: configs[config_filename], parse_logs = validate.parse_configuration( config_filename, validate.schema_filename(), overrides, resolve_env @@ -603,12 +618,13 @@ def get_local_path(configs): return next(iter(configs.values())).get('location', {}).get('local_path', 'borg') -def collect_highlander_action_summary_logs(configs, arguments): +def collect_highlander_action_summary_logs(configs, arguments, configuration_parse_errors): ''' - Given a dict of configuration filename to corresponding parsed configuration and parsed - command-line arguments as a dict from subparser name to a parsed namespace of arguments, run - a highlander action specified in the arguments, if any, and yield a series of logging.LogRecord - instances containing summary information. + Given a dict of configuration filename to corresponding parsed configuration, parsed + command-line arguments as a dict from subparser name to a parsed namespace of arguments, and + whether any configuration files encountered errors during parsing, run a highlander action + specified in the arguments, if any, and yield a series of logging.LogRecord instances containing + summary information. A highlander action is an action that cannot coexist with other actions on the borgmatic command-line, and borgmatic exits after processing such an action. @@ -662,6 +678,37 @@ def collect_highlander_action_summary_logs(configs, arguments): return + if 'validate' in arguments: + if configuration_parse_errors: + yield logging.makeLogRecord( + dict( + levelno=logging.CRITICAL, + levelname='CRITICAL', + msg='Configuration validation failed', + ) + ) + + return + + try: + borgmatic.actions.config.validate.run_validate(arguments['validate'], configs) + + yield logging.makeLogRecord( + dict( + levelno=logging.ANSWER, + levelname='ANSWER', + msg='All configuration files are valid', + ) + ) + except ( + CalledProcessError, + ValueError, + OSError, + ) as error: + yield from log_error_records(error) + + return + def collect_configuration_run_summary_logs(configs, arguments): ''' @@ -800,6 +847,9 @@ def main(extra_summary_logs=[]): # pragma: no cover configs, parse_logs = load_configurations( config_filenames, global_arguments.overrides, global_arguments.resolve_env ) + configuration_parse_errors = ( + (max(log.levelno for log in parse_logs) >= logging.CRITICAL) if parse_logs else False + ) any_json_flags = any( getattr(sub_arguments, 'json', False) for sub_arguments in arguments.values() @@ -822,15 +872,17 @@ def main(extra_summary_logs=[]): # pragma: no cover logger.critical(f'Error configuring logging: {error}') exit_with_help_link() - logger.debug('Ensuring legacy configuration is upgraded') - summary_logs = ( - parse_logs + extra_summary_logs + + parse_logs + ( - list(collect_highlander_action_summary_logs(configs, arguments)) + list( + collect_highlander_action_summary_logs( + configs, arguments, configuration_parse_errors + ) + ) or list(collect_configuration_run_summary_logs(configs, arguments)) ) - + extra_summary_logs ) summary_logs_max_level = max(log.levelno for log in summary_logs) diff --git a/borgmatic/commands/validate_config.py b/borgmatic/commands/validate_config.py index 8aa8d321..0b3dd1ff 100644 --- a/borgmatic/commands/validate_config.py +++ b/borgmatic/commands/validate_config.py @@ -1,68 +1,17 @@ import logging import sys -from argparse import ArgumentParser -import borgmatic.config.generate -from borgmatic.config import collect, validate - -logger = logging.getLogger(__name__) +import borgmatic.commands.borgmatic -def parse_arguments(*arguments): - ''' - Given command-line arguments with which this script was invoked, parse the arguments and return - them as an ArgumentParser instance. - ''' - config_paths = collect.get_default_config_paths() - - parser = ArgumentParser(description='Validate borgmatic configuration file(s).') - parser.add_argument( - '-c', - '--config', - nargs='+', - dest='config_paths', - default=config_paths, - help=f'Configuration filenames or directories, defaults to: {config_paths}', - ) - parser.add_argument( - '-s', - '--show', - action='store_true', - help='Show the validated configuration after all include merging has occurred', +def main(): + warning_log = logging.makeLogRecord( + dict( + levelno=logging.WARNING, + levelname='WARNING', + msg='validate-borgmatic-config is deprecated and will be removed from a future release. Please use "borgmatic config validate" instead.', + ) ) - return parser.parse_args(arguments) - - -def main(): # pragma: no cover - arguments = parse_arguments(*sys.argv[1:]) - - logging.basicConfig(level=logging.INFO, format='%(message)s') - - config_filenames = tuple(collect.collect_config_filenames(arguments.config_paths)) - if len(config_filenames) == 0: - logger.critical('No files to validate found') - sys.exit(1) - - found_issues = False - for config_filename in config_filenames: - try: - config, parse_logs = validate.parse_configuration( - config_filename, validate.schema_filename() - ) - except (ValueError, OSError, validate.Validation_error) as error: - logging.critical(f'{config_filename}: Error parsing configuration file') - logging.critical(error) - found_issues = True - else: - for log in parse_logs: - logger.handle(log) - - if arguments.show: - print('---') - print(borgmatic.config.generate.render_configuration(config)) - - if found_issues: - sys.exit(1) - - logger.info(f"All given configuration files are valid: {', '.join(config_filenames)}") + sys.argv = ['borgmatic', 'config', 'validate'] + sys.argv[1:] + borgmatic.commands.borgmatic.main([warning_log]) diff --git a/docs/Dockerfile b/docs/Dockerfile index 4ac1867b..118768ce 100644 --- a/docs/Dockerfile +++ b/docs/Dockerfile @@ -4,7 +4,7 @@ COPY . /app RUN apk add --no-cache py3-pip py3-ruamel.yaml py3-ruamel.yaml.clib RUN pip install --no-cache /app && generate-borgmatic-config && chmod +r /etc/borgmatic/config.yaml RUN borgmatic --help > /command-line.txt \ - && for action in rcreate transfer create prune compact check extract config "config bootstrap" "config generate" export-tar mount umount restore rlist list rinfo info break-lock borg; do \ + && for action in rcreate transfer create prune compact check extract config "config bootstrap" "config generate" "config validate" export-tar mount umount restore rlist list rinfo info break-lock borg; do \ echo -e "\n--------------------------------------------------------------------------------\n" >> /command-line.txt \ && borgmatic $action --help >> /command-line.txt; done diff --git a/docs/how-to/make-per-application-backups.md b/docs/how-to/make-per-application-backups.md index 1cb37ba0..fb815d87 100644 --- a/docs/how-to/make-per-application-backups.md +++ b/docs/how-to/make-per-application-backups.md @@ -399,9 +399,16 @@ includes. ## Debugging includes -New in version 1.7.12 If you'd +New in version 1.7.15 If you'd like to see what the loaded configuration looks like after includes get merged -in, run `validate-borgmatic-config` on your configuration file: +in, run the `validate` action on your configuration file: + +```bash +sudo borgmatic config validate --show +``` + +In version 1.7.12 through +1.7.14 Use this command instead: ```bash sudo validate-borgmatic-config --show diff --git a/docs/how-to/set-up-backups.md b/docs/how-to/set-up-backups.md index f178400e..7fe69fd9 100644 --- a/docs/how-to/set-up-backups.md +++ b/docs/how-to/set-up-backups.md @@ -185,6 +185,13 @@ redundant](https://torsion.org/borgmatic/docs/how-to/make-backups-redundant/). If you'd like to validate that your borgmatic configuration is valid, the following command is available for that: +```bash +sudo borgmatic config validate +``` + +Prior to version 1.7.15 +Validate a configuration file with this command instead: + ```bash sudo validate-borgmatic-config ``` diff --git a/tests/integration/actions/__init__.py b/tests/integration/actions/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/integration/actions/config/__init__.py b/tests/integration/actions/config/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/integration/actions/config/test_validate.py b/tests/integration/actions/config/test_validate.py new file mode 100644 index 00000000..4dbfd2a3 --- /dev/null +++ b/tests/integration/actions/config/test_validate.py @@ -0,0 +1,37 @@ +import argparse + +from flexmock import flexmock + +import borgmatic.logger +from borgmatic.actions.config import validate as module + + +def test_run_validate_with_show_renders_configurations(): + log_lines = [] + borgmatic.logger.add_custom_log_levels() + + def fake_logger_answer(message): + log_lines.append(message) + + flexmock(module.logger).should_receive('answer').replace_with(fake_logger_answer) + + module.run_validate(argparse.Namespace(show=True), {'test.yaml': {'foo': {'bar': 'baz'}}}) + + assert log_lines == ['''foo:\n bar: baz\n'''] + + +def test_run_validate_with_show_and_multiple_configs_renders_each(): + log_lines = [] + borgmatic.logger.add_custom_log_levels() + + def fake_logger_answer(message): + log_lines.append(message) + + flexmock(module.logger).should_receive('answer').replace_with(fake_logger_answer) + + module.run_validate( + argparse.Namespace(show=True), + {'test.yaml': {'foo': {'bar': 'baz'}}, 'other.yaml': {'quux': 'value'}}, + ) + + assert log_lines == ['---', 'foo:\n bar: baz\n', '---', 'quux: value\n'] diff --git a/tests/integration/commands/test_validate_config.py b/tests/integration/commands/test_validate_config.py index 78887e71..1acd332a 100644 --- a/tests/integration/commands/test_validate_config.py +++ b/tests/integration/commands/test_validate_config.py @@ -3,27 +3,7 @@ from flexmock import flexmock from borgmatic.commands import validate_config as module -def test_parse_arguments_with_no_arguments_uses_defaults(): - config_paths = ['default'] - flexmock(module.collect).should_receive('get_default_config_paths').and_return(config_paths) +def test_main_does_not_raise(): + flexmock(module.borgmatic.commands.borgmatic).should_receive('main') - parser = module.parse_arguments() - - assert parser.config_paths == config_paths - - -def test_parse_arguments_with_multiple_config_paths_parses_as_list(): - flexmock(module.collect).should_receive('get_default_config_paths').and_return(['default']) - - parser = module.parse_arguments('--config', 'myconfig', 'otherconfig') - - assert parser.config_paths == ['myconfig', 'otherconfig'] - - -def test_parse_arguments_supports_show_flag(): - config_paths = ['default'] - flexmock(module.collect).should_receive('get_default_config_paths').and_return(config_paths) - - parser = module.parse_arguments('--config', 'myconfig', '--show') - - assert parser.show + module.main() diff --git a/tests/unit/actions/config/test_generate.py b/tests/unit/actions/config/test_generate.py index 5b82dd35..d25a58e9 100644 --- a/tests/unit/actions/config/test_generate.py +++ b/tests/unit/actions/config/test_generate.py @@ -3,7 +3,7 @@ from flexmock import flexmock from borgmatic.actions.config import generate as module -def test_run_bootstrap_does_not_raise(): +def test_run_generate_does_not_raise(): generate_arguments = flexmock( source_filename=None, destination_filename='destination.yaml', @@ -15,7 +15,7 @@ def test_run_bootstrap_does_not_raise(): module.run_generate(generate_arguments, global_arguments) -def test_run_bootstrap_with_dry_run_does_not_raise(): +def test_run_generate_with_dry_run_does_not_raise(): generate_arguments = flexmock( source_filename=None, destination_filename='destination.yaml', @@ -27,7 +27,7 @@ def test_run_bootstrap_with_dry_run_does_not_raise(): module.run_generate(generate_arguments, global_arguments) -def test_run_bootstrap_with_source_filename_does_not_raise(): +def test_run_generate_with_source_filename_does_not_raise(): generate_arguments = flexmock( source_filename='source.yaml', destination_filename='destination.yaml', diff --git a/tests/unit/actions/config/test_validate.py b/tests/unit/actions/config/test_validate.py new file mode 100644 index 00000000..862d1bfa --- /dev/null +++ b/tests/unit/actions/config/test_validate.py @@ -0,0 +1,17 @@ +from flexmock import flexmock + +from borgmatic.actions.config import validate as module + + +def test_run_validate_does_not_raise(): + validate_arguments = flexmock(show=False) + flexmock(module.borgmatic.config.generate).should_receive('render_configuration') + + module.run_validate(validate_arguments, flexmock()) + + +def test_run_validate_with_show_does_not_raise(): + validate_arguments = flexmock(show=True) + flexmock(module.borgmatic.config.generate).should_receive('render_configuration') + + module.run_validate(validate_arguments, {'test.yaml': flexmock(), 'other.yaml': flexmock()}) diff --git a/tests/unit/commands/test_borgmatic.py b/tests/unit/commands/test_borgmatic.py index f5d7afb4..89eda4ce 100644 --- a/tests/unit/commands/test_borgmatic.py +++ b/tests/unit/commands/test_borgmatic.py @@ -877,7 +877,7 @@ def test_load_configurations_collects_parsed_configurations_and_logs(): configs, logs = tuple(module.load_configurations(('test.yaml', 'other.yaml'))) assert configs == {'test.yaml': configuration, 'other.yaml': other_configuration} - assert logs == test_expected_logs + other_expected_logs + assert set(logs) >= set(test_expected_logs + other_expected_logs) def test_load_configurations_logs_warning_for_permission_error(): @@ -886,7 +886,7 @@ def test_load_configurations_logs_warning_for_permission_error(): configs, logs = tuple(module.load_configurations(('test.yaml',))) assert configs == {} - assert {log.levelno for log in logs} == {logging.WARNING} + assert max(log.levelno for log in logs) == logging.WARNING def test_load_configurations_logs_critical_for_parse_error(): @@ -895,7 +895,7 @@ def test_load_configurations_logs_critical_for_parse_error(): configs, logs = tuple(module.load_configurations(('test.yaml',))) assert configs == {} - assert {log.levelno for log in logs} == {logging.CRITICAL} + assert max(log.levelno for log in logs) == logging.CRITICAL def test_log_record_does_not_raise(): @@ -971,7 +971,9 @@ def test_collect_highlander_action_summary_logs_info_for_success_with_bootstrap( } logs = tuple( - module.collect_highlander_action_summary_logs({'test.yaml': {}}, arguments=arguments) + module.collect_highlander_action_summary_logs( + {'test.yaml': {}}, arguments=arguments, configuration_parse_errors=False + ) ) assert {log.levelno for log in logs} == {logging.ANSWER} @@ -987,7 +989,9 @@ def test_collect_highlander_action_summary_logs_error_on_bootstrap_failure(): } logs = tuple( - module.collect_highlander_action_summary_logs({'test.yaml': {}}, arguments=arguments) + module.collect_highlander_action_summary_logs( + {'test.yaml': {}}, arguments=arguments, configuration_parse_errors=False + ) ) assert {log.levelno for log in logs} == {logging.CRITICAL} @@ -1002,7 +1006,9 @@ def test_collect_highlander_action_summary_logs_error_on_bootstrap_local_borg_ve } logs = tuple( - module.collect_highlander_action_summary_logs({'test.yaml': {}}, arguments=arguments) + module.collect_highlander_action_summary_logs( + {'test.yaml': {}}, arguments=arguments, configuration_parse_errors=False + ) ) assert {log.levelno for log in logs} == {logging.CRITICAL} @@ -1016,7 +1022,9 @@ def test_collect_highlander_action_summary_logs_info_for_success_with_generate() } logs = tuple( - module.collect_highlander_action_summary_logs({'test.yaml': {}}, arguments=arguments) + module.collect_highlander_action_summary_logs( + {'test.yaml': {}}, arguments=arguments, configuration_parse_errors=False + ) ) assert {log.levelno for log in logs} == {logging.ANSWER} @@ -1031,7 +1039,58 @@ def test_collect_highlander_action_summary_logs_error_on_generate_failure(): } logs = tuple( - module.collect_highlander_action_summary_logs({'test.yaml': {}}, arguments=arguments) + module.collect_highlander_action_summary_logs( + {'test.yaml': {}}, arguments=arguments, configuration_parse_errors=False + ) + ) + + assert {log.levelno for log in logs} == {logging.CRITICAL} + + +def test_collect_highlander_action_summary_logs_info_for_success_with_validate(): + flexmock(module.borgmatic.actions.config.validate).should_receive('run_validate') + arguments = { + 'validate': flexmock(), + 'global': flexmock(dry_run=False), + } + + logs = tuple( + module.collect_highlander_action_summary_logs( + {'test.yaml': {}}, arguments=arguments, configuration_parse_errors=False + ) + ) + assert {log.levelno for log in logs} == {logging.ANSWER} + + +def test_collect_highlander_action_summary_logs_error_on_validate_parse_failure(): + flexmock(module.borgmatic.actions.config.validate).should_receive('run_validate') + arguments = { + 'validate': flexmock(), + 'global': flexmock(dry_run=False), + } + + logs = tuple( + module.collect_highlander_action_summary_logs( + {'test.yaml': {}}, arguments=arguments, configuration_parse_errors=True + ) + ) + + assert {log.levelno for log in logs} == {logging.CRITICAL} + + +def test_collect_highlander_action_summary_logs_error_on_run_validate_failure(): + flexmock(module.borgmatic.actions.config.validate).should_receive('run_validate').and_raise( + ValueError + ) + arguments = { + 'validate': flexmock(), + 'global': flexmock(dry_run=False), + } + + logs = tuple( + module.collect_highlander_action_summary_logs( + {'test.yaml': {}}, arguments=arguments, configuration_parse_errors=False + ) ) assert {log.levelno for log in logs} == {logging.CRITICAL}