Skip to content

Commit

Permalink
Run entire linting regardless if syntax-check errors are found
Browse files Browse the repository at this point in the history
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
  • Loading branch information
ssbarnea committed May 5, 2024
1 parent 262c02c commit 232db79
Show file tree
Hide file tree
Showing 11 changed files with 86 additions and 44 deletions.
2 changes: 1 addition & 1 deletion src/ansiblelint/_internal/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
Expand Down
5 changes: 5 additions & 0 deletions src/ansiblelint/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand Down
9 changes: 9 additions & 0 deletions src/ansiblelint/file_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
if TYPE_CHECKING:
from collections.abc import Iterator, Sequence

from ansiblelint.errors import MatchError


_logger = logging.getLogger(__package__)

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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."""
Expand Down
4 changes: 3 additions & 1 deletion src/ansiblelint/rules/fqcn.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down
8 changes: 6 additions & 2 deletions src/ansiblelint/rules/name.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down
10 changes: 9 additions & 1 deletion src/ansiblelint/rules/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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

Expand Down
41 changes: 22 additions & 19 deletions src/ansiblelint/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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):
Expand Down
4 changes: 2 additions & 2 deletions src/ansiblelint/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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]
Expand Down
30 changes: 19 additions & 11 deletions test/rules/test_syntax_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
14 changes: 9 additions & 5 deletions test/test_import_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
3 changes: 1 addition & 2 deletions test/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit 232db79

Please sign in to comment.