From 09183464cde4aa9d82a271c3cb19ac7ccabca1a8 Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Wed, 22 Mar 2023 09:41:39 +0530 Subject: [PATCH 1/4] fix: no error on database backups without source dirs --- borgmatic/borg/create.py | 1 + borgmatic/execute.py | 32 +++++++++++++++++++++++--------- tests/unit/test_execute.py | 26 ++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/borgmatic/borg/create.py b/borgmatic/borg/create.py index 87a0fdd..f0169bf 100644 --- a/borgmatic/borg/create.py +++ b/borgmatic/borg/create.py @@ -291,6 +291,7 @@ def collect_special_file_paths( capture_stderr=True, working_directory=working_directory, extra_environment=borg_environment, + raise_on_exit_code_one=False, ) paths = tuple( diff --git a/borgmatic/execute.py b/borgmatic/execute.py index d4b04bf..2daac55 100644 --- a/borgmatic/execute.py +++ b/borgmatic/execute.py @@ -213,7 +213,12 @@ def execute_command( def execute_command_and_capture_output( - full_command, capture_stderr=False, shell=False, extra_environment=None, working_directory=None, + full_command, + capture_stderr=False, + shell=False, + extra_environment=None, + working_directory=None, + raise_on_exit_code_one=True, ): ''' Execute the given command (a sequence of command/argument strings), capturing and returning its @@ -221,20 +226,29 @@ def execute_command_and_capture_output( stdout. If shell is True, execute the command within a shell. If an extra environment dict is given, then use it to augment the current environment, and pass the result into the command. If a working directory is given, use that as the present working directory when running the command. + If raise on exit code one is False, then treat exit code 1 as a warning instead of an error. - Raise subprocesses.CalledProcessError if an error occurs while running the command. + Raise subprocesses.CalledProcessError if an error occurs while running the command, or if the + command exits with a non-zero exit code and raise on exit code one is True. ''' log_command(full_command) environment = {**os.environ, **extra_environment} if extra_environment else None command = ' '.join(full_command) if shell else full_command - output = subprocess.check_output( - command, - stderr=subprocess.STDOUT if capture_stderr else None, - shell=shell, - env=environment, - cwd=working_directory, - ) + try: + output = subprocess.check_output( + command, + stderr=subprocess.STDOUT if capture_stderr else None, + shell=shell, + env=environment, + cwd=working_directory, + ) + logger.warning('Command output: {}'.format(output)) + except subprocess.CalledProcessError as e: + if raise_on_exit_code_one or e.returncode != 1: + raise + output = e.output + logger.warning('Command output: {}'.format(output)) return output.decode() if output is not None else None diff --git a/tests/unit/test_execute.py b/tests/unit/test_execute.py index 0441e9d..29a80cb 100644 --- a/tests/unit/test_execute.py +++ b/tests/unit/test_execute.py @@ -239,6 +239,32 @@ def test_execute_command_and_capture_output_with_capture_stderr_returns_stderr() assert output == expected_output +def test_execute_command_and_capture_output_returns_output_with_raise_on_exit_code_one_false(): + full_command = ['foo', 'bar'] + expected_output = '[]' + err_output = b'[]' + flexmock(module.os, environ={'a': 'b'}) + flexmock(module.subprocess).should_receive('check_output').with_args( + full_command, stderr=None, shell=False, env=None, cwd=None + ).and_raise(subprocess.CalledProcessError(1, full_command, err_output)).once() + + output = module.execute_command_and_capture_output(full_command, raise_on_exit_code_one=False) + + assert output == expected_output + + +def test_execute_command_and_capture_output_returns_output_with_raise_on_exit_code_one_false_and_exit_code_not_one(): + full_command = ['foo', 'bar'] + expected_output = '[]' + flexmock(module.os, environ={'a': 'b'}) + flexmock(module.subprocess).should_receive('check_output').with_args( + full_command, stderr=None, shell=False, env=None, cwd=None + ).and_raise(subprocess.CalledProcessError(2, full_command, expected_output)).once() + + with pytest.raises(subprocess.CalledProcessError): + module.execute_command_and_capture_output(full_command, raise_on_exit_code_one=False) + + def test_execute_command_and_capture_output_returns_output_with_shell(): full_command = ['foo', 'bar'] expected_output = '[]' From bd235f042602a9a29cc5f81880e3b36c21c0fcc3 Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Wed, 22 Mar 2023 16:23:53 +0530 Subject: [PATCH 2/4] use exit_code_indicates_error and modify it to accept a command --- borgmatic/borg/create.py | 2 +- borgmatic/execute.py | 20 +++++-------- tests/integration/test_execute.py | 8 ++--- tests/unit/test_execute.py | 50 ++++++++++++++++--------------- 4 files changed, 38 insertions(+), 42 deletions(-) diff --git a/borgmatic/borg/create.py b/borgmatic/borg/create.py index f0169bf..acc144b 100644 --- a/borgmatic/borg/create.py +++ b/borgmatic/borg/create.py @@ -291,7 +291,7 @@ def collect_special_file_paths( capture_stderr=True, working_directory=working_directory, extra_environment=borg_environment, - raise_on_exit_code_one=False, + treat_exit_code_warning_as_error=False, ) paths = tuple( diff --git a/borgmatic/execute.py b/borgmatic/execute.py index 2daac55..50a0587 100644 --- a/borgmatic/execute.py +++ b/borgmatic/execute.py @@ -11,7 +11,7 @@ ERROR_OUTPUT_MAX_LINE_COUNT = 25 BORG_ERROR_EXIT_CODE = 2 -def exit_code_indicates_error(process, exit_code, borg_local_path=None): +def exit_code_indicates_error(command, exit_code, borg_local_path=None): ''' Return True if the given exit code from running a command corresponds to an error. If a Borg local path is given and matches the process' command, then treat exit code 1 as a warning @@ -20,8 +20,6 @@ def exit_code_indicates_error(process, exit_code, borg_local_path=None): if exit_code is None: return False - command = process.args.split(' ') if isinstance(process.args, str) else process.args - if borg_local_path and command[0] == borg_local_path: return bool(exit_code < 0 or exit_code >= BORG_ERROR_EXIT_CODE) @@ -121,8 +119,9 @@ def log_outputs(processes, exclude_stdouts, output_log_level, borg_local_path): if exit_code is None: still_running = True + command = process.args.split(' ') if isinstance(process.args, str) else process.args # If any process errors, then raise accordingly. - if exit_code_indicates_error(process, exit_code, borg_local_path): + if exit_code_indicates_error(command, exit_code, borg_local_path): # If an error occurs, include its output in the raised exception so that we don't # inadvertently hide error output. output_buffer = output_buffer_for_process(process, exclude_stdouts) @@ -213,12 +212,7 @@ def execute_command( def execute_command_and_capture_output( - full_command, - capture_stderr=False, - shell=False, - extra_environment=None, - working_directory=None, - raise_on_exit_code_one=True, + full_command, capture_stderr=False, shell=False, extra_environment=None, working_directory=None, ): ''' Execute the given command (a sequence of command/argument strings), capturing and returning its @@ -244,10 +238,10 @@ def execute_command_and_capture_output( cwd=working_directory, ) logger.warning('Command output: {}'.format(output)) - except subprocess.CalledProcessError as e: - if raise_on_exit_code_one or e.returncode != 1: + except subprocess.CalledProcessError as error: + if exit_code_indicates_error(error.returncode): raise - output = e.output + output = error.output logger.warning('Command output: {}'.format(output)) return output.decode() if output is not None else None diff --git a/tests/integration/test_execute.py b/tests/integration/test_execute.py index bbdb977..2a9b61d 100644 --- a/tests/integration/test_execute.py +++ b/tests/integration/test_execute.py @@ -138,10 +138,10 @@ def test_log_outputs_kills_other_processes_when_one_errors(): process = subprocess.Popen(['grep'], stdout=subprocess.PIPE, stderr=subprocess.STDOUT) flexmock(module).should_receive('exit_code_indicates_error').with_args( - process, None, 'borg' + ['grep'], None, 'borg' ).and_return(False) flexmock(module).should_receive('exit_code_indicates_error').with_args( - process, 2, 'borg' + ['grep'], 2, 'borg' ).and_return(True) other_process = subprocess.Popen( ['sleep', '2'], stdout=subprocess.PIPE, stderr=subprocess.STDOUT @@ -245,10 +245,10 @@ def test_log_outputs_truncates_long_error_output(): process = subprocess.Popen(['grep'], stdout=subprocess.PIPE, stderr=subprocess.STDOUT) flexmock(module).should_receive('exit_code_indicates_error').with_args( - process, None, 'borg' + ['grep'], None, 'borg' ).and_return(False) flexmock(module).should_receive('exit_code_indicates_error').with_args( - process, 2, 'borg' + ['grep'], 2, 'borg' ).and_return(True) flexmock(module).should_receive('output_buffer_for_process').and_return(process.stdout) diff --git a/tests/unit/test_execute.py b/tests/unit/test_execute.py index 29a80cb..d6c51c0 100644 --- a/tests/unit/test_execute.py +++ b/tests/unit/test_execute.py @@ -7,32 +7,32 @@ from borgmatic import execute as module @pytest.mark.parametrize( - 'process,exit_code,borg_local_path,expected_result', + 'command,exit_code,borg_local_path,expected_result', ( - (flexmock(args=['grep']), 2, None, True), - (flexmock(args=['grep']), 2, 'borg', True), - (flexmock(args=['borg']), 2, 'borg', True), - (flexmock(args=['borg1']), 2, 'borg1', True), - (flexmock(args=['grep']), 1, None, True), - (flexmock(args=['grep']), 1, 'borg', True), - (flexmock(args=['borg']), 1, 'borg', False), - (flexmock(args=['borg1']), 1, 'borg1', False), - (flexmock(args=['grep']), 0, None, False), - (flexmock(args=['grep']), 0, 'borg', False), - (flexmock(args=['borg']), 0, 'borg', False), - (flexmock(args=['borg1']), 0, 'borg1', False), + (['grep'], 2, None, True), + (['grep'], 2, 'borg', True), + (['borg'], 2, 'borg', True), + (['borg1'], 2, 'borg1', True), + (['grep'], 1, None, True), + (['grep'], 1, 'borg', True), + (['borg'], 1, 'borg', False), + (['borg1'], 1, 'borg1', False), + (['grep'], 0, None, False), + (['grep'], 0, 'borg', False), + (['borg'], 0, 'borg', False), + (['borg1'], 0, 'borg1', False), # -9 exit code occurs when child process get SIGKILLed. - (flexmock(args=['grep']), -9, None, True), - (flexmock(args=['grep']), -9, 'borg', True), - (flexmock(args=['borg']), -9, 'borg', True), - (flexmock(args=['borg1']), -9, 'borg1', True), - (flexmock(args=['borg']), None, None, False), + (['grep'], -9, None, True), + (['grep'], -9, 'borg', True), + (['borg'], -9, 'borg', True), + (['borg1'], -9, 'borg1', True), + (['borg'], None, None, False), ), ) def test_exit_code_indicates_error_respects_exit_code_and_borg_local_path( - process, exit_code, borg_local_path, expected_result + command, exit_code, borg_local_path, expected_result ): - assert module.exit_code_indicates_error(process, exit_code, borg_local_path) is expected_result + assert module.exit_code_indicates_error(command, exit_code, borg_local_path) is expected_result def test_command_for_process_converts_sequence_command_to_string(): @@ -239,7 +239,7 @@ def test_execute_command_and_capture_output_with_capture_stderr_returns_stderr() assert output == expected_output -def test_execute_command_and_capture_output_returns_output_with_raise_on_exit_code_one_false(): +def test_execute_command_and_capture_output_returns_output_when_error_code_is_one(): full_command = ['foo', 'bar'] expected_output = '[]' err_output = b'[]' @@ -247,22 +247,24 @@ def test_execute_command_and_capture_output_returns_output_with_raise_on_exit_co flexmock(module.subprocess).should_receive('check_output').with_args( full_command, stderr=None, shell=False, env=None, cwd=None ).and_raise(subprocess.CalledProcessError(1, full_command, err_output)).once() + flexmock(module).should_receive('exit_code_indicates_error').and_return(False).once() - output = module.execute_command_and_capture_output(full_command, raise_on_exit_code_one=False) + output = module.execute_command_and_capture_output(full_command) assert output == expected_output -def test_execute_command_and_capture_output_returns_output_with_raise_on_exit_code_one_false_and_exit_code_not_one(): +def test_execute_command_and_capture_output_returns_output_when_error_code_not_one(): full_command = ['foo', 'bar'] expected_output = '[]' flexmock(module.os, environ={'a': 'b'}) flexmock(module.subprocess).should_receive('check_output').with_args( full_command, stderr=None, shell=False, env=None, cwd=None ).and_raise(subprocess.CalledProcessError(2, full_command, expected_output)).once() + flexmock(module).should_receive('exit_code_indicates_error').and_return(True).once() with pytest.raises(subprocess.CalledProcessError): - module.execute_command_and_capture_output(full_command, raise_on_exit_code_one=False) + module.execute_command_and_capture_output(full_command) def test_execute_command_and_capture_output_returns_output_with_shell(): From 3b5ede8044fa93bd2f7bab40f1a6138af127d27b Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Wed, 22 Mar 2023 23:11:44 +0530 Subject: [PATCH 3/4] remove extra parameter from function call --- borgmatic/borg/create.py | 1 - 1 file changed, 1 deletion(-) diff --git a/borgmatic/borg/create.py b/borgmatic/borg/create.py index acc144b..87a0fdd 100644 --- a/borgmatic/borg/create.py +++ b/borgmatic/borg/create.py @@ -291,7 +291,6 @@ def collect_special_file_paths( capture_stderr=True, working_directory=working_directory, extra_environment=borg_environment, - treat_exit_code_warning_as_error=False, ) paths = tuple( From 1e3a3bf1e7b73fa34e73e6905d6b17c27bb1bd5f Mon Sep 17 00:00:00 2001 From: Divyansh Singh Date: Thu, 23 Mar 2023 01:18:06 +0530 Subject: [PATCH 4/4] review --- borgmatic/execute.py | 6 ++---- tests/unit/test_execute.py | 2 +- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/borgmatic/execute.py b/borgmatic/execute.py index 50a0587..a01e1a1 100644 --- a/borgmatic/execute.py +++ b/borgmatic/execute.py @@ -220,10 +220,8 @@ def execute_command_and_capture_output( stdout. If shell is True, execute the command within a shell. If an extra environment dict is given, then use it to augment the current environment, and pass the result into the command. If a working directory is given, use that as the present working directory when running the command. - If raise on exit code one is False, then treat exit code 1 as a warning instead of an error. - Raise subprocesses.CalledProcessError if an error occurs while running the command, or if the - command exits with a non-zero exit code and raise on exit code one is True. + Raise subprocesses.CalledProcessError if an error occurs while running the command. ''' log_command(full_command) environment = {**os.environ, **extra_environment} if extra_environment else None @@ -239,7 +237,7 @@ def execute_command_and_capture_output( ) logger.warning('Command output: {}'.format(output)) except subprocess.CalledProcessError as error: - if exit_code_indicates_error(error.returncode): + if exit_code_indicates_error(command, error.returncode): raise output = error.output logger.warning('Command output: {}'.format(output)) diff --git a/tests/unit/test_execute.py b/tests/unit/test_execute.py index d6c51c0..3226a5a 100644 --- a/tests/unit/test_execute.py +++ b/tests/unit/test_execute.py @@ -254,7 +254,7 @@ def test_execute_command_and_capture_output_returns_output_when_error_code_is_on assert output == expected_output -def test_execute_command_and_capture_output_returns_output_when_error_code_not_one(): +def test_execute_command_and_capture_output_raises_when_command_errors(): full_command = ['foo', 'bar'] expected_output = '[]' flexmock(module.os, environ={'a': 'b'})