From 232db7908da39a5e8d7a4a45cd8493aa7d3a5c80 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Fri, 3 May 2024 16:50:10 +0100 Subject: [PATCH] Run entire linting regardless if syntax-check errors are found This is a notable change in behavior as from now syntax-errors will no longer prevent the linter from checking other rules in other files. Fixes: #4122 --- src/ansiblelint/_internal/rules.py | 2 +- src/ansiblelint/errors.py | 5 ++++ src/ansiblelint/file_utils.py | 9 +++++++ src/ansiblelint/rules/fqcn.py | 4 ++- src/ansiblelint/rules/name.py | 8 ++++-- src/ansiblelint/rules/schema.py | 10 +++++++- src/ansiblelint/runner.py | 41 ++++++++++++++++-------------- src/ansiblelint/utils.py | 4 +-- test/rules/test_syntax_check.py | 30 ++++++++++++++-------- test/test_import_tasks.py | 14 ++++++---- test/test_runner.py | 3 +-- 11 files changed, 86 insertions(+), 44 deletions(-) diff --git a/src/ansiblelint/_internal/rules.py b/src/ansiblelint/_internal/rules.py index b28dd0d4be..6672a5cb43 100644 --- a/src/ansiblelint/_internal/rules.py +++ b/src/ansiblelint/_internal/rules.py @@ -96,7 +96,7 @@ def getmatches(self, file: Lintable) -> list[MatchError]: _logger.warning( "Ignored exception from %s.%s while processing %s: %s", self.__class__.__name__, - method, + method.__name__, str(file), exc, ) diff --git a/src/ansiblelint/errors.py b/src/ansiblelint/errors.py index 416fa46412..861779d7fc 100644 --- a/src/ansiblelint/errors.py +++ b/src/ansiblelint/errors.py @@ -101,6 +101,11 @@ def __post_init__(self) -> None: self.lineno += self.lintable.line_offset + # We make the lintable aware that we found a match inside it, as this + # can be used to skip running other rules that do require current one + # to pass. + self.lintable.matches.append(self) + @functools.cached_property def level(self) -> str: """Return the level of the rule: error, warning or notice.""" diff --git a/src/ansiblelint/file_utils.py b/src/ansiblelint/file_utils.py index c3daf00a92..308da6facd 100644 --- a/src/ansiblelint/file_utils.py +++ b/src/ansiblelint/file_utils.py @@ -23,6 +23,8 @@ if TYPE_CHECKING: from collections.abc import Iterator, Sequence + from ansiblelint.errors import MatchError + _logger = logging.getLogger(__package__) @@ -201,6 +203,7 @@ def __init__( self.line_offset = ( 0 # Amount to offset line numbers by to get accurate position ) + self.matches: list[MatchError] = [] if isinstance(name, str): name = Path(name) @@ -391,6 +394,12 @@ def is_owned_by_ansible(self) -> bool: """Return true for YAML files that are managed by Ansible.""" return self.kind in ANSIBLE_OWNED_KINDS + def failed(self) -> bool: + """Return true if we already found syntax-check errors on this file.""" + return any( + match.rule.id in ("syntax-check", "load-failure") for match in self.matches + ) + @property def data(self) -> Any: """Return loaded data representation for current file, if possible.""" diff --git a/src/ansiblelint/rules/fqcn.py b/src/ansiblelint/rules/fqcn.py index da2200efab..5cbaf12f75 100644 --- a/src/ansiblelint/rules/fqcn.py +++ b/src/ansiblelint/rules/fqcn.py @@ -116,7 +116,9 @@ def matchtask( task: Task, file: Lintable | None = None, ) -> list[MatchError]: - result = [] + result: list[MatchError] = [] + if file and file.failed(): + return result module = task["action"]["__ansible_module_original__"] if not isinstance(module, str): msg = "Invalid data for module." diff --git a/src/ansiblelint/rules/name.py b/src/ansiblelint/rules/name.py index b6a6d6ffaf..4e930a7053 100644 --- a/src/ansiblelint/rules/name.py +++ b/src/ansiblelint/rules/name.py @@ -40,9 +40,11 @@ class NameRule(AnsibleLintRule, TransformMixin): def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: """Return matches found for a specific play (entry in playbook).""" - results = [] + results: list[MatchError] = [] if file.kind != "playbook": return [] + if file.failed(): + return results if "name" not in data: return [ self.create_matcherror( @@ -66,7 +68,9 @@ def matchtask( task: Task, file: Lintable | None = None, ) -> list[MatchError]: - results = [] + results: list[MatchError] = [] + if file and file.failed(): + return results name = task.get("name") if not name: results.append( diff --git a/src/ansiblelint/rules/schema.py b/src/ansiblelint/rules/schema.py index 1937d615f0..32f4d6ec4e 100644 --- a/src/ansiblelint/rules/schema.py +++ b/src/ansiblelint/rules/schema.py @@ -132,9 +132,13 @@ def matchtask( task: Task, file: Lintable | None = None, ) -> bool | str | MatchError | list[MatchError]: - results = [] + results: list[MatchError] = [] if not file: file = Lintable("", kind="tasks") + + if file.failed(): + return results + results.extend(self._get_field_matches(file=file, data=task.raw_task)) for key in pre_checks["task"]: if key in task.raw_task: @@ -154,6 +158,10 @@ def matchtask( def matchyaml(self, file: Lintable) -> list[MatchError]: """Return JSON validation errors found as a list of MatchError(s).""" result: list[MatchError] = [] + + if file.failed(): + return result + if file.kind not in JSON_SCHEMAS: return result diff --git a/src/ansiblelint/runner.py b/src/ansiblelint/runner.py index b01cfd387c..5b8d61fb12 100644 --- a/src/ansiblelint/runner.py +++ b/src/ansiblelint/runner.py @@ -260,28 +260,28 @@ def worker(lintable: Lintable) -> list[MatchError]: matches.extend(data) matches = self._filter_excluded_matches(matches) + # -- phase 2 --- - if not matches: - # do our processing only when ansible syntax check passed in order - # to avoid causing runtime exceptions. Our processing is not as - # resilient to be able process garbage. - matches.extend(self._emit_matches(files)) + # do our processing only when ansible syntax check passed in order + # to avoid causing runtime exceptions. Our processing is not as + # resilient to be able process garbage. + matches.extend(self._emit_matches(files)) - # remove duplicates from files list - files = [value for n, value in enumerate(files) if value not in files[:n]] + # remove duplicates from files list + files = [value for n, value in enumerate(files) if value not in files[:n]] - for file in self.lintables: - if file in self.checked_files or not file.kind: - continue - _logger.debug( - "Examining %s of type %s", - ansiblelint.file_utils.normpath(file.path), - file.kind, - ) + for file in self.lintables: + if file in self.checked_files or not file.kind or file.failed(): + continue + _logger.debug( + "Examining %s of type %s", + ansiblelint.file_utils.normpath(file.path), + file.kind, + ) - matches.extend( - self.rules.run(file, tags=set(self.tags), skip_list=self.skip_list), - ) + matches.extend( + self.rules.run(file, tags=set(self.tags), skip_list=self.skip_list), + ) # update list of checked files self.checked_files.update(self.lintables) @@ -479,7 +479,10 @@ def find_children(self, lintable: Lintable) -> list[Lintable]: try: playbook_ds = ansiblelint.utils.parse_yaml_from_file(str(lintable.path)) except AnsibleError as exc: - raise SystemExit(exc) from exc + msg = f"Loading {lintable.filename} caused an {type(exc).__name__} exception: {exc}, file was ignored." + logging.error(msg) + # raise SystemExit(exc) from exc + return [] results = [] # playbook_ds can be an AnsibleUnicode string, which we consider invalid if isinstance(playbook_ds, str): diff --git a/src/ansiblelint/utils.py b/src/ansiblelint/utils.py index 2da21ca15e..ca16aff7ea 100644 --- a/src/ansiblelint/utils.py +++ b/src/ansiblelint/utils.py @@ -334,7 +334,7 @@ def taskshandlers_children( ) -> list[Lintable]: """TasksHandlers Children.""" results: list[Lintable] = [] - if v is None: + if v is None or isinstance(v, int | str): raise MatchError( message="A malformed block was encountered while loading a block.", rule=RuntimeErrorRule(), @@ -463,7 +463,7 @@ def _get_task_handler_children_for_tasks_or_playbooks( for task_handler_key in INCLUSION_ACTION_NAMES: with contextlib.suppress(KeyError): # ignore empty tasks - if not task_handler: # pragma: no branch + if not task_handler or isinstance(task_handler, str): # pragma: no branch continue file_name = task_handler[task_handler_key] diff --git a/test/rules/test_syntax_check.py b/test/rules/test_syntax_check.py index 04e5a07f49..bc23581f46 100644 --- a/test/rules/test_syntax_check.py +++ b/test/rules/test_syntax_check.py @@ -16,18 +16,26 @@ def test_get_ansible_syntax_check_matches( kind="playbook", ) - result = Runner(lintable, rules=default_rules_collection).run() + result = sorted(Runner(lintable, rules=default_rules_collection).run()) - assert result[0].lineno == 4 - assert result[0].column == 7 - assert ( - result[0].message - == "conflicting action statements: ansible.builtin.debug, ansible.builtin.command" - ) - # We internally convert absolute paths returned by ansible into paths - # relative to current directory. - assert result[0].filename.endswith("/conflicting_action.yml") - assert len(result) == 1 + expected_results = [ + ["name[play]", 2, None, ""], + [ + "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] + assert result[index].lineno == expected[1] + assert result[index].column == expected[2] + 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") def test_empty_playbook(default_rules_collection: RulesCollection) -> None: diff --git a/test/test_import_tasks.py b/test/test_import_tasks.py index 8018e08e52..ceb5c2863f 100644 --- a/test/test_import_tasks.py +++ b/test/test_import_tasks.py @@ -7,24 +7,28 @@ @pytest.mark.parametrize( - "playbook_path", + ("playbook_path", "lintable_count", "match_count"), ( pytest.param( "examples/playbooks/test_import_with_conflicting_action_statements.yml", + 2, + 4, id="0", ), - pytest.param("examples/playbooks/test_import_with_malformed.yml", id="1"), + pytest.param("examples/playbooks/test_import_with_malformed.yml", 2, 2, id="1"), ), ) def test_import_tasks( default_rules_collection: RulesCollection, playbook_path: str, + lintable_count: int, + match_count: int, ) -> None: """Assures import_playbook includes are recognized.""" runner = Runner(playbook_path, rules=default_rules_collection) results = runner.run() - assert len(runner.lintables) == 1 - assert len(results) == 1 + assert len(runner.lintables) == lintable_count + assert len(results) == match_count # Assures we detected the issues from imported file - assert results[0].rule.id == "syntax-check" + assert results[0].rule.id in ("syntax-check", "load-failure") diff --git a/test/test_runner.py b/test/test_runner.py index 0fcaa2328a..d12e6c924a 100644 --- a/test/test_runner.py +++ b/test/test_runner.py @@ -103,8 +103,7 @@ def test_runner_exclude_globs( ) matches = runner.run() - # we expect to find one match from the very few .yaml file we have there (most of them have .yml extension) - assert len(matches) == 1 + assert len(matches) == 83 @pytest.mark.parametrize(