From 1fb48771e8042a5587ddbfa8e8cb04a6a7899979 Mon Sep 17 00:00:00 2001 From: Kevin DeJong Date: Mon, 30 Dec 2024 14:36:25 -0800 Subject: [PATCH] More additions to testing --- src/cfnlint/config.py | 34 ++--- .../rules/deployment_files/Parameters.py | 27 ++-- src/cfnlint/runner/__init__.py | 2 +- .../runner/deployment_file/__init__.py | 4 +- src/cfnlint/runner/deployment_file/runner.py | 2 +- src/cfnlint/runner/template/runner.py | 6 +- test/unit/module/config/test_config_mixin.py | 37 +++++ .../runner/deployment_file/test_runner.py | 138 ++++++++++++------ .../template/test_run_template_by_data.py | 65 +++++++++ .../rules/deployment_files/test_parameters.py | 52 +++++++ 10 files changed, 273 insertions(+), 94 deletions(-) create mode 100644 test/unit/module/runner/template/test_run_template_by_data.py diff --git a/src/cfnlint/config.py b/src/cfnlint/config.py index 63db113e63..74d45d5e41 100644 --- a/src/cfnlint/config.py +++ b/src/cfnlint/config.py @@ -250,17 +250,6 @@ def comma_separated_arg(string): return string.split(",") -class key_value(argparse.Action): - def __call__(self, parser, namespace, values, option_string=None): - setattr(namespace, self.dest, dict()) - - for value in values: - # split it into key and value - key, value = value.split("=", 1) - # assign into dictionary - getattr(namespace, self.dest)[key.strip()] = value.strip() - - def _ensure_value(namespace, name, value): if getattr(namespace, name, None) is None: setattr(namespace, name, value) @@ -324,7 +313,7 @@ def __call__(self, parser, namespace, values, option_string=None): setattr(namespace, self.dest, items) except Exception: # pylint: disable=W0703 parser.print_help() - parser.exit() + parser.exit(1) class ExtendKeyValuePairs(argparse.Action): @@ -366,7 +355,7 @@ def __call__(self, parser, namespace, values, option_string=None): setattr(namespace, self.dest, result) except Exception: # pylint: disable=W0703 parser.print_help() - parser.exit() + parser.exit(1) class ExtendAction(argparse.Action): @@ -460,9 +449,8 @@ def error(self, message): action="extend", ) parameter_group.add_argument( - "-tp", - "--template-parameters", - dest="template_parameters", + "--parameters", + dest="parameters", nargs="+", default=[], action="extend_key_value", @@ -693,7 +681,7 @@ class ManualArgs(TypedDict, total=False): non_zero_exit_code: str output_file: str regions: list - template_parameters: list[dict[str, Any]] + parameters: list[dict[str, Any]] # pylint: disable=too-many-public-methods @@ -730,7 +718,7 @@ def __repr__(self): "non_zero_exit_code": self.non_zero_exit_code, "override_spec": self.override_spec, "regions": self.regions, - "template_parameters": self.template_parameters, + "parameters": self.parameters, "templates": self.templates, } ) @@ -890,12 +878,12 @@ def append_rules(self): ) @property - def template_parameters(self): - return self._get_argument_value("template_parameters", True, True) + def parameters(self): + return self._get_argument_value("parameters", True, True) - @template_parameters.setter - def template_parameters(self, template_parameters: list[dict[str, Any]]): - self._manual_args["template_parameters"] = template_parameters + @parameters.setter + def parameters(self, parameters: list[dict[str, Any]]): + self._manual_args["parameters"] = parameters @property def override_spec(self): diff --git a/src/cfnlint/rules/deployment_files/Parameters.py b/src/cfnlint/rules/deployment_files/Parameters.py index dc870c52af..596c52a9f1 100644 --- a/src/cfnlint/rules/deployment_files/Parameters.py +++ b/src/cfnlint/rules/deployment_files/Parameters.py @@ -32,7 +32,7 @@ def __init__(self): ) def _is_type_a_list(self, parameter_type: str) -> bool: - return "List" in parameter_type and "CommaDelimitedList" not in parameter_type + return "List" in parameter_type def _build_schema(self, instance: Any) -> dict[str, Any]: if not isinstance(instance, dict): @@ -58,6 +58,12 @@ def _build_schema(self, instance: Any) -> dict[str, Any]: if not isinstance(parameter_type, str): continue + schema_constraints = {} + if "AllowedValues" in parameter_object: + schema_constraints["enum"] = parameter_object["AllowedValues"] + if "Pattern" in parameter_object: + schema_constraints["pattern"] = parameter_object["Pattern"] + if self._is_type_a_list(parameter_type): schema["properties"][parameter_name] = { "type": "array", @@ -65,25 +71,10 @@ def _build_schema(self, instance: Any) -> dict[str, Any]: "type": singular_types, }, } - if "AllowedValues" in parameter_object: - schema["properties"][parameter_name]["items"]["enum"] = ( - parameter_object["AllowedValues"] - ) - if "Pattern" in parameter_object: - if self._is_type_a_list(parameter_type): - schema["properties"][parameter_name]["items"]["pattern"] = ( - parameter_object["Pattern"] - ) + schema["properties"][parameter_name]["items"].update(schema_constraints) else: schema["properties"][parameter_name]["type"] = singular_types - if "AllowedValues" in parameter_object: - schema["properties"][parameter_name]["enum"] = parameter_object[ - "AllowedValues" - ] - if "Pattern" in parameter_object: - schema["properties"][parameter_name]["pattern"] = parameter_object[ - "Pattern" - ] + schema["properties"][parameter_name].update(schema_constraints) return schema diff --git a/src/cfnlint/runner/__init__.py b/src/cfnlint/runner/__init__.py index 7d5635920f..ea1a996f4c 100644 --- a/src/cfnlint/runner/__init__.py +++ b/src/cfnlint/runner/__init__.py @@ -15,7 +15,7 @@ ] from cfnlint.runner.cli import Runner, main -from cfnlint.runner.deployment_file import run_deployment_file +from cfnlint.runner.deployment_file import run_deployment_files from cfnlint.runner.exceptions import ( CfnLintExitException, InvalidRegionException, diff --git a/src/cfnlint/runner/deployment_file/__init__.py b/src/cfnlint/runner/deployment_file/__init__.py index e0811bcbd6..e9d82a7326 100644 --- a/src/cfnlint/runner/deployment_file/__init__.py +++ b/src/cfnlint/runner/deployment_file/__init__.py @@ -3,6 +3,6 @@ SPDX-License-Identifier: MIT-0 """ -__all__ = ["run_deployment_file"] +__all__ = ["run_deployment_files"] -from cfnlint.runner.deployment_file.runner import run_deployment_file +from cfnlint.runner.deployment_file.runner import run_deployment_files diff --git a/src/cfnlint/runner/deployment_file/runner.py b/src/cfnlint/runner/deployment_file/runner.py index 3d296fdcab..a65101c56b 100644 --- a/src/cfnlint/runner/deployment_file/runner.py +++ b/src/cfnlint/runner/deployment_file/runner.py @@ -68,7 +68,7 @@ def run_deployment_file( ) template_path = Path(filename).parent / deployment_data.template_file_path template_config = deepcopy(config) - template_config.template_parameters = [deployment_data.parameters] + template_config.parameters = [deployment_data.parameters] yield from run_template_by_file_path( filename=template_path, diff --git a/src/cfnlint/runner/template/runner.py b/src/cfnlint/runner/template/runner.py index 666236c7e1..1155bb886b 100644 --- a/src/cfnlint/runner/template/runner.py +++ b/src/cfnlint/runner/template/runner.py @@ -107,10 +107,10 @@ def _run_template( ) -> Iterator[Match]: config.set_template_args(template) - if config.template_parameters: + if config.parameters: matches: list[Match] = [] - for template_parameters in config.template_parameters: - cfn = Template(filename, template, config.regions, template_parameters) + for parameters in config.parameters: + cfn = Template(filename, template, config.regions, parameters) matches.extend(list(_run_template_per_config(cfn, config, rules))) yield from _dedup(iter(matches)) else: diff --git a/test/unit/module/config/test_config_mixin.py b/test/unit/module/config/test_config_mixin.py index 9e584ead07..22cff293a7 100644 --- a/test/unit/module/config/test_config_mixin.py +++ b/test/unit/module/config/test_config_mixin.py @@ -277,3 +277,40 @@ def test_config_merge(self, yaml_mock): ) # template file wins over config file self.assertEqual(config.ignore_checks, ["W3001", "E3001"]) + + @patch("cfnlint.config.ConfigFileArgs._read_config", create=True) + def test_parameters(self, yaml_mock): + yaml_mock.side_effect = [{}, {}] + config = cfnlint.config.ConfigMixIn(["--parameters", "Foo=Bar"]) + + # test defaults + self.assertEqual(config.parameters, [{"Foo": "Bar"}]) + + @patch("cfnlint.config.ConfigFileArgs._read_config", create=True) + def test_parameters_lists(self, yaml_mock): + yaml_mock.side_effect = [{}, {}] + config = cfnlint.config.ConfigMixIn(["--parameters", "A=1", "B=2"]) + + # test defaults + self.assertEqual(config.parameters, [{"A": "1", "B": "2"}]) + + @patch("cfnlint.config.ConfigFileArgs._read_config", create=True) + def test_parameters_lists_bad_value(self, yaml_mock): + yaml_mock.side_effect = [{}, {}] + + with patch("sys.exit") as exit: + cfnlint.config.ConfigMixIn( + [ + "--parameters", + "A", + ] + ) + exit.assert_called_once_with(1) + + @patch("cfnlint.config.ConfigFileArgs._read_config", create=True) + def test_template_files(self, yaml_mock): + yaml_mock.side_effect = [{}, {}] + config = cfnlint.config.ConfigMixIn(["--deployment-files", "file1.json"]) + + # test defaults + self.assertEqual(config.deployment_files, ["file1.json"]) diff --git a/test/unit/module/runner/deployment_file/test_runner.py b/test/unit/module/runner/deployment_file/test_runner.py index c33701fe96..6f317c16b6 100644 --- a/test/unit/module/runner/deployment_file/test_runner.py +++ b/test/unit/module/runner/deployment_file/test_runner.py @@ -11,49 +11,56 @@ from cfnlint.config import ConfigMixIn from cfnlint.rules import Match from cfnlint.rules.deployment_files.Configuration import Configuration -from cfnlint.runner.deployment_file import run_deployment_file +from cfnlint.runner.deployment_file import run_deployment_files _filename = "deployment-file.yaml" @pytest.mark.parametrize( - "name, decode, validate_template_parameters, validate_template_return, expected", + ( + "name,deployment_files,validate_template_parameters," + "validate_template_return,expected" + ), [ ( "A standard git sync file", - ( - { - "template-file-path": "../a/path", - "parameters": { - "Foo": "Bar", - }, - "tags": { - "Key": "Value", + { + _filename: ( + { + "template-file-path": "../a/path", + "parameters": { + "Foo": "Bar", + }, + "tags": { + "Key": "Value", + }, }, - }, - [], - ), + [], + ), + }, { "filename": Path("../a/path"), - "template_parameters": [{"Foo": "Bar"}], + "parameters": [{"Foo": "Bar"}], }, [], [], ), ( "Bad template-file-path type", - ( - { - "template-file-path": ["../a/path"], - "parameters": { - "Foo": "Bar", - }, - "tags": { - "Key": "Value", + { + _filename: ( + { + "template-file-path": ["../a/path"], + "parameters": { + "Foo": "Bar", + }, + "tags": { + "Key": "Value", + }, }, - }, - [], - ), + [], + ) + }, {}, [], [ @@ -70,21 +77,23 @@ ), ( "Bad template-file-path type", - ( - { - "template-file-path": "../a/path", - "parameters": { - "Foo": "Bar", - }, - "tags": { - "Key": "Value", + { + _filename: ( + { + "template-file-path": "../a/path", + "parameters": { + "Foo": "Bar", + }, + "tags": { + "Key": "Value", + }, }, - }, - [], - ), + [], + ), + }, { "filename": Path("../a/path"), - "template_parameters": [{"Foo": "Bar"}], + "parameters": [{"Foo": "Bar"}], }, iter( [ @@ -111,30 +120,67 @@ ) ], ), + ( + "Bad decode", + { + _filename: ( + {}, + [ + Match( + linenumber=1, + columnnumber=1, + linenumberend=1, + columnnumberend=1, + filename=_filename, + message=f"Deployment file {_filename!r} is not supported", + rule=Configuration(), + ) + ], + ), + }, + {}, + None, + [ + Match( + linenumber=1, + columnnumber=1, + linenumberend=1, + columnnumberend=1, + filename=_filename, + message=f"Deployment file {_filename!r} is not supported", + rule=Configuration(), + ) + ], + ), ], ) def test_runner( - name, decode, validate_template_parameters, validate_template_return, expected + name, + deployment_files, + validate_template_parameters, + validate_template_return, + expected, ): - with patch( - "cfnlint.runner.deployment_file.runner.decode", return_value=decode - ) as mock_decode: + decode_results = [v for _, v in deployment_files.items()] + deployment_files = {k for k, _ in deployment_files.items()} + with patch("cfnlint.runner.deployment_file.runner.decode") as mock_decode: + mock_decode.side_effect = decode_results with patch( "cfnlint.runner.deployment_file.runner.run_template_by_file_path", return_value=validate_template_return, ) as mock_run_template_by_file_path: - deployment = list(run_deployment_file(_filename, ConfigMixIn(), None)) + config = ConfigMixIn([], deployment_files=deployment_files) + deployment = list(run_deployment_files(config, None)) - mock_decode.assert_called_once() + for deployment_file in deployment_files: + mock_decode.assert_called_with(deployment_file) if validate_template_parameters: mock_run_template_by_file_path.assert_called_once() config = mock_run_template_by_file_path.call_args_list assert config[0].kwargs.get( "config" - ).template_parameters == validate_template_parameters.get( - "template_parameters" - ) + ).parameters == validate_template_parameters.get("parameters") assert config[0].kwargs.get( "filename" ) == validate_template_parameters.get("filename") diff --git a/test/unit/module/runner/template/test_run_template_by_data.py b/test/unit/module/runner/template/test_run_template_by_data.py new file mode 100644 index 0000000000..8c2c8664bb --- /dev/null +++ b/test/unit/module/runner/template/test_run_template_by_data.py @@ -0,0 +1,65 @@ +""" +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +SPDX-License-Identifier: MIT-0 +""" + +from unittest.mock import patch + +import pytest + +from cfnlint.config import ConfigMixIn +from cfnlint.rules import RulesCollection +from cfnlint.runner.template import run_template_by_data + + +@pytest.mark.parametrize( + "name, config, runner_results, expected_parameters", + [ + ( + "A standard template", + ConfigMixIn(), + [ + iter([]), + ], + [None], + ), + ( + "One set of parameters", + ConfigMixIn(parameters=[{"Foo": "Bar"}]), + [ + iter([]), + ], + [{"Foo": "Bar"}], + ), + ( + "Multiple parameters", + ConfigMixIn(parameters=[{"A": "B"}, {"C": "D"}]), + [ + iter([]), + iter([]), + ], + [ + {"A": "B"}, + {"C": "D"}, + ], + ), + ], +) +def test_runner( + name, + config, + runner_results, + expected_parameters, +): + + with patch( + "cfnlint.rules._rules.RulesCollection.run", side_effect=runner_results + ) as mock_run: + list(run_template_by_data({}, config, RulesCollection())) + + calls = mock_run.call_args_list + for index, call in enumerate(calls): + assert call.kwargs["cfn"].parameters == expected_parameters[index], ( + f"{name}: {call.kwargs['cfn'].parameters} " + f"!= {expected_parameters[index]}" + ) diff --git a/test/unit/rules/deployment_files/test_parameters.py b/test/unit/rules/deployment_files/test_parameters.py index 2d2546695c..27b2677bed 100644 --- a/test/unit/rules/deployment_files/test_parameters.py +++ b/test/unit/rules/deployment_files/test_parameters.py @@ -82,6 +82,58 @@ def rule(): ) ], ), + ( + "Okay with a list", + {"Foo": {"Type": "CommaDelimitedList"}}, + {"Foo": ["D"]}, + [], + ), + ( + "Not okay with a list and a bad pattern", + {"Foo": {"Type": "CommaDelimitedList", "Pattern": "^Bar$"}}, + {"Foo": ["Bar", "D"]}, + [ + ValidationError( + "'D' does not match '^Bar$'", + path=deque(["Foo", 1]), + validator="pattern", + schema_path=deque(["properties", "Foo", "items", "pattern"]), + rule=Parameters(), + ) + ], + ), + ( + "Not okay with a list and an enum", + {"Foo": {"Type": "CommaDelimitedList", "AllowedValues": ["Bar"]}}, + {"Foo": ["Bar", "D"]}, + [ + ValidationError( + "'D' is not one of ['Bar']", + path=deque(["Foo", 1]), + validator="enum", + schema_path=deque(["properties", "Foo", "items", "enum"]), + rule=Parameters(), + ) + ], + ), + ( + "Issues when a bad properties type", + [{"Foo": {"Type": "CommaDelimitedList", "AllowedValues": ["Bar"]}}], + {"Foo": "Bar"}, + [], + ), + ( + "Issues when a bad property type", + {"Foo": [{"Type": "CommaDelimitedList", "AllowedValues": ["Bar"]}]}, + {"Foo": "Bar"}, + [], + ), + ( + "Issues when a bad type", + {"Foo": {"Type": ["String"], "AllowedValues": ["Bar"]}}, + {"Foo": "Foo"}, + [], + ), ], indirect=["parameters"], )