From 9128c4504b8389049508be8c6dcb10694b4a4454 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Fri, 31 May 2024 17:58:00 +0100 Subject: [PATCH] Refactor task normalization Fixes: #4177 --- .github/workflows/tox.yml | 2 +- examples/playbooks/conflicting_action2.yml | 9 ++++ src/ansiblelint/runner.py | 10 ++-- src/ansiblelint/utils.py | 58 +++++++++++----------- test/rules/test_syntax_check.py | 51 +++++++++++++++---- test/test_utils.py | 53 ++++++++++++-------- 6 files changed, 117 insertions(+), 66 deletions(-) create mode 100644 examples/playbooks/conflicting_action2.yml diff --git a/.github/workflows/tox.yml b/.github/workflows/tox.yml index c2c36858cd..188d15516c 100644 --- a/.github/workflows/tox.yml +++ b/.github/workflows/tox.yml @@ -72,7 +72,7 @@ jobs: env: # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. - PYTEST_REQPASS: 871 + PYTEST_REQPASS: 872 steps: - uses: actions/checkout@v4 with: diff --git a/examples/playbooks/conflicting_action2.yml b/examples/playbooks/conflicting_action2.yml new file mode 100644 index 0000000000..380857d4fe --- /dev/null +++ b/examples/playbooks/conflicting_action2.yml @@ -0,0 +1,9 @@ +--- +- hosts: localhost + gather_facts: false + tasks: + - block: + include_role: + tasks_from: ghe-config-apply.yml + tags: + - github diff --git a/src/ansiblelint/runner.py b/src/ansiblelint/runner.py index efe6e8e1a8..a874d4b40a 100644 --- a/src/ansiblelint/runner.py +++ b/src/ansiblelint/runner.py @@ -499,7 +499,7 @@ def find_children(self, lintable: Lintable) -> list[Lintable]: for item in ansiblelint.utils.playbook_items(playbook_ds): # if lintable.kind not in ["playbook"]: for child in self.play_children( - lintable.path.parent, + lintable, item, lintable.kind, playbook_dir, @@ -525,19 +525,19 @@ def find_children(self, lintable: Lintable) -> list[Lintable]: def play_children( self, - basedir: Path, + lintable: Lintable, item: tuple[str, Any], parent_type: FileType, playbook_dir: str, ) -> list[Lintable]: """Flatten the traversed play tasks.""" # pylint: disable=unused-argument - + basedir = lintable.path.parent handlers = HandleChildren(self.rules, app=self.app) delegate_map: dict[ str, - Callable[[str, Any, Any, FileType], list[Lintable]], + Callable[[Lintable, Any, Any, FileType], list[Lintable]], ] = { "tasks": handlers.taskshandlers_children, "pre_tasks": handlers.taskshandlers_children, @@ -565,7 +565,7 @@ def play_children( {"playbook_dir": PLAYBOOK_DIR or str(basedir.resolve())}, fail_on_undefined=False, ) - return delegate_map[k](str(basedir), k, v, parent_type) + return delegate_map[k](lintable, k, v, parent_type) return [] def plugin_children(self, lintable: Lintable) -> list[Lintable]: diff --git a/src/ansiblelint/utils.py b/src/ansiblelint/utils.py index df8aa2d432..1838d3b81c 100644 --- a/src/ansiblelint/utils.py +++ b/src/ansiblelint/utils.py @@ -287,12 +287,13 @@ class HandleChildren: def include_children( self, - basedir: str, + lintable: Lintable, k: str, v: Any, parent_type: FileType, ) -> list[Lintable]: """Include children.""" + basedir = str(lintable.path.parent) # import_playbook only accepts a string as argument (no dict syntax) if k in ( "import_playbook", @@ -332,12 +333,13 @@ def include_children( def taskshandlers_children( self, - basedir: str, + lintable: Lintable, k: str, v: None | Any, parent_type: FileType, ) -> list[Lintable]: """TasksHandlers Children.""" + basedir = str(lintable.path.parent) results: list[Lintable] = [] if v is None or isinstance(v, int | str): raise MatchError( @@ -360,14 +362,16 @@ def taskshandlers_children( continue if any(x in task_handler for x in ROLE_IMPORT_ACTION_NAMES): - task_handler = normalize_task_v2(task_handler) + task_handler = normalize_task_v2( + Task(task_handler, filename=str(lintable.path)), + ) self._validate_task_handler_action_for_role(task_handler["action"]) name = task_handler["action"].get("name") if has_jinja(name): # we cannot deal with dynamic imports continue results.extend( - self.roles_children(basedir, k, [name], parent_type), + self.roles_children(lintable, k, [name], parent_type), ) continue @@ -378,7 +382,7 @@ def taskshandlers_children( if elem in task_handler: results.extend( self.taskshandlers_children( - basedir, + lintable, k, task_handler[elem], parent_type, @@ -410,13 +414,14 @@ def _validate_task_handler_action_for_role(self, th_action: dict[str, Any]) -> N def roles_children( self, - basedir: str, + lintable: Lintable, k: str, v: Sequence[Any], parent_type: FileType, ) -> list[Lintable]: """Roles children.""" # pylint: disable=unused-argument # parent_type) + basedir = str(lintable.path.parent) results: list[Lintable] = [] if not v: # typing does not prevent junk from being passed in @@ -549,13 +554,14 @@ def _extract_ansible_parsed_keys_from_task( return result -def normalize_task_v2(task: dict[str, Any]) -> dict[str, Any]: +def normalize_task_v2(task: Task) -> dict[str, Any]: """Ensure tasks have a normalized action key and strings are converted to python objects.""" + raw_task = task.raw_task result: dict[str, Any] = {} ansible_parsed_keys = ("action", "local_action", "args", "delegate_to") - if is_nested_task(task): - _extract_ansible_parsed_keys_from_task(result, task, ansible_parsed_keys) + if is_nested_task(raw_task): + _extract_ansible_parsed_keys_from_task(result, raw_task, ansible_parsed_keys) # Add dummy action for block/always/rescue statements result["action"] = { "__ansible_module__": "block/always/rescue", @@ -564,7 +570,7 @@ def normalize_task_v2(task: dict[str, Any]) -> dict[str, Any]: return result - sanitized_task = _sanitize_task(task) + sanitized_task = _sanitize_task(raw_task) mod_arg_parser = ModuleArgsParser(sanitized_task) try: @@ -575,8 +581,8 @@ def normalize_task_v2(task: dict[str, Any]) -> dict[str, Any]: raise MatchError( rule=AnsibleParserErrorRule(), message=exc.message, - filename=task.get(FILENAME_KEY, "Unknown"), - lineno=task.get(LINE_NUMBER_KEY, 0), + filename=task.filename or "Unknown", + lineno=raw_task.get(LINE_NUMBER_KEY, 1), ) from exc # denormalize shell -> command conversion @@ -586,7 +592,7 @@ def normalize_task_v2(task: dict[str, Any]) -> dict[str, Any]: _extract_ansible_parsed_keys_from_task( result, - task, + raw_task, (*ansible_parsed_keys, action), ) @@ -608,17 +614,6 @@ def normalize_task_v2(task: dict[str, Any]) -> dict[str, Any]: return result -def normalize_task(task: dict[str, Any], filename: str) -> dict[str, Any]: - """Unify task-like object structures.""" - ansible_action_type = task.get("__ansible_action_type__", "task") - if "__ansible_action_type__" in task: - del task["__ansible_action_type__"] - task = normalize_task_v2(task) - task[FILENAME_KEY] = filename - task["__ansible_action_type__"] = ansible_action_type - return task - - def task_to_str(task: dict[str, Any]) -> str: """Make a string identifier for the given task.""" name = task.get("name") @@ -742,10 +737,7 @@ def normalized_task(self) -> dict[str, Any]: """Return the name of the task.""" if not hasattr(self, "_normalized_task"): try: - self._normalized_task = normalize_task( - self.raw_task, - filename=self.filename, - ) + self._normalized_task = self._normalize_task() except MatchError as err: self.error = err # When we cannot normalize it, we just use the raw task instead @@ -756,6 +748,16 @@ def normalized_task(self) -> dict[str, Any]: raise TypeError(msg) return self._normalized_task + def _normalize_task(self) -> dict[str, Any]: + """Unify task-like object structures.""" + ansible_action_type = self.raw_task.get("__ansible_action_type__", "task") + if "__ansible_action_type__" in self.raw_task: + del self.raw_task["__ansible_action_type__"] + task = normalize_task_v2(self) + task[FILENAME_KEY] = self.filename + task["__ansible_action_type__"] = ansible_action_type + return task + @property def skip_tags(self) -> list[str]: """Return the list of tags to skip.""" diff --git a/test/rules/test_syntax_check.py b/test/rules/test_syntax_check.py index 7d9dc0661c..6ec111de96 100644 --- a/test/rules/test_syntax_check.py +++ b/test/rules/test_syntax_check.py @@ -2,30 +2,61 @@ from typing import Any +import pytest + from ansiblelint.file_utils import Lintable from ansiblelint.rules import RulesCollection from ansiblelint.runner import Runner +@pytest.mark.parametrize( + ("filename", "expected_results"), + ( + pytest.param( + "examples/playbooks/conflicting_action.yml", + [ + ( + "syntax-check[specific]", + 4, + 7, + "conflicting action statements: ansible.builtin.debug, ansible.builtin.command", + ), + ], + id="0", + ), + pytest.param( + "examples/playbooks/conflicting_action2.yml", + [ + ( + "parser-error", + 1, + None, + "conflicting action statements: block, include_role", + ), + ( + "syntax-check[specific]", + 5, + 7, + "'include_role' is not a valid attribute for a Block", + ), + ], + id="1", + ), + ), +) def test_get_ansible_syntax_check_matches( default_rules_collection: RulesCollection, + filename: str, + expected_results: list[tuple[str, int, int, str]], ) -> None: """Validate parsing of ansible output.""" lintable = Lintable( - "examples/playbooks/conflicting_action.yml", + filename, kind="playbook", ) result = sorted(Runner(lintable, rules=default_rules_collection).run()) - expected_results = [ - [ - "syntax-check[specific]", - 4, - 7, - "conflicting action statements: ansible.builtin.debug, ansible.builtin.command", - ], - ] assert len(result) == len(expected_results) for index, expected in enumerate(expected_results): assert result[index].tag == expected[0] @@ -34,7 +65,7 @@ def test_get_ansible_syntax_check_matches( assert str(expected[3]) in result[index].message # We internally convert absolute paths returned by ansible into paths # relative to current directory. - assert result[index].filename.endswith("/conflicting_action.yml") + # assert result[index].filename.endswith("/conflicting_action.yml") def test_empty_playbook(default_rules_collection: RulesCollection) -> None: diff --git a/test/test_utils.py b/test/test_utils.py index 0d8afcdc19..9010205c67 100644 --- a/test/test_utils.py +++ b/test/test_utils.py @@ -114,37 +114,42 @@ def test_normalize( alternate_forms: tuple[dict[str, Any]], ) -> None: """Test that tasks specified differently are normalized same way.""" - normal_form = utils.normalize_task(reference_form, "tasks.yml") + task = utils.Task(reference_form, filename="tasks.yml") + normal_form = task._normalize_task() # noqa: SLF001 for form in alternate_forms: - assert normal_form == utils.normalize_task(form, "tasks.yml") + task2 = utils.Task(form, filename="tasks.yml") + assert normal_form == task2._normalize_task() # noqa: SLF001 def test_normalize_complex_command() -> None: """Test that tasks specified differently are normalized same way.""" - task1 = { - "name": "hello", - "action": {"module": "pip", "name": "df", "editable": "false"}, - } - task2 = {"name": "hello", "pip": {"name": "df", "editable": "false"}} - task3 = {"name": "hello", "pip": "name=df editable=false"} - task4 = {"name": "hello", "action": "pip name=df editable=false"} - assert utils.normalize_task(task1, "tasks.yml") == utils.normalize_task( - task2, - "tasks.yml", + task1 = utils.Task( + { + "name": "hello", + "action": {"module": "pip", "name": "df", "editable": "false"}, + }, + filename="tasks.yml", + ) + task2 = utils.Task( + {"name": "hello", "pip": {"name": "df", "editable": "false"}}, + filename="tasks.yml", ) - assert utils.normalize_task(task2, "tasks.yml") == utils.normalize_task( - task3, - "tasks.yml", + task3 = utils.Task( + {"name": "hello", "pip": "name=df editable=false"}, + filename="tasks.yml", ) - assert utils.normalize_task(task3, "tasks.yml") == utils.normalize_task( - task4, - "tasks.yml", + task4 = utils.Task( + {"name": "hello", "action": "pip name=df editable=false"}, + filename="tasks.yml", ) + assert task1._normalize_task() == task2._normalize_task() # noqa: SLF001 + assert task2._normalize_task() == task3._normalize_task() # noqa: SLF001 + assert task3._normalize_task() == task4._normalize_task() # noqa: SLF001 @pytest.mark.parametrize( - ("task", "expected_form"), + ("task_raw", "expected_form"), ( pytest.param( { @@ -192,8 +197,12 @@ def test_normalize_complex_command() -> None: ), ), ) -def test_normalize_task_v2(task: dict[str, Any], expected_form: dict[str, Any]) -> None: +def test_normalize_task_v2( + task_raw: dict[str, Any], + expected_form: dict[str, Any], +) -> None: """Check that it normalizes task and returns the expected form.""" + task = utils.Task(task_raw) assert utils.normalize_task_v2(task) == expected_form @@ -263,8 +272,8 @@ def test_template(template: str, output: str) -> None: def test_task_to_str_unicode() -> None: """Ensure that extracting messages from tasks preserves Unicode.""" - task = {"fail": {"msg": "unicode é ô à"}} - result = utils.task_to_str(utils.normalize_task(task, "filename.yml")) + task = utils.Task({"fail": {"msg": "unicode é ô à"}}, filename="filename.yml") + result = utils.task_to_str(task._normalize_task()) # noqa: SLF001 assert result == "fail msg=unicode é ô à"