From 6fdce2a4a61b53a85146562c09c5d17bc84a32cb Mon Sep 17 00:00:00 2001 From: Dan Helfman Date: Thu, 3 Oct 2024 16:48:34 -0700 Subject: [PATCH] Fix a confusing apparent hang when when the repository location changes, and instead show a helpful error message (#914). --- NEWS | 2 ++ borgmatic/borg/environment.py | 3 +-- borgmatic/commands/borgmatic.py | 10 ++++++++++ tests/unit/borg/test_environment.py | 8 ++++++-- tests/unit/commands/test_borgmatic.py | 18 ++++++++++++++++++ 5 files changed, 37 insertions(+), 4 deletions(-) diff --git a/NEWS b/NEWS index 715c12c9..156e00fd 100644 --- a/NEWS +++ b/NEWS @@ -1,4 +1,6 @@ 1.8.15.dev0 + * #914: Fix a confusing apparent hang when when the repository location changes, and instead + show a helpful error message. * #919: Clarify the command-line help for the "--config" flag. * #919: Document a policy for versioning and breaking changes: https://torsion.org/borgmatic/docs/how-to/upgrade/#versioning-and-breaking-changes diff --git a/borgmatic/borg/environment.py b/borgmatic/borg/environment.py index 30be936c..41ba8011 100644 --- a/borgmatic/borg/environment.py +++ b/borgmatic/borg/environment.py @@ -39,8 +39,7 @@ def make_environment(config): environment_variable_name, ) in DEFAULT_BOOL_OPTION_TO_DOWNCASE_ENVIRONMENT_VARIABLE.items(): value = config.get(option_name) - if value is not None: - environment[environment_variable_name] = 'yes' if value else 'no' + environment[environment_variable_name] = 'yes' if value else 'no' for ( option_name, diff --git a/borgmatic/commands/borgmatic.py b/borgmatic/commands/borgmatic.py index f483a36e..7b6388b4 100644 --- a/borgmatic/commands/borgmatic.py +++ b/borgmatic/commands/borgmatic.py @@ -611,6 +611,9 @@ def log_record(suppress_log=False, **kwargs): return record +BORG_REPOSITORY_ACCESS_ABORTED_EXIT_CODE = 62 + + def log_error_records( message, error=None, levelno=logging.CRITICAL, log_command_error_output=False ): @@ -651,6 +654,13 @@ def log_error_records( ) yield log_record(levelno=levelno, levelname=level_name, msg=str(error)) + + if error.returncode == BORG_REPOSITORY_ACCESS_ABORTED_EXIT_CODE: + yield log_record( + levelno=levelno, + levelname=level_name, + msg='\nTo work around this, set either the "relocated_repo_access_is_ok" or "unknown_unencrypted_repo_access_is_ok" option to "true", as appropriate.', + ) except (ValueError, OSError) as error: yield log_record(levelno=levelno, levelname=level_name, msg=str(message)) yield log_record(levelno=levelno, levelname=level_name, msg=str(error)) diff --git a/tests/unit/borg/test_environment.py b/tests/unit/borg/test_environment.py index 224bd7b3..85944e9c 100644 --- a/tests/unit/borg/test_environment.py +++ b/tests/unit/borg/test_environment.py @@ -19,11 +19,15 @@ def test_make_environment_with_ssh_command_should_set_environment(): assert environment.get('BORG_RSH') == 'ssh -C' -def test_make_environment_without_configuration_should_not_set_environment(): +def test_make_environment_without_configuration_sets_certain_environment_variables(): environment = module.make_environment({}) # borgmatic always sets this Borg environment variable. - assert environment == {'BORG_EXIT_CODES': 'modern'} + assert environment == { + 'BORG_EXIT_CODES': 'modern', + 'BORG_RELOCATED_REPO_ACCESS_IS_OK': 'no', + 'BORG_UNKNOWN_UNENCRYPTED_REPO_ACCESS_IS_OK': 'no', + } def test_make_environment_with_relocated_repo_access_true_should_set_environment_yes(): diff --git a/tests/unit/commands/test_borgmatic.py b/tests/unit/commands/test_borgmatic.py index ffd0e61e..b6b57598 100644 --- a/tests/unit/commands/test_borgmatic.py +++ b/tests/unit/commands/test_borgmatic.py @@ -1168,6 +1168,24 @@ def test_log_error_records_generates_output_logs_for_called_process_error_with_s assert any(log for log in logs if 'error output' in str(log)) +def test_log_error_records_generates_work_around_output_logs_for_called_process_error_with_repository_access_aborted_exit_code(): + flexmock(module).should_receive('log_record').replace_with(dict).times(4) + flexmock(module.logger).should_receive('getEffectiveLevel').and_return(logging.WARNING) + + logs = tuple( + module.log_error_records( + 'Error', + subprocess.CalledProcessError( + module.BORG_REPOSITORY_ACCESS_ABORTED_EXIT_CODE, 'ls', 'error output' + ), + ) + ) + + assert {log['levelno'] for log in logs} == {logging.CRITICAL} + assert any(log for log in logs if 'error output' in str(log)) + assert any(log for log in logs if 'To work around this' in str(log)) + + def test_log_error_records_splits_called_process_error_with_multiline_ouput_into_multiple_logs(): flexmock(module).should_receive('log_record').replace_with(dict).times(4) flexmock(module.logger).should_receive('getEffectiveLevel').and_return(logging.WARNING)