Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions samcli/cli/cli_config_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from click.core import ParameterSource

from samcli.cli.context import Context, get_cmd_names
from samcli.commands._utils.custom_options.structured_output_option import StructuredOutputOption
from samcli.commands.exceptions import ConfigException
from samcli.lib.config.samconfig import DEFAULT_CONFIG_FILE_NAME, DEFAULT_ENV, SamConfig
from samcli.lib.utils.defaults import get_default_aws_region
Expand Down Expand Up @@ -308,6 +309,14 @@ def save_command_line_args_to_config(
"config_env",
]

# The shared structured output flag describes how a single run reports rather than what to
# build, so persisting it would change the output format of later runs, and for sam init it
# would make the interactive flow unreachable. Matched by option type, because sam list and
# sam remote invoke define an unrelated --output that is a display preference worth saving.
params_to_exclude += [

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[GENERAL] The type-based exclusion only covers options created by structured_output_click_option() (build, deploy, init). sam logs and sam traces declare an --output with the same structured-reporting semantics that is still persisted, so the behavior this PR fixes remains reachable through them.

# samcli/commands/_utils/options.py:415-440 (common_observability_click_options)
click.option(
   "--output",
   help="""
   The formatting style of the command output. Following options are available:\n
   TEXT: Prints information as regular text with some formatting (default option)\n
   JSON: Prints each line as JSON without formatting
   """,
   type=click.Choice(OutputOption.__members__, case_sensitive=False),
),

Same OutputOption enum, same text/json values, same "how this run reports" meaning — but a plain click.Option, so isinstance(param, StructuredOutputOption) is False. Both consumers pair it with save_params_option:

  • samcli/commands/logs/command.py:95,98@common_observability_options + @save_params_option
  • samcli/commands/traces/command.py:52,55 — same pair

So sam logs --output json --save-params still writes output = "json" into samconfig.toml, and every later sam logs in that directory emits JSON with nothing on the command line, undoable only by editing the file. Applying cls=StructuredOutputOption to that option too would make the exclusion match the semantics the class docstring describes; as it stands the docstring's distinction ("other commands, such as sam list and sam remote invoke, have an unrelated --output that selects a display format") does not hold for logs and traces.

This was raised on two earlier iterations without a response, so re-raising.

Note on the rest of the diff: the previously flagged items are addressed in this revision — failure_result_json is now reused instead of a local failure document, SAM_TEMPLATE_FILE_NAMES is now the single source for the template search order (options.py:70 consumes it), the capture buffer uses errors="replace", captured template output is re-emitted from a finally so it survives a failing hook, and the hint text is rendered from NON_INTERACTIVE_PARAM_COMBINATIONS rather than restated. The Windows descriptor-swap concern was dismissed by the author with CI evidence, so I did not re-raise it. I verified the new cli_config_filecustom_options.structured_output_option import introduces no cycle (the intermediate __init__.py files are empty and the module imports only click), and that logging goes to stderr, so nothing else pollutes stdout in JSON mode.

param.name for param in ctx.command.params if isinstance(param, StructuredOutputOption) and param.name
]

saved_params = {}
for param_name, param_source in ctx._parameter_source.items():
if param_name in params_to_exclude:
Expand Down
4 changes: 4 additions & 0 deletions samcli/commands/_utils/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@
DEFAULT_BUILD_DIR_WITH_AUTO_DEPENDENCY_LAYER = os.path.join(".aws-sam", "auto-dependency-layer")
DEFAULT_CACHE_DIR = os.path.join(".aws-sam", "cache")
DEFAULT_BUILT_TEMPLATE_PATH = os.path.join(".aws-sam", "build", "template.yaml")

# Template file names SAM CLI recognises, in resolution order. Order matters, so that a template
# path reported by one command is the one another command would resolve to.
SAM_TEMPLATE_FILE_NAMES = ["template.yaml", "template.yml", "template.json"]
Comment thread
roger-zhangg marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
"""
Custom click option for the shared structured output flag
"""

import click


class StructuredOutputOption(click.Option):
"""Marks the shared --output option that selects structured (JSON) output.

Exists so the option can be recognised by type rather than by name. Other commands, such as
sam list and sam remote invoke, have an unrelated --output that selects a display format and
is worth saving to a config file, while this one describes how a single run reports.
"""
6 changes: 5 additions & 1 deletion samcli/commands/_utils/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,12 @@
DEFAULT_BUILT_TEMPLATE_PATH,
DEFAULT_CACHE_DIR,
DEFAULT_STACK_NAME,
SAM_TEMPLATE_FILE_NAMES,
)
from samcli.commands._utils.custom_options.hook_name_option import HookNameOption
from samcli.commands._utils.custom_options.option_nargs import OptionNargs
from samcli.commands._utils.custom_options.replace_help_option import ReplaceHelpSummaryOption
from samcli.commands._utils.custom_options.structured_output_option import StructuredOutputOption
from samcli.commands._utils.parameterized_option import parameterized_option
from samcli.commands._utils.template import TemplateNotFoundException, get_template_artifacts_format, get_template_data
from samcli.lib.hook.hook_wrapper import get_available_hook_packages_ids
Expand Down Expand Up @@ -65,7 +67,7 @@ def get_or_default_template_file_name(ctx, param, provided_value, include_build)

original_template_path = os.path.abspath(provided_value)

search_paths = ["template.yaml", "template.yml", "template.json"]
search_paths = list(SAM_TEMPLATE_FILE_NAMES)

if include_build:
search_paths.insert(0, DEFAULT_BUILT_TEMPLATE_PATH)
Expand Down Expand Up @@ -460,6 +462,8 @@ def structured_output_click_option():
"Supported formats: text (default), json.",
# Derive choices from OutputOption so the accepted CLI values cannot drift from the enum.
type=click.Choice([option.value for option in OutputOption], case_sensitive=False),
# Lets --save-params recognise this option by type, so it is not persisted.
cls=StructuredOutputOption,
)


Expand Down
Loading
Loading