diff --git a/.changes/next-release/enhancement-codeartifactlogin-88335.json b/.changes/next-release/enhancement-codeartifactlogin-88335.json new file mode 100644 index 000000000000..3b64f5810bc3 --- /dev/null +++ b/.changes/next-release/enhancement-codeartifactlogin-88335.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "``codeartifact login``", + "description": "Include stderr output from underlying login tool when subprocess fails" +} diff --git a/awscli/compat.py b/awscli/compat.py index 5e896f5b999d..32e16204fe12 100644 --- a/awscli/compat.py +++ b/awscli/compat.py @@ -215,6 +215,13 @@ def get_stderr_text_writer(): return _get_text_writer(sys.stderr, errors="replace") +def get_stderr_encoding(): + encoding = getattr(sys.__stderr__, 'encoding', None) + if encoding is None: + encoding = 'utf-8' + return encoding + + def compat_input(prompt): """ Cygwin's pty's are based on pipes. Therefore, when it interacts with a Win32 diff --git a/awscli/customizations/codeartifact/login.py b/awscli/customizations/codeartifact/login.py index 2365fbe2b009..ee0fbe3cfa3c 100644 --- a/awscli/customizations/codeartifact/login.py +++ b/awscli/customizations/codeartifact/login.py @@ -10,7 +10,7 @@ from dateutil.relativedelta import relativedelta from botocore.utils import parse_timestamp -from awscli.compat import is_windows, urlparse, RawConfigParser, StringIO +from awscli.compat import is_windows, urlparse, RawConfigParser, StringIO, get_stderr_encoding from awscli.customizations import utils as cli_utils from awscli.customizations.commands import BasicCommand from awscli.customizations.utils import uni_print @@ -34,6 +34,17 @@ def get_relative_expiration_time(remaining): return message +class CommandFailedError(Exception): + def __init__(self, called_process_error): + msg = str(called_process_error) + if called_process_error.stderr is not None: + msg +=( + f' Stderr from command:\n' + f'{called_process_error.stderr.decode(get_stderr_encoding())}' + ) + Exception.__init__(self, msg) + + class BaseLogin(object): _TOOL_NOT_FOUND_MESSAGE = '%s was not found. Please verify installation.' @@ -84,14 +95,14 @@ def _run_commands(self, tool, commands, dry_run=False): def _run_command(self, tool, command, *, ignore_errors=False): try: - self.subprocess_utils.check_call( + self.subprocess_utils.run( command, - stdout=self.subprocess_utils.PIPE, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) except subprocess.CalledProcessError as ex: if not ignore_errors: - raise ex + raise CommandFailedError(ex) except OSError as ex: if ex.errno == errno.ENOENT: raise ValueError( @@ -153,13 +164,14 @@ def login(self, dry_run=False): return try: - self.subprocess_utils.check_output( + self.subprocess_utils.run( command, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) except subprocess.CalledProcessError as e: uni_print('Failed to update the NuGet.Config\n') - raise e + raise CommandFailedError(e) uni_print(source_configured_message % source_name) self._write_success_message('nuget') diff --git a/tests/functional/codeartifact/test_codeartifact_login.py b/tests/functional/codeartifact/test_codeartifact_login.py index 4b58c6752709..c402912c12d0 100644 --- a/tests/functional/codeartifact/test_codeartifact_login.py +++ b/tests/functional/codeartifact/test_codeartifact_login.py @@ -40,7 +40,7 @@ def setUp(self): self.pypi_rc_path_mock = self.pypi_rc_path_patch.start() self.pypi_rc_path_mock.return_value = self.test_pypi_rc_path - self.subprocess_patch = mock.patch('subprocess.check_call') + self.subprocess_patch = mock.patch('subprocess.run') self.subprocess_mock = self.subprocess_patch.start() self.subprocess_check_output_patch = mock.patch( 'subprocess.check_output' @@ -51,6 +51,7 @@ def setUp(self): def tearDown(self): self.pypi_rc_path_patch.stop() + self.subprocess_check_output_patch.stop() self.subprocess_patch.stop() self.file_creator.remove_all() @@ -274,8 +275,8 @@ def _assert_subprocess_execution(self, commands): expected_calls = [ mock.call( command, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + capture_output=True, + check=True ) for command in commands ] self.subprocess_mock.assert_has_calls( @@ -336,7 +337,7 @@ def test_nuget_login_without_domain_owner_without_duration_seconds(self): self.assertEqual(result.rc, 0) self._assert_operations_called(package_format='nuget', result=result) self._assert_expiration_printed_to_stdout(result.stdout) - self._assert_subprocess_check_output_execution( + self._assert_subprocess_execution( self._get_nuget_commands() ) @@ -350,7 +351,7 @@ def test_nuget_login_with_domain_owner_without_duration_seconds(self): result=result ) self._assert_expiration_printed_to_stdout(result.stdout) - self._assert_subprocess_check_output_execution( + self._assert_subprocess_execution( self._get_nuget_commands() ) @@ -364,7 +365,7 @@ def test_nuget_login_without_domain_owner_with_duration_seconds(self): result=result ) self._assert_expiration_printed_to_stdout(result.stdout) - self._assert_subprocess_check_output_execution( + self._assert_subprocess_execution( self._get_nuget_commands() ) @@ -383,7 +384,7 @@ def test_nuget_login_with_domain_owner_duration_sections(self): result=result ) self._assert_expiration_printed_to_stdout(result.stdout) - self._assert_subprocess_check_output_execution( + self._assert_subprocess_execution( self._get_nuget_commands() ) @@ -454,7 +455,7 @@ def test_dotnet_login_without_domain_owner_without_duration_seconds(self): self.assertEqual(result.rc, 0) self._assert_operations_called(package_format='nuget', result=result) self._assert_expiration_printed_to_stdout(result.stdout) - self._assert_subprocess_check_output_execution( + self._assert_subprocess_execution( self._get_dotnet_commands() ) @@ -469,7 +470,7 @@ def test_dotnet_login_with_domain_owner_without_duration_seconds(self): result=result ) self._assert_expiration_printed_to_stdout(result.stdout) - self._assert_subprocess_check_output_execution( + self._assert_subprocess_execution( self._get_dotnet_commands() ) @@ -484,7 +485,7 @@ def test_dotnet_login_without_domain_owner_with_duration_seconds(self): result=result ) self._assert_expiration_printed_to_stdout(result.stdout) - self._assert_subprocess_check_output_execution( + self._assert_subprocess_execution( self._get_dotnet_commands() ) @@ -504,7 +505,7 @@ def test_dotnet_login_with_domain_owner_duration_sections(self): result=result ) self._assert_expiration_printed_to_stdout(result.stdout) - self._assert_subprocess_check_output_execution( + self._assert_subprocess_execution( self._get_dotnet_commands() ) @@ -623,7 +624,7 @@ def test_npm_login_always_auth_error_ignored(self): exit code. This is to make sure that login ignores that error and all other commands executes successfully. """ - def side_effect(command, stdout, stderr): + def side_effect(command, capture_output, check): if any('always-auth' in arg for arg in command): raise subprocess.CalledProcessError( returncode=1, diff --git a/tests/unit/customizations/codeartifact/test_adapter_login.py b/tests/unit/customizations/codeartifact/test_adapter_login.py index cd24f4cb4fdc..e5dfae6b9486 100644 --- a/tests/unit/customizations/codeartifact/test_adapter_login.py +++ b/tests/unit/customizations/codeartifact/test_adapter_login.py @@ -1,5 +1,6 @@ import errno import os +import re import signal import subprocess @@ -13,7 +14,7 @@ from awscli.compat import urlparse, RawConfigParser from awscli.customizations.codeartifact.login import ( BaseLogin, NuGetLogin, DotNetLogin, NpmLogin, PipLogin, TwineLogin, - get_relative_expiration_time + get_relative_expiration_time, CommandFailedError ) @@ -51,8 +52,23 @@ def test_get_commands(self): self.endpoint, self.auth_token ) + def test_run_commands_command_failed(self): + error_to_be_caught = subprocess.CalledProcessError( + returncode=1, + cmd=['cmd'], + output=None, + stderr=b'Command error message.' + ) + self.subprocess_utils.run.side_effect = error_to_be_caught + with self.assertRaisesRegex( + CommandFailedError, + rf"{re.escape(str(error_to_be_caught))}" + rf" Stderr from command:\nCommand error message." + ): + self.test_subject._run_commands('tool', ['cmd']) + def test_run_commands_nonexistent_command(self): - self.subprocess_utils.check_call.side_effect = OSError( + self.subprocess_utils.run.side_effect = OSError( errno.ENOENT, 'not found error' ) tool = 'NotSupported' @@ -61,7 +77,7 @@ def test_run_commands_nonexistent_command(self): self.test_subject._run_commands(tool, ['echo', tool]) def test_run_commands_unhandled_error(self): - self.subprocess_utils.check_call.side_effect = OSError( + self.subprocess_utils.run.side_effect = OSError( errno.ENOSYS, 'unhandled error' ) tool = 'NotSupported' @@ -146,9 +162,10 @@ def test_login(self): self.list_operation_command, stderr=self.subprocess_utils.PIPE ) - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( self.add_operation_command, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) def test_login_dry_run(self): @@ -168,9 +185,10 @@ def test_login_old_nuget(self): self.list_operation_command, stderr=self.subprocess_utils.PIPE ) - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( self.add_operation_command, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) def test_login_dry_run_old_nuget(self): @@ -189,9 +207,10 @@ def test_login_source_name_already_exists(self): self.subprocess_utils.check_output.return_value = \ list_response.encode('utf-8') self.test_subject.login() - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( self.update_operation_command, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) def test_login_source_url_already_exists_old_nuget(self): @@ -203,7 +222,7 @@ def test_login_source_url_already_exists_old_nuget(self): self.subprocess_utils.check_output.return_value = \ list_response.encode('utf-8') self.test_subject.login() - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( [ 'nuget', 'sources', 'update', '-name', non_default_source_name, @@ -211,7 +230,8 @@ def test_login_source_url_already_exists_old_nuget(self): '-username', 'aws', '-password', self.auth_token ], - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) def test_login_source_url_already_exists(self): @@ -222,7 +242,7 @@ def test_login_source_url_already_exists(self): self.subprocess_utils.check_output.return_value = \ list_response.encode('utf-8') self.test_subject.login() - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( [ 'nuget', 'sources', 'update', '-name', non_default_source_name, @@ -230,7 +250,8 @@ def test_login_source_url_already_exists(self): '-username', 'aws', '-password', self.auth_token ], - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) def test_login_nuget_not_installed(self): @@ -348,9 +369,10 @@ def test_login(self): self.list_operation_command, stderr=self.subprocess_utils.PIPE ) - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( self.add_operation_command_non_windows, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) @mock.patch('awscli.customizations.codeartifact.login.is_windows', True) @@ -362,9 +384,10 @@ def test_login_on_windows(self): self.list_operation_command, stderr=self.subprocess_utils.PIPE ) - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( self.add_operation_command_windows, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) def test_login_dry_run(self): @@ -385,9 +408,10 @@ def test_login_sources_listed_with_extra_non_list_text(self): self.list_operation_command, stderr=self.subprocess_utils.PIPE ) - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( self.add_operation_command_non_windows, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) @mock.patch('awscli.customizations.codeartifact.login.is_windows', True) @@ -399,9 +423,10 @@ def test_login_sources_listed_with_extra_non_list_text_on_windows(self): self.list_operation_command, stderr=self.subprocess_utils.PIPE ) - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( self.add_operation_command_windows, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) def test_login_sources_listed_with_extra_non_list_text_dry_run(self): @@ -446,9 +471,10 @@ def test_login_source_name_already_exists(self): self.subprocess_utils.check_output.return_value = \ list_response.encode('utf-8') self.test_subject.login() - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( self.update_operation_command_non_windows, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) @mock.patch('awscli.customizations.codeartifact.login.is_windows', True) @@ -459,9 +485,10 @@ def test_login_source_name_already_exists_on_windows(self): self.subprocess_utils.check_output.return_value = \ list_response.encode('utf-8') self.test_subject.login() - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( self.update_operation_command_windows, - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) @mock.patch('awscli.customizations.codeartifact.login.is_windows', True) @@ -473,14 +500,15 @@ def test_login_source_url_already_exists(self): self.subprocess_utils.check_output.return_value = \ list_response.encode('utf-8') self.test_subject.login() - self.subprocess_utils.check_output.assert_called_with( + self.subprocess_utils.run.assert_called_with( [ 'dotnet', 'nuget', 'update', 'source', non_default_source_name, '--source', self.nuget_index_url, '--username', 'aws', '--password', self.auth_token ], - stderr=self.subprocess_utils.PIPE + capture_output=True, + check=True ) def test_login_dotnet_not_installed(self): @@ -544,11 +572,11 @@ def test_login(self): expected_calls = [ mock.call( command, - stdout=self.subprocess_utils.PIPE, - stderr=self.subprocess_utils.PIPE, + capture_output=True, + check=True ) for command in self.commands ] - self.subprocess_utils.check_call.assert_has_calls( + self.subprocess_utils.run.assert_has_calls( expected_calls, any_order=True ) @@ -560,7 +588,7 @@ def test_login_always_auth_error_ignored(self): exit code. This is to make sure that login ignores that error and all other commands executes successfully. """ - def side_effect(command, stdout, stderr): + def side_effect(command, capture_output, check): """Set side_effect for always-auth config setting command""" if any('always-auth' in arg for arg in command): raise subprocess.CalledProcessError( @@ -570,19 +598,19 @@ def side_effect(command, stdout, stderr): return mock.DEFAULT - self.subprocess_utils.check_call.side_effect = side_effect + self.subprocess_utils.run.side_effect = side_effect expected_calls = [] for command in self.commands: expected_calls.append(mock.call( command, - stdout=self.subprocess_utils.PIPE, - stderr=self.subprocess_utils.PIPE, + capture_output=True, + check=True ) ) self.test_subject.login() - self.subprocess_utils.check_call.assert_has_calls( + self.subprocess_utils.run.assert_has_calls( expected_calls, any_order=True ) @@ -669,15 +697,15 @@ def test_get_commands(self): def test_login(self): self.test_subject.login() - self.subprocess_utils.check_call.assert_called_once_with( + self.subprocess_utils.run.assert_called_once_with( ['pip', 'config', 'set', 'global.index-url', self.pip_index_url], - stdout=self.subprocess_utils.PIPE, - stderr=self.subprocess_utils.PIPE, + capture_output=True, + check=True ) def test_login_dry_run(self): self.test_subject.login(dry_run=True) - self.subprocess_utils.check_call.assert_not_called() + self.subprocess_utils.run.assert_not_called() class TestTwineLogin(unittest.TestCase): @@ -776,7 +804,7 @@ def test_login_pypi_rc_not_found_defaults_set(self): def test_login_dry_run(self): self.test_subject.login(dry_run=True) - self.subprocess_utils.check_call.assert_not_called() + self.subprocess_utils.run.assert_not_called() self.assertFalse(os.path.exists(self.test_pypi_rc_path)) def test_login_existing_pypi_rc_not_clobbered(self):