From 77bc0959d4df57be70463dcb227836c6a46fbf78 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Tue, 4 Feb 2025 22:11:45 -0500 Subject: [PATCH 01/13] Support file:// for parameter-overrides --- samcli/cli/types.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index a3743128ad..137a869950 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -6,10 +6,12 @@ import logging import re from json import JSONDecodeError +from pathlib import Path from typing import Dict, List, Optional, Union import click +from samcli.lib.config.file_manager import FILE_MANAGER_MAPPER from samcli.lib.package.ecr_utils import is_ecr_url PARAM_AND_METADATA_KEY_REGEX = """([A-Za-z0-9\\"\']+)""" @@ -97,6 +99,19 @@ def convert(self, value, param, ctx): value = (value,) if isinstance(value, str) else value for val in value: + if val.startswith('file://'): + filepath = Path(val[7:]) + if not filepath.is_file(): + return self.fail(f"{val} was not found or is a directory", param, ctx) + file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) + if not file_manager: + return self.fail(f"{val} uses an unsupported extension", param, ctx) + try: + parameters = file_manager.read(filepath) + except Exception as e: + return self.fail(str(e), param, ctx) + result |= parameters + continue # Add empty string to start of the string to help match `_pattern2` normalized_val = " " + val.strip() From 18d6d40feb9729f960274e293b4f30a0904d65af Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 5 Feb 2025 11:41:27 -0500 Subject: [PATCH 02/13] Flatten nested lists and normalize parameters --- samcli/cli/types.py | 107 ++++++++++++++++++++++++++------------------ 1 file changed, 64 insertions(+), 43 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 137a869950..ab22a03a2e 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -10,6 +10,9 @@ from typing import Dict, List, Optional, Union import click +import ruamel.yaml + +from ruamel.yaml.comments import CommentedMap, CommentedSeq from samcli.lib.config.file_manager import FILE_MANAGER_MAPPER from samcli.lib.package.ecr_utils import is_ecr_url @@ -62,6 +65,24 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") +def _format_value(value): + if isinstance(value, bool): + return str(value).lower() + if isinstance(value, int): + return str(value) + if isinstance(value, list): + return f'{",".join(map(str, value))}' + return f'{value}' + +def _flatten_list(data): + flat_data = [] + for item in data: + if isinstance(item, list): + flat_data.extend(_flatten_list(item)) + else: + flat_data.append(item) + return flat_data + class CfnParameterOverridesType(click.ParamType): """ @@ -90,56 +111,56 @@ class CfnParameterOverridesType(click.ParamType): name = "string,list" - def convert(self, value, param, ctx): + def convert(self, values, param, ctx): result = {} # Empty tuple - if value == ("",): + if values == ("",): return result - value = (value,) if isinstance(value, str) else value - for val in value: - if val.startswith('file://'): - filepath = Path(val[7:]) - if not filepath.is_file(): - return self.fail(f"{val} was not found or is a directory", param, ctx) - file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) - if not file_manager: - return self.fail(f"{val} uses an unsupported extension", param, ctx) - try: - parameters = file_manager.read(filepath) - except Exception as e: - return self.fail(str(e), param, ctx) - result |= parameters - continue - # Add empty string to start of the string to help match `_pattern2` - normalized_val = " " + val.strip() + LOG.debug("Input parameters: %s", values) + + # Flatten to support YAML anchors & aliases + values = _flatten_list(values) if isinstance(values, (list, CommentedSeq)) else [values,] + LOG.debug("Flattened parameters: %s", values) + + parameters = {} + for value in values: + if isinstance(value, str): + if value.startswith('file://'): + filepath = Path(value[7:]) + if not filepath.is_file(): + self.fail(f"{value} was not found or is a directory", param, ctx) + file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) + if not file_manager: + self.fail(f"{value} uses an unsupported extension", param, ctx) + parameters |= self.convert(file_manager.read(filepath), param, ctx) + else: + # Legacy parameter matching + normalized_value = " " + value.strip() + for pattern in self.ordered_pattern_match: + groups = re.findall(pattern, normalized_value) + if groups: + # Group tuples can be merged into dict + parameters |= groups + break + else: + self.fail( + f"{value} is not in valid format. It must look something like '{self.__EXAMPLE_1}' or '{self.__EXAMPLE_2}'", + param, + ctx, + ) + + elif isinstance(value, CommentedMap) or isinstance(value, dict): + # e.g. YAML key-value pairs + for k, v in value.items(): + parameters[k] = _format_value(v) - try: - # NOTE(TheSriram): find the first regex that matched. - # pylint is concerned that we are checking at the same `val` within the loop, - # but that is the point, so disabling it. - pattern = next( - i - for i in filter( - lambda item: re.findall(item, normalized_val), self.ordered_pattern_match - ) # pylint: disable=cell-var-from-loop - ) - except StopIteration: - return self.fail( - "{} is not in valid format. It must look something like '{}' or '{}'".format( - val, self.__EXAMPLE_1, self.__EXAMPLE_2 - ), - param, - ctx, - ) - - groups = re.findall(pattern, normalized_val) - - # 'groups' variable is a list of tuples ex: [(key1, value1), (key2, value2)] - for key, param_value in groups: - result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) + result = {} + for key, param_value in parameters.items(): + result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) + LOG.debug("Output parameters: %s", result) return result From 42b5bc60f3fbaf7cedfa44d02cd6adfff52f2514 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 5 Feb 2025 12:13:47 -0500 Subject: [PATCH 03/13] handle empty param files and non-string yaml keys --- samcli/cli/types.py | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index ab22a03a2e..71de82efc5 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -65,15 +65,6 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") -def _format_value(value): - if isinstance(value, bool): - return str(value).lower() - if isinstance(value, int): - return str(value) - if isinstance(value, list): - return f'{",".join(map(str, value))}' - return f'{value}' - def _flatten_list(data): flat_data = [] for item in data: @@ -114,8 +105,9 @@ class CfnParameterOverridesType(click.ParamType): def convert(self, values, param, ctx): result = {} - # Empty tuple - if values == ("",): + # Empty tuple or empty param file + if values == ("",) or values == "" or values is None: + LOG.debug("Empty parameter set (%s)", values) return result LOG.debug("Input parameters: %s", values) @@ -126,6 +118,8 @@ def convert(self, values, param, ctx): parameters = {} for value in values: + LOG.debug("Processing parameter: %s", value) + if isinstance(value, str): if value.startswith('file://'): filepath = Path(value[7:]) @@ -154,9 +148,20 @@ def convert(self, values, param, ctx): elif isinstance(value, CommentedMap) or isinstance(value, dict): # e.g. YAML key-value pairs for k, v in value.items(): - parameters[k] = _format_value(v) + if isinstance(value, list): + parameters[str(k)] = ",".join(map(str, value)) # Collapse lists to csv + else: + parameters[str(k)] = str(v) + else: + self.fail( + "This error should never appear.", + param, + ctx, + ) result = {} + LOG.debug("Pre-unwrap parameters: %s", parameters) + for key, param_value in parameters.items(): result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) From be1a66317c6858a015f191ad80e54648f4a71d1c Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 5 Feb 2025 12:46:37 -0500 Subject: [PATCH 04/13] fix list collapse to comma delimited --- samcli/cli/types.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 71de82efc5..e6fb532dd7 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -145,11 +145,11 @@ def convert(self, values, param, ctx): ctx, ) - elif isinstance(value, CommentedMap) or isinstance(value, dict): + elif isinstance(value, (dict, CommentedMap)): # e.g. YAML key-value pairs for k, v in value.items(): - if isinstance(value, list): - parameters[str(k)] = ",".join(map(str, value)) # Collapse lists to csv + if isinstance(v, (list, CommentedSeq)): + parameters[str(k)] = ",".join(map(str, v)) # Collapse lists to comma delimited else: parameters[str(k)] = str(v) else: From abf99263794893a118dd7e86fd4fbc80ea6c27f4 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 5 Feb 2025 13:16:49 -0500 Subject: [PATCH 05/13] handle tuples from cli parameter-overrides --- samcli/cli/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index e6fb532dd7..921ecf75ee 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -113,7 +113,7 @@ def convert(self, values, param, ctx): LOG.debug("Input parameters: %s", values) # Flatten to support YAML anchors & aliases - values = _flatten_list(values) if isinstance(values, (list, CommentedSeq)) else [values,] + values = _flatten_list(values) if isinstance(values, (tuple, list, CommentedSeq)) else (values,) LOG.debug("Flattened parameters: %s", values) parameters = {} From 27f03caa768f4599e78f00adac7cddfd7ef300d6 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Thu, 6 Feb 2025 10:13:58 -0500 Subject: [PATCH 06/13] Merge samconfig and cli parameter overrides --- samcli/cli/types.py | 68 ++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 25 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 921ecf75ee..71801c04b6 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -10,7 +10,6 @@ from typing import Dict, List, Optional, Union import click -import ruamel.yaml from ruamel.yaml.comments import CommentedMap, CommentedSeq @@ -65,10 +64,23 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") -def _flatten_list(data): +def _flatten_list(data: list) -> list: + """ + Recursively flattens lists and other list-like types for easy sequential processing. + This also helps with lists combined with YAML anchors & aliases. + + Parameters + ---------- + value : data + List to flatten + + Returns + ------- + Flattened list + """ flat_data = [] for item in data: - if isinstance(item, list): + if isinstance(item, (tuple, list, CommentedSeq)): flat_data.extend(_flatten_list(item)) else: flat_data.append(item) @@ -83,6 +95,8 @@ class CfnParameterOverridesType(click.ParamType): __EXAMPLE_1 = "ParameterKey=KeyPairName,ParameterValue=MyKey ParameterKey=InstanceType,ParameterValue=t1.micro" __EXAMPLE_2 = "KeyPairName=MyKey InstanceType=t1.micro" + __EXAMPLE_3 = "file://MyParams.yaml" + # Regex that parses CloudFormation parameter key-value pairs: # https://regex101.com/r/xqfSjW/2 @@ -100,26 +114,21 @@ class CfnParameterOverridesType(click.ParamType): ordered_pattern_match = [_pattern_1, _pattern_2] - name = "string,list" - - def convert(self, values, param, ctx): - result = {} - - # Empty tuple or empty param file - if values == ("",) or values == "" or values is None: + def _normalize_parameters(self, values, param, ctx): + """ + Normalizes parameter overrides into key-value pairs of strings + Later keys overwrite previous ones in case of key conflict + """ + if values in (("",), "", None) or values == {}: LOG.debug("Empty parameter set (%s)", values) - return result + return {} LOG.debug("Input parameters: %s", values) - - # Flatten to support YAML anchors & aliases - values = _flatten_list(values) if isinstance(values, (tuple, list, CommentedSeq)) else (values,) + values = _flatten_list([values]) LOG.debug("Flattened parameters: %s", values) parameters = {} for value in values: - LOG.debug("Processing parameter: %s", value) - if isinstance(value, str): if value.startswith('file://'): filepath = Path(value[7:]) @@ -128,47 +137,56 @@ def convert(self, values, param, ctx): file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) if not file_manager: self.fail(f"{value} uses an unsupported extension", param, ctx) - parameters |= self.convert(file_manager.read(filepath), param, ctx) + parameters |= self._normalize_parameters(file_manager.read(filepath), param, ctx) else: # Legacy parameter matching normalized_value = " " + value.strip() for pattern in self.ordered_pattern_match: groups = re.findall(pattern, normalized_value) if groups: - # Group tuples can be merged into dict parameters |= groups break else: self.fail( - f"{value} is not in valid format. It must look something like '{self.__EXAMPLE_1}' or '{self.__EXAMPLE_2}'", + f"{value} is not a valid format. It must look something like '{self.__EXAMPLE_1}', '{self.__EXAMPLE_2}', or '{self.__EXAMPLE_3}'", param, ctx, ) - elif isinstance(value, (dict, CommentedMap)): # e.g. YAML key-value pairs for k, v in value.items(): if isinstance(v, (list, CommentedSeq)): - parameters[str(k)] = ",".join(map(str, v)) # Collapse lists to comma delimited + # Collapse list values to comma delimited + parameters[str(k)] = ",".join(map(str, v)) else: parameters[str(k)] = str(v) else: self.fail( - "This error should never appear.", + f"{value} is not valid in a way the code doesn't expect", param, ctx, ) result = {} - LOG.debug("Pre-unwrap parameters: %s", parameters) - for key, param_value in parameters.items(): result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) - LOG.debug("Output parameters: %s", result) return result + def convert(self, values, param, ctx): + # Merge samconfig with CLI parameter-overrides + if isinstance(values, tuple): # Tuple implies CLI + # default_map was populated from samconfig + default_map = ctx.default_map.get('parameter_overrides', {}) + LOG.debug("Default map: %s", default_map) + LOG.debug("Current values: %s", values) + # Easy merge - will flatten later + values = [default_map] + [values] + result = self._normalize_parameters(values, param, ctx) + return result + + class CfnMetadataType(click.ParamType): """ Custom Click options type to accept values for metadata parameters. From 4c740cfc025e859108dd21cb2b9174ab90ee8a26 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Thu, 6 Feb 2025 13:00:10 -0500 Subject: [PATCH 07/13] Lint --- samcli/cli/types.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 71801c04b6..b3cc30f07e 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -10,7 +10,6 @@ from typing import Dict, List, Optional, Union import click - from ruamel.yaml.comments import CommentedMap, CommentedSeq from samcli.lib.config.file_manager import FILE_MANAGER_MAPPER @@ -64,7 +63,8 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") -def _flatten_list(data: list) -> list: + +def _flatten_list(data: list | tuple) -> list: """ Recursively flattens lists and other list-like types for easy sequential processing. This also helps with lists combined with YAML anchors & aliases. @@ -97,7 +97,6 @@ class CfnParameterOverridesType(click.ParamType): __EXAMPLE_2 = "KeyPairName=MyKey InstanceType=t1.micro" __EXAMPLE_3 = "file://MyParams.yaml" - # Regex that parses CloudFormation parameter key-value pairs: # https://regex101.com/r/xqfSjW/2 # https://regex101.com/r/xqfSjW/5 @@ -116,8 +115,8 @@ class CfnParameterOverridesType(click.ParamType): def _normalize_parameters(self, values, param, ctx): """ - Normalizes parameter overrides into key-value pairs of strings - Later keys overwrite previous ones in case of key conflict + Normalizes parameter overrides into key-value pairs of strings + Later keys overwrite previous ones in case of key conflict """ if values in (("",), "", None) or values == {}: LOG.debug("Empty parameter set (%s)", values) @@ -130,7 +129,7 @@ def _normalize_parameters(self, values, param, ctx): parameters = {} for value in values: if isinstance(value, str): - if value.startswith('file://'): + if value.startswith("file://"): filepath = Path(value[7:]) if not filepath.is_file(): self.fail(f"{value} was not found or is a directory", param, ctx) @@ -148,7 +147,8 @@ def _normalize_parameters(self, values, param, ctx): break else: self.fail( - f"{value} is not a valid format. It must look something like '{self.__EXAMPLE_1}', '{self.__EXAMPLE_2}', or '{self.__EXAMPLE_3}'", + f"{value} is not a valid format. It must look something like " + f"'{self.__EXAMPLE_1}', '{self.__EXAMPLE_2}', or '{self.__EXAMPLE_3}'", param, ctx, ) @@ -169,16 +169,15 @@ def _normalize_parameters(self, values, param, ctx): result = {} for key, param_value in parameters.items(): - result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) + result[_unquote_wrapped_quotes(key.lower())] = _unquote_wrapped_quotes(param_value) LOG.debug("Output parameters: %s", result) return result - def convert(self, values, param, ctx): # Merge samconfig with CLI parameter-overrides - if isinstance(values, tuple): # Tuple implies CLI + if isinstance(values, tuple): # Tuple implies CLI # default_map was populated from samconfig - default_map = ctx.default_map.get('parameter_overrides', {}) + default_map = ctx.default_map.get("parameter_overrides", {}) LOG.debug("Default map: %s", default_map) LOG.debug("Current values: %s", values) # Easy merge - will flatten later From 124d78f0c7952aecc92a411be484190f19efd3e1 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Fri, 7 Feb 2025 11:24:13 -0500 Subject: [PATCH 08/13] Add unit tests, apply related fixes --- samcli/cli/types.py | 14 ++-- tests/unit/cli/test_types.py | 143 +++++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 5 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index b3cc30f07e..4aa29fda1c 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -155,11 +155,15 @@ def _normalize_parameters(self, values, param, ctx): elif isinstance(value, (dict, CommentedMap)): # e.g. YAML key-value pairs for k, v in value.items(): - if isinstance(v, (list, CommentedSeq)): - # Collapse list values to comma delimited - parameters[str(k)] = ",".join(map(str, v)) + if v is None: + parameters[str(k)] = "" + elif isinstance(v, (list, CommentedSeq)): + # Collapse list values to comma delimited, ignore empty strings and remove extra spaces + parameters[str(k)] = ",".join(str(x).strip() for x in v if x not in (None, "")) else: parameters[str(k)] = str(v) + elif value is None: + continue else: self.fail( f"{value} is not valid in a way the code doesn't expect", @@ -169,13 +173,13 @@ def _normalize_parameters(self, values, param, ctx): result = {} for key, param_value in parameters.items(): - result[_unquote_wrapped_quotes(key.lower())] = _unquote_wrapped_quotes(param_value) + result[_unquote_wrapped_quotes(key)] = _unquote_wrapped_quotes(param_value) LOG.debug("Output parameters: %s", result) return result def convert(self, values, param, ctx): # Merge samconfig with CLI parameter-overrides - if isinstance(values, tuple): # Tuple implies CLI + if isinstance(values, tuple) and ctx: # ctx isn't set in all unit tests # default_map was populated from samconfig default_map = ctx.default_map.get("parameter_overrides", {}) LOG.debug("Default map: %s", default_map) diff --git a/tests/unit/cli/test_types.py b/tests/unit/cli/test_types.py index f6943107f6..23e175fa2f 100644 --- a/tests/unit/cli/test_types.py +++ b/tests/unit/cli/test_types.py @@ -150,7 +150,150 @@ def test_successful_parsing(self, input, expected): self.assertEqual(result, expected, msg="Failed with Input = " + str(input)) + @parameterized.expand( + [ + ("", "", {}, ), + (("A=1 A=2"), ("A=3 A=4",), {"A": "4"}), + (("Path=C:\\Windows\\System32"),("Path=/usr/bin",), {"Path": "/usr/bin"}), + ( + ("A=1 B=2"), + ("ParameterKey=C,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), + {"A": "1", "B": "2", "C": "3", "D": "4"}, + ), + ( + ('Quoted="Hello, World!"'), + ('ParameterKey=Quoted,ParameterValue="\'Hi\'"',), + {"Quoted": "'Hi'"} + ), + ( + ("A=1 B=2"), + ("ParameterKey=A,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), + {"A": "3", "B": "2", "D": "4"}, + ), + ( + ["A=1", "B=2"], + ("ParameterKey=A,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), + {"A": "3", "B": "2", "D": "4"}, + ), + ( + ("A=1,2,3 B=1,2,3"), + ("ParameterKey=A,ParameterValue=2,2", "ParameterKey=D,ParameterValue=4,5,6",), + {"A": "2,2", "B": "1,2,3", "D": "4,5,6"}, + ), + ( + ("A=1 B=2 C=3"), + ("C=4",), + {"A": "1", "B": "2", "C": "4"}, + ), + ( + {"Flag": False}, + ("Flag=True",), + {"Flag": "True"} + ), + ( + ("A=1 B=2"), + "A=3 D=4", # String not tuple + {"A": "3", "D": "4"} + ), + ( + ("A=1 B=2 C=3"), + ["C=4"], # List not tuple + {"C": "4"}, + ), + ( + [[[[[[[[[[[[[[[[[[]]]]]]]]]]]], {"A": "1"}, [[[{"B": "2"}]]]]]]]]], + ("A=3",), + {"A": "3", "B": "2"}, + ), + ( + {"A": "1"}, + ("B=2",), + {"A": "1", "B": "2"}, + ), + ( + {"Spaces": "String with spaces"}, + ("Pam='Param Pam Pam Param'",), + {"Spaces": "String with spaces", "Pam": "Param Pam Pam Param"}, + ), + ( + [None, {"Empty": None}], + ("None=None",), + {"Empty": "", "None": "None"} + ), + ( + {"List": [" A ", 1, False, "", None]}, + ("OtherList='A,1,False'",), + {"List": "A,1,False", "OtherList": 'A,1,False'}, + ), + ( + {"List": [" A ", "' B '", 1, False, "", None]}, + ("List2=' A ,1,False'",), + {"List": "A,' B ',1,False", "List2": ' A ,1,False'}, + ), + ] + ) + def test_merge_default_map_parsing(self, file_overrides, cli_overrides, expected): + ctx = MagicMock() + ctx.default_map = {"parameter_overrides": file_overrides} + result = self.param_type.convert(cli_overrides, None, ctx) + self.assertEqual(result, expected, msg="Failed with Input = " + str(cli_overrides)) + + @parameterized.expand( + [ + ( + ("A=1 B=2"), + ("file://params.toml",), + {"A": "toml", "B": "toml", "Toml": "toml"}, + ), + ( + ("A=1 B=2"), + ("file://params.toml", "D=4"), + {"A": "toml", "B": "toml", "Toml": "toml", "D": "4"}, + ), + ( + ("A=1 B=2"), + ("ParameterKey=Y,ParameterValue=y", "file://params.toml", "D=4", "file://params.yaml", "A=6"), + {"A": "6", "B": "yaml", "Y": "y", "Toml": "toml", "D": "4", "Yaml": "yaml"}, + ), + ( + ("A=1 B=2"), + ("file://list.yaml",), + {"A": "a", "B": "2", "List": "1,2,3", "Yaml": "yaml"}, + ), + ( + ("A=1 B=2"), + ("file://nested.yaml",), + {"A": "yaml", "B": "yaml", "Toml": "toml", "Yaml": "yaml"}, + ), + ] + ) + def test_merge_file_parsing(self, file_overrides, cli_overrides, expected): + mock_files = { + "params.toml": 'A = "toml"\nB = "toml"\nToml = "toml"', + "params.yaml": "A: yaml\nB: yaml\nYaml: yaml", + "list.yaml": "- - - - - - A: a\n- List:\n - 1\n - 2\n - 3\n- Yaml: yaml", + "nested.yaml": "- file://params.toml\n- file://params.yaml", + } + + def mock_read_text(file_path): + file_name = file_path.name + return mock_files.get(file_name, "") + + def mock_is_file(file_path): + return file_path.name in mock_files + + with patch("pathlib.Path.is_file", new=mock_is_file), patch("pathlib.Path.read_text", new=mock_read_text): + ctx = MagicMock() + ctx.default_map = {"parameter_overrides": file_overrides} + + result = self.param_type.convert(cli_overrides, None, ctx) + print(result) + self.assertEqual(result, expected, msg="Failed with Input = " + str(cli_overrides)) + + + class TestCfnMetadataType(TestCase): + def setUp(self): self.param_type = CfnMetadataType() From 8b3782e07dde6c9225bdd9367bff5c322fb73444 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Sat, 8 Feb 2025 18:56:42 -0500 Subject: [PATCH 09/13] Replace |= with .updates() to support older Python --- samcli/cli/types.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 4aa29fda1c..36d1ab16f1 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -136,14 +136,14 @@ def _normalize_parameters(self, values, param, ctx): file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) if not file_manager: self.fail(f"{value} uses an unsupported extension", param, ctx) - parameters |= self._normalize_parameters(file_manager.read(filepath), param, ctx) + parameters.update(self._normalize_parameters(file_manager.read(filepath), param, ctx)) else: # Legacy parameter matching normalized_value = " " + value.strip() for pattern in self.ordered_pattern_match: groups = re.findall(pattern, normalized_value) if groups: - parameters |= groups + parameters.update(groups) break else: self.fail( From c2aaa39ce489aca9b711f7602881a8546d3b5f9f Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Mon, 10 Feb 2025 15:40:30 -0500 Subject: [PATCH 10/13] Remove merging samconfig and cli --- samcli/cli/types.py | 20 +------ tests/unit/cli/test_types.py | 108 ++--------------------------------- 2 files changed, 9 insertions(+), 119 deletions(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 36d1ab16f1..3bcc7e41ab 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -113,10 +113,9 @@ class CfnParameterOverridesType(click.ParamType): ordered_pattern_match = [_pattern_1, _pattern_2] - def _normalize_parameters(self, values, param, ctx): + def convert(self, values, param, ctx): """ - Normalizes parameter overrides into key-value pairs of strings - Later keys overwrite previous ones in case of key conflict + Normalizes different parameter overrides formats into key-value pairs of strings """ if values in (("",), "", None) or values == {}: LOG.debug("Empty parameter set (%s)", values) @@ -136,7 +135,7 @@ def _normalize_parameters(self, values, param, ctx): file_manager = FILE_MANAGER_MAPPER.get(filepath.suffix, None) if not file_manager: self.fail(f"{value} uses an unsupported extension", param, ctx) - parameters.update(self._normalize_parameters(file_manager.read(filepath), param, ctx)) + parameters.update(self.convert(file_manager.read(filepath), param, ctx)) else: # Legacy parameter matching normalized_value = " " + value.strip() @@ -177,19 +176,6 @@ def _normalize_parameters(self, values, param, ctx): LOG.debug("Output parameters: %s", result) return result - def convert(self, values, param, ctx): - # Merge samconfig with CLI parameter-overrides - if isinstance(values, tuple) and ctx: # ctx isn't set in all unit tests - # default_map was populated from samconfig - default_map = ctx.default_map.get("parameter_overrides", {}) - LOG.debug("Default map: %s", default_map) - LOG.debug("Current values: %s", values) - # Easy merge - will flatten later - values = [default_map] + [values] - result = self._normalize_parameters(values, param, ctx) - return result - - class CfnMetadataType(click.ParamType): """ Custom Click options type to accept values for metadata parameters. diff --git a/tests/unit/cli/test_types.py b/tests/unit/cli/test_types.py index 23e175fa2f..307f455495 100644 --- a/tests/unit/cli/test_types.py +++ b/tests/unit/cli/test_types.py @@ -152,126 +152,33 @@ def test_successful_parsing(self, input, expected): @parameterized.expand( [ - ("", "", {}, ), - (("A=1 A=2"), ("A=3 A=4",), {"A": "4"}), - (("Path=C:\\Windows\\System32"),("Path=/usr/bin",), {"Path": "/usr/bin"}), ( - ("A=1 B=2"), - ("ParameterKey=C,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), - {"A": "1", "B": "2", "C": "3", "D": "4"}, - ), - ( - ('Quoted="Hello, World!"'), - ('ParameterKey=Quoted,ParameterValue="\'Hi\'"',), - {"Quoted": "'Hi'"} - ), - ( - ("A=1 B=2"), - ("ParameterKey=A,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), - {"A": "3", "B": "2", "D": "4"}, - ), - ( - ["A=1", "B=2"], - ("ParameterKey=A,ParameterValue=3", "ParameterKey=D,ParameterValue=4",), - {"A": "3", "B": "2", "D": "4"}, - ), - ( - ("A=1,2,3 B=1,2,3"), - ("ParameterKey=A,ParameterValue=2,2", "ParameterKey=D,ParameterValue=4,5,6",), - {"A": "2,2", "B": "1,2,3", "D": "4,5,6"}, - ), - ( - ("A=1 B=2 C=3"), - ("C=4",), - {"A": "1", "B": "2", "C": "4"}, - ), - ( - {"Flag": False}, - ("Flag=True",), - {"Flag": "True"} - ), - ( - ("A=1 B=2"), - "A=3 D=4", # String not tuple - {"A": "3", "D": "4"} - ), - ( - ("A=1 B=2 C=3"), - ["C=4"], # List not tuple - {"C": "4"}, - ), - ( - [[[[[[[[[[[[[[[[[[]]]]]]]]]]]], {"A": "1"}, [[[{"B": "2"}]]]]]]]]], - ("A=3",), - {"A": "3", "B": "2"}, - ), - ( - {"A": "1"}, - ("B=2",), - {"A": "1", "B": "2"}, - ), - ( - {"Spaces": "String with spaces"}, - ("Pam='Param Pam Pam Param'",), - {"Spaces": "String with spaces", "Pam": "Param Pam Pam Param"}, - ), - ( - [None, {"Empty": None}], - ("None=None",), - {"Empty": "", "None": "None"} - ), - ( - {"List": [" A ", 1, False, "", None]}, - ("OtherList='A,1,False'",), - {"List": "A,1,False", "OtherList": 'A,1,False'}, - ), - ( - {"List": [" A ", "' B '", 1, False, "", None]}, - ("List2=' A ,1,False'",), - {"List": "A,' B ',1,False", "List2": ' A ,1,False'}, - ), - ] - ) - def test_merge_default_map_parsing(self, file_overrides, cli_overrides, expected): - ctx = MagicMock() - ctx.default_map = {"parameter_overrides": file_overrides} - result = self.param_type.convert(cli_overrides, None, ctx) - self.assertEqual(result, expected, msg="Failed with Input = " + str(cli_overrides)) - - @parameterized.expand( - [ - ( - ("A=1 B=2"), ("file://params.toml",), {"A": "toml", "B": "toml", "Toml": "toml"}, ), ( - ("A=1 B=2"), ("file://params.toml", "D=4"), {"A": "toml", "B": "toml", "Toml": "toml", "D": "4"}, ), ( - ("A=1 B=2"), ("ParameterKey=Y,ParameterValue=y", "file://params.toml", "D=4", "file://params.yaml", "A=6"), {"A": "6", "B": "yaml", "Y": "y", "Toml": "toml", "D": "4", "Yaml": "yaml"}, ), ( - ("A=1 B=2"), - ("file://list.yaml",), - {"A": "a", "B": "2", "List": "1,2,3", "Yaml": "yaml"}, + ("file://params.yaml", "file://list.yaml",), + {"A": "a", "B": "yaml", "List": "1,2,3", "Yaml": "yaml"}, ), ( - ("A=1 B=2"), ("file://nested.yaml",), {"A": "yaml", "B": "yaml", "Toml": "toml", "Yaml": "yaml"}, ), ] ) - def test_merge_file_parsing(self, file_overrides, cli_overrides, expected): + def test_merge_file_parsing(self, inputs, expected): mock_files = { "params.toml": 'A = "toml"\nB = "toml"\nToml = "toml"', "params.yaml": "A: yaml\nB: yaml\nYaml: yaml", - "list.yaml": "- - - - - - A: a\n- List:\n - 1\n - 2\n - 3\n- Yaml: yaml", + "list.yaml": "- - - - - - A: a\n- List:\n - 1\n - 2\n - 3\n", "nested.yaml": "- file://params.toml\n- file://params.yaml", } @@ -283,12 +190,9 @@ def mock_is_file(file_path): return file_path.name in mock_files with patch("pathlib.Path.is_file", new=mock_is_file), patch("pathlib.Path.read_text", new=mock_read_text): - ctx = MagicMock() - ctx.default_map = {"parameter_overrides": file_overrides} - - result = self.param_type.convert(cli_overrides, None, ctx) + result = self.param_type.convert(inputs, None, MagicMock()) print(result) - self.assertEqual(result, expected, msg="Failed with Input = " + str(cli_overrides)) + self.assertEqual(result, expected, msg="Failed with Input = " + str(inputs)) From 2152c8bc72d261243a2381f1739759d6e74d9dd6 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Tue, 18 Feb 2025 08:19:57 -0500 Subject: [PATCH 11/13] Fix type hint union --- samcli/cli/types.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 3bcc7e41ab..493dde784f 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -64,7 +64,7 @@ def _unquote_wrapped_quotes(value): return value.replace("\\ ", " ").replace('\\"', '"').replace("\\'", "'") -def _flatten_list(data: list | tuple) -> list: +def _flatten_list(data: Union[list, tuple]) -> list: """ Recursively flattens lists and other list-like types for easy sequential processing. This also helps with lists combined with YAML anchors & aliases. From b5f3f6b1f1cdedceebc50bb998f737ec98f3379f Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 19 Feb 2025 08:28:10 -0500 Subject: [PATCH 12/13] re-add accidental line deletion --- samcli/cli/types.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 493dde784f..6978c9f051 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -113,6 +113,8 @@ class CfnParameterOverridesType(click.ParamType): ordered_pattern_match = [_pattern_1, _pattern_2] + name = "string,list" + def convert(self, values, param, ctx): """ Normalizes different parameter overrides formats into key-value pairs of strings From af3c3e446f9ced7a3925104c78134471f6574e42 Mon Sep 17 00:00:00 2001 From: Joe Wakefield Date: Wed, 19 Feb 2025 13:17:28 -0500 Subject: [PATCH 13/13] Fix json schema and add tests --- samcli/cli/types.py | 3 +- schema/make_schema.py | 39 ++++++++++++- schema/samcli.json | 55 ++++++++++++++++--- tests/unit/cli/test_types.py | 1 - ...md_wrongtype_parameter_overrides_list.toml | 4 ++ ...md_wrongtype_parameter_overrides_list.yaml | 7 +++ .../parameter_overrides_features.yaml | 19 +++++++ .../schema/testdata/passing_tests/sample.toml | 4 ++ 8 files changed, 119 insertions(+), 13 deletions(-) create mode 100644 tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.toml create mode 100644 tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.yaml create mode 100644 tests/unit/schema/testdata/passing_tests/parameter_overrides_features.yaml diff --git a/samcli/cli/types.py b/samcli/cli/types.py index 6978c9f051..9d05d57940 100644 --- a/samcli/cli/types.py +++ b/samcli/cli/types.py @@ -113,7 +113,7 @@ class CfnParameterOverridesType(click.ParamType): ordered_pattern_match = [_pattern_1, _pattern_2] - name = "string,list" + name = "list,object,string" def convert(self, values, param, ctx): """ @@ -178,6 +178,7 @@ def convert(self, values, param, ctx): LOG.debug("Output parameters: %s", result) return result + class CfnMetadataType(click.ParamType): """ Custom Click options type to accept values for metadata parameters. diff --git a/schema/make_schema.py b/schema/make_schema.py index 8e1a8e1568..3905d2b2b3 100644 --- a/schema/make_schema.py +++ b/schema/make_schema.py @@ -45,7 +45,7 @@ class SamCliParameterSchema: type: Union[str, List[str]] description: str = "" default: Optional[Any] = None - items: Optional[str] = None + items: Optional[Union[str, Dict[str, str]]] = None choices: Optional[Any] = None def to_schema(self) -> Dict[str, Any]: @@ -55,7 +55,11 @@ def to_schema(self) -> Dict[str, Any]: if self.default: param.update({"default": self.default}) if self.items: - param.update({"items": {"type": self.items}}) + if isinstance(self.items, dict): + param.update({"items": self.items}) + else: + param.update({"items": {"type": self.items}}) + if self.choices: if isinstance(self.choices, list): self.choices.sort() @@ -143,11 +147,15 @@ def format_param(param: click.core.Option) -> SamCliParameterSchema: formatted_param_types.append(param_name or "string") formatted_param_types = sorted(list(set(formatted_param_types))) # deduplicate + # Allow nested arrays of objects and strings + parameter_overrides_ref = { "$ref": "#/$defs/parameter_overrides_items" } + formatted_param: SamCliParameterSchema = SamCliParameterSchema( param.name or "", formatted_param_types if len(formatted_param_types) > 1 else formatted_param_types[0], clean_text(param.help or ""), - items="string" if "array" in formatted_param_types else None, + items=parameter_overrides_ref if param.name == "parameter_overrides" + else "string" if "array" in formatted_param_types else None, ) if param.default and param.name not in PARAMS_TO_OMIT_DEFAULT_FIELD: @@ -231,6 +239,31 @@ def generate_schema() -> dict: } schema["required"] = ["version"] schema["additionalProperties"] = False + # Allows objects and strings beneath variably nested arrays + schema["$defs"]= { + "parameter_overrides_items": { + "anyOf": [ + { + "type": "array", + "items": { "$ref": "#/$defs/parameter_overrides_items" } + }, + { + "type": "object", + "additionalProperties": { + "anyOf": [ + { "type": "string" }, + { "type": "integer" }, # Allow cooler types if it's a dict + { "type": "boolean" }, + ] + }, + }, + { + "type": "string" + }, + ] + } + } + # Iterate through packages for command and parameter information for package_name in _SAM_CLI_COMMAND_PACKAGES: commands.extend(retrieve_command_structure(package_name)) diff --git a/schema/samcli.json b/schema/samcli.json index 143f7f3031..4a012ff3f0 100644 --- a/schema/samcli.json +++ b/schema/samcli.json @@ -13,6 +13,37 @@ "version" ], "additionalProperties": false, + "$defs": { + "parameter_overrides_items": { + "anyOf": [ + { + "type": "array", + "items": { + "$ref": "#/$defs/parameter_overrides_items" + } + }, + { + "type": "object", + "additionalProperties": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "integer" + }, + { + "type": "boolean" + } + ] + } + }, + { + "type": "string" + } + ] + } + }, "patternProperties": { "^.+$": { "title": "Environment", @@ -346,11 +377,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "skip_pull_image": { @@ -445,11 +477,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "debug_port": { @@ -636,11 +669,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "debug_port": { @@ -835,11 +869,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "debug_port": { @@ -1225,11 +1260,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "signing_profiles": { @@ -1699,11 +1735,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "beta_features": { @@ -1982,11 +2019,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "stack_name": { @@ -2111,11 +2149,12 @@ "title": "parameter_overrides", "type": [ "array", + "object", "string" ], "description": "String that contains AWS CloudFormation parameter overrides encoded as key=value pairs.", "items": { - "type": "string" + "$ref": "#/$defs/parameter_overrides_items" } }, "stack_name": { diff --git a/tests/unit/cli/test_types.py b/tests/unit/cli/test_types.py index 307f455495..bd7ffabfa8 100644 --- a/tests/unit/cli/test_types.py +++ b/tests/unit/cli/test_types.py @@ -195,7 +195,6 @@ def mock_is_file(file_path): self.assertEqual(result, expected, msg="Failed with Input = " + str(inputs)) - class TestCfnMetadataType(TestCase): def setUp(self): diff --git a/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.toml b/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.toml new file mode 100644 index 0000000000..64e3a4805d --- /dev/null +++ b/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.toml @@ -0,0 +1,4 @@ +version = 0.1 + +[default.build.parameters] +parameter_overrides = [ 10 ] \ No newline at end of file diff --git a/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.yaml b/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.yaml new file mode 100644 index 0000000000..7d477dd685 --- /dev/null +++ b/tests/unit/schema/testdata/failing_tests/buildcmd_wrongtype_parameter_overrides_list.yaml @@ -0,0 +1,7 @@ +version: 0.1 + +default: + build: + parameters: + parameter_overrides: + - 10 \ No newline at end of file diff --git a/tests/unit/schema/testdata/passing_tests/parameter_overrides_features.yaml b/tests/unit/schema/testdata/passing_tests/parameter_overrides_features.yaml new file mode 100644 index 0000000000..ede1025358 --- /dev/null +++ b/tests/unit/schema/testdata/passing_tests/parameter_overrides_features.yaml @@ -0,0 +1,19 @@ +version: 0.1 +prod-a: + deploy: + parameters: + stack_name: sam-app + region: us-east-1 + parameter_overrides: &prod_a_overrides + - Name: prod-a + - ParamA: 1 + - ParamB: True + +prod-b: + deploy: + parameters: + stack_name: sam-app-2 + region: us-east-1 + parameter_overrides: + - *prod_a_overrides + - Name: prod-b \ No newline at end of file diff --git a/tests/unit/schema/testdata/passing_tests/sample.toml b/tests/unit/schema/testdata/passing_tests/sample.toml index 4b95a2c644..03726773f1 100644 --- a/tests/unit/schema/testdata/passing_tests/sample.toml +++ b/tests/unit/schema/testdata/passing_tests/sample.toml @@ -14,6 +14,10 @@ capabilities = "CAPABILITY_IAM" confirm_changeset = true resolve_s3 = true +[default.deploy.parameters.parameter_overrides] +A = "foo" +B = "bar" + [default.sync.parameters] watch = true