Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changes/next-release/feature-Output-59989.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"type": "feature",
"category": "Output",
"description": "AWS service errors containing modeled fields beyond Code and Message now write structured output to stdout in the configured output format. Set cli_error_format=LEGACY to disable or use --output off to suppress stdout."
}
Comment on lines +1 to +5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--output off is substantial, please add its own entry in a separate file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to see changes, but in hindsight I'd rather have done --output off in a separate PR, these features aren't dependent on one another technically.

75 changes: 65 additions & 10 deletions awscli/clidriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import distro
from botocore import xform_name
from botocore.compat import OrderedDict, copy_kwargs
from botocore.exceptions import ClientError
from botocore.configprovider import (
ChainProvider,
ConstantProvider,
Expand Down Expand Up @@ -76,6 +77,7 @@
)
from awscli.plugin import load_plugins
from awscli.telemetry import add_session_id_component_to_user_agent_extra
from awscli.structured_error import StructuredErrorHandler
from awscli.utils import (
IMDSRegionProvider,
OutputStreamFactory,
Expand Down Expand Up @@ -275,6 +277,9 @@ def _update_config_chain(self):
config_store.set_config_provider(
'cli_help_output', self._construct_cli_help_output_chain()
)
config_store.set_config_provider(
'cli_error_format', self._construct_cli_error_format_chain()
)

def _construct_cli_region_chain(self):
providers = [
Expand Down Expand Up @@ -368,6 +373,20 @@ def _construct_cli_auto_prompt_chain(self):
]
return ChainProvider(providers=providers)

def _construct_cli_error_format_chain(self):
providers = [
EnvironmentProvider(
name='AWS_CLI_ERROR_FORMAT',
env=os.environ,
),
ScopedConfigProvider(
config_var_name='cli_error_format',
session=self.session,
),
ConstantProvider(value='STANDARD'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few things on the enum values:

  1. Left a comment elsewhere, but please start by documenting valid values in global_options.rst
  2. Add validation like we do for output for invalid values.
  3. Only STANDARD | LEGACY were never discussed as options, we should revisit with the team internally.

]
return ChainProvider(providers=providers)

@property
def subcommand_table(self):
return self._get_command_table()
Expand Down Expand Up @@ -983,6 +1002,9 @@ class CLIOperationCaller:
def __init__(self, session):
self._session = session
self._output_stream_factory = OutputStreamFactory(session)
self._structured_error_handler = StructuredErrorHandler(
session, self._output_stream_factory
)

def invoke(self, service_name, operation_name, parameters, parsed_globals):
"""Invoke an operation and format the response.
Expand Down Expand Up @@ -1023,21 +1045,54 @@ def invoke(self, service_name, operation_name, parameters, parsed_globals):
def _make_client_call(
self, client, operation_name, parameters, parsed_globals
):
py_operation_name = xform_name(operation_name)
if client.can_paginate(py_operation_name) and parsed_globals.paginate:
paginator = client.get_paginator(py_operation_name)
response = paginator.paginate(**parameters)
else:
response = getattr(client, xform_name(operation_name))(
**parameters
try:
py_operation_name = xform_name(operation_name)
if (
client.can_paginate(py_operation_name)
and parsed_globals.paginate
):
paginator = client.get_paginator(py_operation_name)
response = paginator.paginate(**parameters)
else:
response = getattr(client, py_operation_name)(**parameters)
return response
except ClientError as e:
# Display structured error output before re-raising
self._display_structured_error_for_exception(
e, parsed_globals
)
raise

def _display_structured_error_for_exception(
self, exception, parsed_globals
):
try:
error_response = (
self._structured_error_handler.extract_error_response(
exception
)
)
if error_response:
self._structured_error_handler.handle_error(
error_response, parsed_globals
)
except Exception as e:
# Don't let structured error display break error handling
LOG.debug(
'Failed to display structured error: %s',
e,
exc_info=True,
)
return response

def _display_response(self, command_name, response, parsed_globals):
output = parsed_globals.output
if output is None:
output = self._session.get_config_variable('output')

formatter = get_formatter(output, parsed_globals)
with self._output_stream_factory.get_output_stream() as stream:
formatter(command_name, response, stream)
try:
with self._output_stream_factory.get_output_stream() as stream:
formatter(command_name, response, stream)
except ClientError as e:
self._display_structured_error_for_exception(e, parsed_globals)
raise
Comment on lines +1096 to +1098
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this block? Can the formatter raise a structured error that the actual API call wouldn't have?

3 changes: 2 additions & 1 deletion awscli/data/cli.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
"text",
"table",
"yaml",
"yaml-stream"
"yaml-stream",
"off"
],
"help": "<p>The formatting style for command output.</p>"
},
Expand Down
2 changes: 2 additions & 0 deletions awscli/examples/global_options.rst
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@

* yaml-stream

* off
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're here, can you catch up the section in the API reference too?

The valid values of the ``output`` configuration variable are:
* json
* table
* text

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add new information there on the new error format setting.



``--query`` (string)

Expand Down
17 changes: 14 additions & 3 deletions awscli/formatter.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ def _format_response(self, command_name, response, stream):
try:
self.table.render(stream)
except OSError:
# If they're piping stdout to another process which exits before
# we're done writing all of our output, we'll get an error about a
# closed pipe which we can safely ignore.
# If they're piping stdout to another process which exits
# before we're done writing all of our output, we'll get an
# error about a closed pipe which we can safely ignore.
pass

def _build_table(self, title, current, indent_level=0):
Expand Down Expand Up @@ -368,12 +368,23 @@ def _format_response(self, response, stream):
text.format_text(response, stream)


class OffFormatter(Formatter):
"""Formatter that suppresses all output.
Only stdout is suppressed; stderr (error messages) remains visible.
"""

def __call__(self, command_name, response, stream=None):
# Suppress all output
pass


CLI_OUTPUT_FORMATS = {
'json': JSONFormatter,
'text': TextFormatter,
'table': TableFormatter,
'yaml': YAMLFormatter,
'yaml-stream': StreamedYAMLFormatter,
'off': OffFormatter,
}


Expand Down
94 changes: 94 additions & 0 deletions awscli/structured_error.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
# Copyright 2012-2013 Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Throughout, either drop years, or use the short version:

# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
# SPDX-License-Identifier: Apache-2.0

#
# Licensed under the Apache License, Version 2.0 (the "License"). You
# may not use this file except in compliance with the License. A copy of
# the License is located at
#
# http://aws.amazon.com/apache2.0/
#
# or in the "license" file accompanying this file. This file is
# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
# ANY KIND, either express or implied. See the License for the specific
# language governing permissions and limitations under the License.
import logging

from botocore.exceptions import ClientError

from awscli.formatter import get_formatter


LOG = logging.getLogger('awscli.structured_error')


class StructuredErrorHandler:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be handled through the existing ClientErrorHandler? That already has an overridable method with access to stdout/stderr.

We'd need less new plumbing in the driver, and we already should have confidence that it is catching ClientError instances.

I also wonder if that'll help apply more of this to custom commands.

"""Handles display of structured error information from AWS services.
This class is responsible for determining when to display structured
error output and formatting it appropriately based on user configuration
and output format settings.
"""

def __init__(self, session, output_stream_factory):
self._session = session
self._output_stream_factory = output_stream_factory

def handle_error(self, error_response, parsed_globals):
error_info = error_response.get('Error', {})

if self.should_display(error_info, parsed_globals):
self.display(error_info, parsed_globals)

def should_display(self, error_info, parsed_globals):
if not self._has_additional_error_members(error_info):
return False

config_store = self._session.get_component('config_store')
error_format = config_store.get_config_variable('cli_error_format')
if error_format == 'LEGACY':
return False

output = self._get_output_format(parsed_globals)
if output == 'off':
return False

return True

def display(self, error_response, parsed_globals):
output = self._get_output_format(parsed_globals)

try:
formatter = get_formatter(output, parsed_globals)

with self._output_stream_factory.get_output_stream() as stream:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In the last internal review of the design doc, we weren't keen anymore on writing these to stdout. Users checking for any output and assuming that means the command was successful may be broken.
  2. I definitely don't think we should feed errors into the pager. It's odd to need to q to see the original message. Though they can contain lists, there is no mechanism for pagination.

formatter('error', error_response, stream)
except Exception as e:
# Log the error but don't let it prevent the normal error handling
LOG.debug(
"Failed to display structured error output: %s",
e,
exc_info=True,
)

def _get_output_format(self, parsed_globals):
output = parsed_globals.output
if output is None:
output = self._session.get_config_variable('output')
return output

def _has_additional_error_members(self, error_response):
if not error_response:
return False

standard_keys = {'Code', 'Message'}
error_keys = set(error_response.keys())
return len(error_keys - standard_keys) > 0

@staticmethod
def extract_error_response(exception):
if not isinstance(exception, ClientError):
return None

if hasattr(exception, 'response') and 'Error' in exception.response:
return {'Error': exception.response['Error']}

return None
2 changes: 1 addition & 1 deletion tests/functional/autocomplete/test_completer.py
Original file line number Diff line number Diff line change
Expand Up @@ -483,7 +483,7 @@ def test_return_suggestions_for_global_arg_with_choices(self):
suggestions = self.completer.complete(parsed)
names = [s.name for s in suggestions]
self.assertEqual(
names, ['json', 'text', 'table', 'yaml', 'yaml-stream']
names, ['json', 'text', 'table', 'yaml', 'yaml-stream', 'off']
)

def test_not_return_suggestions_for_global_arg_wo_trailing_space(self):
Expand Down
48 changes: 47 additions & 1 deletion tests/unit/test_formatter.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,12 @@
from botocore.paginate import PageIterator

from awscli.compat import StringIO, contextlib
from awscli.formatter import JSONFormatter, StreamedYAMLFormatter, YAMLDumper
from awscli.formatter import (
JSONFormatter,
OffFormatter,
StreamedYAMLFormatter,
YAMLDumper,
)
from awscli.testutils import mock, unittest


Expand Down Expand Up @@ -180,3 +185,44 @@ def test_encoding_override(self, env_vars):
'}\n'
).encode()
)


class TestOffFormatter:
def setup_method(self):
self.args = Namespace(query=None)
self.formatter = OffFormatter(self.args)
self.output = StringIO()

def test_suppresses_simple_response(self):
response = {'Key': 'Value'}
self.formatter('test-command', response, self.output)
assert self.output.getvalue() == ''

def test_suppresses_complex_response(self):
response = {
'Items': [
{'Name': 'Item1', 'Value': 'data'},
{'Name': 'Item2', 'Value': 'more-data'}
],
'Count': 2
}
self.formatter('test-command', response, self.output)
assert self.output.getvalue() == ''

def test_suppresses_empty_response(self):
response = {}
self.formatter('test-command', response, self.output)
assert self.output.getvalue() == ''

def test_suppresses_paginated_response(self):
response = FakePageIterator([
{'Items': ['Item1']},
{'Items': ['Item2']}
])
self.formatter('test-command', response, self.output)
assert self.output.getvalue() == ''

def test_works_without_stream(self):
response = {'Key': 'Value'}
# Should not raise an exception
self.formatter('test-command', response, None)
Loading
Loading