Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Handle input parameters classified as multiple value click options #6108

Merged
merged 7 commits into from
Oct 19, 2023
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
50 changes: 50 additions & 0 deletions samcli/cli/cli_config_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ def __call__(self, config_path: Path, config_env: str, cmd_names: List[str]) ->
# NOTE(TheSriram): change from tomlkit table type to normal dictionary,
# so that click defaults work out of the box.
resolved_config = dict(samconfig.get_all(self.cmd_names, self.section, env=config_env).items())
handle_parse_options(resolved_config)
LOG.debug("Configuration values successfully loaded.")
LOG.debug("Configuration values are: %s", resolved_config)

Expand All @@ -123,6 +124,55 @@ def __call__(self, config_path: Path, config_env: str, cmd_names: List[str]) ->
return resolved_config


def handle_parse_options(resolved_config: dict) -> None:
"""
Click does some handling of options to convert them to the intended types.
When injecting the options to click through a samconfig, we should do a similar
parsing of the options to ensure we pass the intended type.

E.g. if multiple is defined in the click option but only a single value is passed,
handle it the same way click does by converting it to a list first.

Mutates the resolved_config object

Parameters
----------
resolved_config: dict
Configuration options extracted from the configuration file
"""
options_map = get_options_map()
for config_name, config_value in resolved_config.items():
if config_name in options_map:
try:
allow_multiple = options_map[config_name].multiple
mndeveci marked this conversation as resolved.
Show resolved Hide resolved
if allow_multiple and not isinstance(config_value, list):
resolved_config[config_name] = [config_value]
mildaniel marked this conversation as resolved.
Show resolved Hide resolved
LOG.debug(
f"Adjusting value of {config_name} to be a list "
f"since this option is defined with 'multiple=True'"
)
except (AttributeError, KeyError):
LOG.debug(f"Unable to parse option: {config_name}. Leaving option as inputted")


def get_options_map() -> dict:
"""
Attempt to get all of the options that exist for a command.
Return a mapping from each option name to that options' properties.

Returns
-------
dict
Dict of command options if successful, None otherwise
"""
try:
command_options = click.get_current_context().command.params
return {command_option.name: command_option for command_option in command_options}
except AttributeError:
LOG.debug("Unable to get parameters from click context.")
return {}


def configuration_callback(
cmd_name: str,
option_name: str,
Expand Down
4 changes: 4 additions & 0 deletions tests/integration/local/start_api/start_api_integ_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class StartApiIntegBaseClass(TestCase):
integration_dir = str(Path(__file__).resolve().parents[2])
invoke_image: Optional[List] = None
layer_cache_base_dir: Optional[str] = None
config_file: Optional[str] = None

build_before_invoke = False
build_overrides: Optional[Dict[str, str]] = None
Expand Down Expand Up @@ -103,6 +104,9 @@ def start_api(cls):
for image in cls.invoke_image:
command_list += ["--invoke-image", image]

if cls.config_file:
command_list += ["--config-file", cls.config_file]

cls.start_api_process = (
Popen(command_list, stderr=PIPE, stdout=PIPE)
if not cls.project_directory
Expand Down
16 changes: 16 additions & 0 deletions tests/integration/local/start_api/test_start_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -3179,3 +3179,19 @@ def test_can_invoke_lambda_layer_successfully(self):
response = requests.get(self.url + "/", timeout=300)
self.assertEqual(response.status_code, 200)
self.assertEqual(response.content.decode("utf-8"), '"Layer1"')


class TestStartApiDebugPortsConfigFile(StartApiIntegBaseClass):
template_path = "/testdata/start_api/serverless-sample-output.yaml"
config_file = "debug-config.toml"

def setUp(self):
self.url = "http://127.0.0.1:{}".format(self.port)

@pytest.mark.flaky(reruns=3)
@pytest.mark.timeout(timeout=600, method="thread")
def test_starts_process_successfully(self):
response = requests.get(self.url + "/hello-world", timeout=300)

self.assertEqual(response.status_code, 200)
self.assertEqual(response.json(), {"hello": "world"})
4 changes: 4 additions & 0 deletions tests/integration/testdata/start_api/debug-config.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
version = 0.1

[default.local_start_api.parameters]
debug_port = 5858
90 changes: 89 additions & 1 deletion tests/unit/cli/test_cli_config_file.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import os
import tempfile
from dataclasses import dataclass

from pathlib import Path
from typing import List, Optional
from unittest import TestCase, skipIf
from unittest.mock import MagicMock, patch

Expand All @@ -15,6 +17,7 @@
configuration_callback,
get_ctx_defaults,
save_command_line_args_to_config,
handle_parse_options,
)
from samcli.lib.config.exceptions import SamConfigFileReadException, SamConfigVersionException
from samcli.lib.config.samconfig import DEFAULT_ENV
Expand All @@ -38,7 +41,8 @@ def setUp(self):
self.parameters = "parameters"
self.cmd_name = "topic"

def test_toml_valid_with_section(self):
@patch("samcli.cli.cli_config_file.handle_parse_options")
def test_toml_valid_with_section(self, mock_handle_parse_options):
config_dir = tempfile.gettempdir()
config_path = Path(config_dir, "samconfig.toml")
config_path.write_text("version=0.1\n[config_env.topic.parameters]\nword='clarity'\n")
Expand Down Expand Up @@ -363,3 +367,87 @@ def test_dont_save_command_line_args_if_value_is_none(self):
self.assertNotIn("save_params", params.keys(), "--save-params should not be saved to config")
self.assertNotIn("none_param", params.keys(), "Param with None value should not be saved to config")
self.assertNotIn(None, params.values(), "None value should not be saved to config")


@dataclass
class MockParam:
multiple: Optional[bool]
name: Optional[str]


@dataclass
class MockCommand:
params: List[MockParam]


@dataclass
class MockCommandContext:
command: MockCommand


class TestHandleParseOptions(TestCase):
@patch("samcli.cli.cli_config_file.click")
def test_succeeds_updating_option(self, mock_click):
mock_param = MockParam(multiple=True, name="debug_port")
mock_command = MockCommand(params=[mock_param])
mock_context = MockCommandContext(command=mock_command)
mock_click.get_current_context.return_value = mock_context
resolved_config = {"debug_port": 5858}
handle_parse_options(resolved_config)
self.assertEqual(resolved_config, {"debug_port": [5858]})

@patch("samcli.cli.cli_config_file.click")
def test_doesnt_update_not_needed_options(self, mock_click):
mock_param = MockParam(multiple=True, name="debug_port")
mock_command = MockCommand(params=[mock_param])
mock_context = MockCommandContext(command=mock_command)
mock_click.get_current_context.return_value = mock_context
resolved_config = {"debug_port": [5858]}
handle_parse_options(resolved_config)
self.assertEqual(resolved_config, {"debug_port": [5858]})

@patch("samcli.cli.cli_config_file.click")
def test_doesnt_update_multiple_false(self, mock_click):
mock_param = MockParam(multiple=False, name="debug_port")
mock_command = MockCommand(params=[mock_param])
mock_context = MockCommandContext(command=mock_command)
mock_click.get_current_context.return_value = mock_context
resolved_config = {"debug_port": 5858}
handle_parse_options(resolved_config)
self.assertEqual(resolved_config, {"debug_port": 5858})

@patch("samcli.cli.cli_config_file.click")
def test_doesnt_update_not_found(self, mock_click):
mock_param = MockParam(multiple=False, name="debug_port")
mock_command = MockCommand(params=[mock_param])
mock_context = MockCommandContext(command=mock_command)
mock_click.get_current_context.return_value = mock_context
resolved_config = {"other_option": "hello"}
handle_parse_options(resolved_config)
self.assertEqual(resolved_config, {"other_option": "hello"})

@patch("samcli.cli.cli_config_file.LOG")
@patch("samcli.cli.cli_config_file.click")
def test_handles_invalid_param_name(self, mock_click, mock_log):
mock_param = None
mock_command = MockCommand(params=[mock_param])
mock_context = MockCommandContext(command=mock_command)
mock_click.get_current_context.return_value = mock_context
resolved_config = {"other_option": "hello"}
handle_parse_options(resolved_config)
self.assertEqual(resolved_config, {"other_option": "hello"})
mock_log.debug.assert_called_once_with("Unable to get parameters from click context.")

@patch("samcli.cli.cli_config_file.get_options_map")
@patch("samcli.cli.cli_config_file.LOG")
@patch("samcli.cli.cli_config_file.click")
def test_handles_invalid_param_multiple(self, mock_click, mock_log, mock_get_options_map):
mock_param = None
mock_command = MockCommand(params=[mock_param])
mock_context = MockCommandContext(command=mock_command)
mock_click.get_current_context.return_value = mock_context
resolved_config = {"other_option": "hello"}
mock_get_options_map.return_value = {"other_option": "option"}
handle_parse_options(resolved_config)
self.assertEqual(resolved_config, {"other_option": "hello"})
mock_log.debug.assert_called_once_with("Unable to parse option: other_option. Leaving option as inputted")
Loading