Skip to content

Commit

Permalink
Refactor parsing of ansible syntax check
Browse files Browse the repository at this point in the history
- adds an internal 'warning' rule for displaying generic warnings
- change parsing of syntax check in order to make it easier to
  maintain and extend, with generic error message detection
  • Loading branch information
ssbarnea committed Oct 1, 2022
1 parent c62e9df commit 302ca91
Show file tree
Hide file tree
Showing 9 changed files with 125 additions and 50 deletions.
1 change: 1 addition & 0 deletions docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@
("py:class", "handlers"),
("py:class", "meta"),
("py:class", "playbook"),
("py:class", "re.Pattern"),
("py:class", "requirements"),
("py:class", "role"),
("py:class", "ruamel.yaml.comments.CommentedMap"),
Expand Down
11 changes: 11 additions & 0 deletions src/ansiblelint/_internal/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,14 @@ class LoadingFailureRule(BaseRule):
version_added = "v4.3.0"
help = LOAD_FAILURE_MD
_order = 0


class WarningRule(BaseRule):
"""Other warnings detected during run."""

id = "warning"
severity = "LOW"
# should remain experimental as that would keep it warning only
tags = ["core", "experimental"]
version_added = "v6.8.0"
_order = 0
7 changes: 7 additions & 0 deletions src/ansiblelint/_internal/warning.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# warning

`warning` is a special type of internal rule that is used to report generic
runtime warnings found during execution. As stated by its name, they are
not counted as errors, so they do not influence the final outcome.

- `warning[empty-playbook]` is raised when a playbook file has no content.
1 change: 1 addition & 0 deletions src/ansiblelint/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"name[casing]",
"name[play]",
"role-name",
"warning[empty-playbook]", # because ansible considers it warning only
]

DEFAULT_KINDS = [
Expand Down
8 changes: 7 additions & 1 deletion src/ansiblelint/rules/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
BaseRule,
LoadingFailureRule,
RuntimeErrorRule,
WarningRule,
)
from ansiblelint.config import PROFILES, get_rule_config
from ansiblelint.config import options as default_options
Expand Down Expand Up @@ -383,7 +384,12 @@ def __init__(
# internal rules included in order to expose them for docs as they are
# not directly loaded by our rule loader.
self.rules.extend(
[RuntimeErrorRule(), AnsibleParserErrorRule(), LoadingFailureRule()]
[
RuntimeErrorRule(),
AnsibleParserErrorRule(),
LoadingFailureRule(),
WarningRule(),
]
)
for rule in load_plugins(rulesdirs):
self.register(rule, conditional=conditional)
Expand Down
133 changes: 91 additions & 42 deletions src/ansiblelint/rules/syntax_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
import re
import subprocess
import sys
from dataclasses import dataclass
from typing import Any

from ansiblelint._internal.rules import BaseRule, RuntimeErrorRule
from ansiblelint._internal.rules import BaseRule, RuntimeErrorRule, WarningRule
from ansiblelint.app import get_app
from ansiblelint.config import options
from ansiblelint.errors import MatchError
Expand All @@ -16,14 +17,44 @@
from ansiblelint.rules import AnsibleLintRule
from ansiblelint.text import strip_ansi_escape

_ansible_syntax_check_re = re.compile(
r"^ERROR! (?P<title>[^\n]*)\n\nThe error appears to be in "
r"'(?P<filename>.*)': line (?P<line>\d+), column (?P<column>\d+)",
re.MULTILINE | re.S | re.DOTALL,
)

_empty_playbook_re = re.compile(
r"^ERROR! Empty playbook, nothing to do", re.MULTILINE | re.S | re.DOTALL
@dataclass
class KnownError:
"""Class that tracks result of linting."""

tag: str
regex: re.Pattern[str]


OUTPUT_PATTERNS = (
KnownError(
tag="missing-file",
regex=re.compile(
# do not use <filename> capture group for this because we want to report original file, not the missing target one
r"(?P<title>Unable to retrieve file contents)\n(?P<details>Could not find or access '(?P<value>.*)'[^\n]*)",
re.MULTILINE | re.S | re.DOTALL,
),
),
KnownError(
tag="specific",
regex=re.compile(
r"^ERROR! (?P<title>[^\n]*)\n\nThe error appears to be in '(?P<filename>.*)': line (?P<line>\d+), column (?P<column>\d+)",
re.MULTILINE | re.S | re.DOTALL,
),
),
KnownError(
tag="empty-playbook",
regex=re.compile(
"Empty playbook, nothing to do", re.MULTILINE | re.S | re.DOTALL
),
),
KnownError(
tag="malformed",
regex=re.compile(
"^ERROR! (?P<title>A malformed block was encountered while loading a block[^\n]*)",
re.MULTILINE | re.S | re.DOTALL,
),
),
)


Expand All @@ -32,14 +63,16 @@ class AnsibleSyntaxCheckRule(AnsibleLintRule):

id = "syntax-check"
severity = "VERY_HIGH"
tags = ["core", "unskippable"]
tags = ["core"]
version_added = "v5.0.0"
_order = 0

@staticmethod
# pylint: disable=too-many-locals
def _get_ansible_syntax_check_matches(lintable: Lintable) -> list[MatchError]:
"""Run ansible syntax check and return a list of MatchError(s)."""
default_rule: BaseRule = AnsibleSyntaxCheckRule()
results = []
if lintable.kind != "playbook":
return []

Expand Down Expand Up @@ -72,7 +105,7 @@ def _get_ansible_syntax_check_matches(lintable: Lintable) -> list[MatchError]:
check=False,
env=env,
)
result = []

if run.returncode != 0:
message = None
filename = lintable
Expand All @@ -89,40 +122,56 @@ def _get_ansible_syntax_check_matches(lintable: Lintable) -> list[MatchError]:
else:
details = stdout

match = _ansible_syntax_check_re.search(stderr)
if match:
message = match.groupdict()["title"]
# Ansible returns absolute paths
filename = Lintable(match.groupdict()["filename"])
linenumber = int(match.groupdict()["line"])
column = int(match.groupdict()["column"])
elif _empty_playbook_re.search(stderr):
message = "Empty playbook, nothing to do"
filename = lintable
tag = "empty-playbook"

if run.returncode == 4:
rule: BaseRule = AnsibleSyntaxCheckRule()
else:
rule = RuntimeErrorRule()
if not message:
message = (
f"Unexpected error code {run.returncode} from "
f"execution of: {' '.join(cmd)}"
for pattern in OUTPUT_PATTERNS:
rule = default_rule
match = re.search(pattern.regex, stderr)
if match:
groups = match.groupdict()
title = groups.get("title", match.group(0))
details = groups.get("details", "")
linenumber = int(groups.get("line", 1))

if "filename" in groups:
filename = Lintable(groups["filename"])
else:
filename = lintable
column = int(groups.get("column", 1))

if pattern.tag == "empty-playbook":
rule = WarningRule()

# breakpoint()
results.append(
MatchError(
message=title,
filename=filename,
linenumber=linenumber,
column=column,
rule=rule,
details=details,
tag=f"{rule.id}[{pattern.tag}]",
)
)

result.append(
MatchError(
message=message,
filename=filename,
linenumber=linenumber,
column=column,
rule=rule,
details=details,
tag=tag,
if not results:
rule = RuntimeErrorRule()
message = (
f"Unexpected error code {run.returncode} from "
f"execution of: {' '.join(cmd)}"
)
)
return result
results.append(
MatchError(
message=message,
filename=filename,
linenumber=linenumber,
column=column,
rule=rule,
details=details,
tag=tag,
)
)

return results


# testing code to be loaded only with pytest or when executed the rule file
Expand Down Expand Up @@ -155,7 +204,7 @@ def test_empty_playbook() -> None:
# We internally convert absolute paths returned by ansible into paths
# relative to current directory.
assert result[0].filename.endswith("/empty_playbook.yml")
assert result[0].tag == "empty-playbook"
assert result[0].tag == "warning[empty-playbook]"
assert result[0].message == "Empty playbook, nothing to do"
assert len(result) == 1

Expand Down
6 changes: 3 additions & 3 deletions test/test_profiles.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@
def test_profile_min() -> None:
"""Asserts our ability to unload rules based on profile."""
collection = RulesCollection()
assert len(collection.rules) == 3, "Unexpected number of implicit rules."
assert len(collection.rules) == 4, "Unexpected number of implicit rules."
# register one extra rule that we know not to be part of "min" profile

collection.register(ShellWithoutPipefail())
assert len(collection.rules) == 4, "Failed to register new rule."
assert len(collection.rules) == 5, "Failed to register new rule."

filter_rules_with_profile(collection.rules, "min")
assert (
len(collection.rules) == 3
len(collection.rules) == 4
), "Failed to unload rule that is not part of 'min' profile."


Expand Down
4 changes: 2 additions & 2 deletions test/test_rules_collection.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ def fixture_bracketsmatchtestfile() -> Lintable:
def test_load_collection_from_directory(test_rules_collection: RulesCollection) -> None:
"""Test that custom rules extend the default ones."""
# two detected rules plus the internal ones
assert len(test_rules_collection) == 6
assert len(test_rules_collection) == 7


def test_run_collection(
Expand Down Expand Up @@ -166,5 +166,5 @@ def test_rules_id_format() -> None:
rule.help != "" or rule.description or rule.__doc__
), f"Rule {rule.id} must have at least one of: .help, .description, .__doc__"
assert "yaml" in keys, "yaml rule is missing"
assert len(rules) == 46 # update this number when adding new rules!
assert len(rules) == 47 # update this number when adding new rules!
assert len(keys) == len(rules), "Duplicate rule ids?"
4 changes: 2 additions & 2 deletions test/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,8 @@ def test_cli_auto_detect(capfd: CaptureFixture[str]) -> None:
assert "Excluded removed files using: git ls-files --deleted -z" in err
# An expected rule match from our examples
assert (
"examples/playbooks/empty_playbook.yml:1: "
"syntax-check: Empty playbook, nothing to do" in out
"examples/playbooks/empty_playbook.yml:1:1: "
"warning: Empty playbook, nothing to do" in out
)
# assures that our ansible-lint config exclude was effective in excluding github files
assert "Identified: .github/" not in out
Expand Down

0 comments on commit 302ca91

Please sign in to comment.