Improve error messages for codeartifact login

Currently codeartifact login command uses check_call() to run
commands, which doesn't capture the stderr from the command if it failed.
This commit uses run() instead and introduces a new error class to show the
captured error message from the command.

.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"
+}

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

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')

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,