Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing MatchErrors due to hash collisions #4307

Merged
merged 1 commit into from
Sep 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions examples/playbooks/vars/rule_var_naming_fails_files/bar.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
CamelCaseIsBad: bar
ALL_CAPS_ARE_BAD_TOO: bar
3 changes: 3 additions & 0 deletions examples/playbooks/vars/rule_var_naming_fails_files/foo.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
CamelCaseIsBad: foo
ALL_CAPS_ARE_BAD_TOO: foo
55 changes: 43 additions & 12 deletions src/ansiblelint/rules/var_naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,15 @@ def get_var_naming_matcherror(
ident: str,
*,
prefix: Prefix | None = None,
file: Lintable,
) -> MatchError | None:
"""Return a MatchError if the variable name is not valid, otherwise None."""
if not isinstance(ident, str): # pragma: no cover
return MatchError(
tag="var-naming[non-string]",
message="Variables names must be strings.",
rule=self,
lintable=file,
)

if ident in ANNOTATION_KEYS or ident in self.allowed_special_names:
Expand All @@ -136,27 +138,31 @@ def get_var_naming_matcherror(
tag="var-naming[non-ascii]",
message=f"Variables names must be ASCII. ({ident})",
rule=self,
lintable=file,
)

if keyword.iskeyword(ident):
return MatchError(
tag="var-naming[no-keyword]",
message=f"Variables names must not be Python keywords. ({ident})",
rule=self,
lintable=file,
)

if ident in self.reserved_names:
return MatchError(
tag="var-naming[no-reserved]",
message=f"Variables names must not be Ansible reserved names. ({ident})",
rule=self,
lintable=file,
)

if ident in self.read_only_names:
return MatchError(
tag="var-naming[read-only]",
message=f"This special variable is read-only. ({ident})",
rule=self,
lintable=file,
)

# We want to allow use of jinja2 templating for variable names
Expand All @@ -165,6 +171,7 @@ def get_var_naming_matcherror(
tag="var-naming[no-jinja]",
message="Variables names must not contain jinja2 templating.",
rule=self,
lintable=file,
)

if not bool(self.re_pattern.match(ident)) and (
Expand All @@ -174,6 +181,7 @@ def get_var_naming_matcherror(
tag="var-naming[pattern]",
message=f"Variables names should match {self.re_pattern_str} regex. ({ident})",
rule=self,
lintable=file,
)

if (
Expand All @@ -186,6 +194,7 @@ def get_var_naming_matcherror(
tag="var-naming[no-role-prefix]",
message=f"Variables names from within roles should use {prefix.value}_ as a prefix.",
rule=self,
lintable=file,
)
return None

Expand All @@ -199,9 +208,8 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
# If the Play uses the 'vars' section to set variables
our_vars = data.get("vars", {})
for key in our_vars:
match_error = self.get_var_naming_matcherror(key)
match_error = self.get_var_naming_matcherror(key, file=file)
if match_error:
match_error.filename = str(file.path)
match_error.lineno = (
key.ansible_pos[1]
if isinstance(key, AnsibleUnicode)
Expand All @@ -219,9 +227,9 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file,
)
if match_error:
match_error.filename = str(file.path)
match_error.message += f" (vars: {key})"
match_error.lineno = (
key.ansible_pos[1]
Expand All @@ -235,9 +243,9 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file,
)
if match_error:
match_error.filename = str(file.path)
match_error.message += f" (vars: {key})"
match_error.lineno = (
key.ansible_pos[1]
Expand Down Expand Up @@ -266,7 +274,6 @@ def matchtask(
"""Return matches for task based variables."""
results = []
prefix = Prefix()
filename = "" if file is None else str(file.path)
if file and file.parent and file.parent.kind == "role":
prefix = Prefix(file.parent.path.name)
ansible_module = task["action"]["__ansible_module__"]
Expand All @@ -283,9 +290,9 @@ def matchtask(
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file or Lintable(""),
)
if match_error:
match_error.filename = filename
match_error.lineno = our_vars[LINE_NUMBER_KEY]
match_error.message += f" (vars: {key})"
results.append(match_error)
Expand All @@ -298,9 +305,12 @@ def matchtask(
and x != "cacheable",
task["action"].keys(),
):
match_error = self.get_var_naming_matcherror(key, prefix=prefix)
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file or Lintable(""),
)
if match_error:
match_error.filename = filename
match_error.lineno = task["action"][LINE_NUMBER_KEY]
match_error.message += f" (set_fact: {key})"
results.append(match_error)
Expand All @@ -311,10 +321,10 @@ def matchtask(
match_error = self.get_var_naming_matcherror(
registered_var,
prefix=prefix,
file=file or Lintable(""),
)
if match_error:
match_error.message += f" (register: {registered_var})"
match_error.filename = filename
match_error.lineno = task[LINE_NUMBER_KEY]
results.append(match_error)

Expand All @@ -325,15 +335,17 @@ def matchyaml(self, file: Lintable) -> list[MatchError]:
results: list[MatchError] = []
raw_results: list[MatchError] = []
meta_data: dict[AnsibleUnicode, Any] = {}
filename = "" if file is None else str(file.path)

if str(file.kind) == "vars" and file.data:
meta_data = parse_yaml_from_file(str(file.path))
for key in meta_data:
prefix = Prefix(file.role) if file.role else Prefix()
match_error = self.get_var_naming_matcherror(key, prefix=prefix)
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
file=file,
)
if match_error:
match_error.filename = filename
match_error.lineno = key.ansible_pos[1]
match_error.message += f" (vars: {key})"
raw_results.append(match_error)
Expand Down Expand Up @@ -413,6 +425,25 @@ def test_invalid_var_name_varsfile(
assert result.tag == expected_errors[idx][0]
assert result.lineno == expected_errors[idx][1]

def test_invalid_vars_diff_files(
default_rules_collection: RulesCollection,
) -> None:
"""Test rule matches."""
results = Runner(
Lintable("examples/playbooks/vars/rule_var_naming_fails_files"),
rules=default_rules_collection,
).run()
expected_errors = (
("var-naming[pattern]", 2),
("var-naming[pattern]", 3),
("var-naming[pattern]", 2),
("var-naming[pattern]", 3),
)
assert len(results) == len(expected_errors)
for idx, result in enumerate(results):
assert result.tag == expected_errors[idx][0]
assert result.lineno == expected_errors[idx][1]

def test_var_naming_with_role_prefix(
default_rules_collection: RulesCollection,
) -> None:
Expand Down
13 changes: 6 additions & 7 deletions src/ansiblelint/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,7 @@ def taskshandlers_children(
raise MatchError(
message="A malformed block was encountered while loading a block.",
rule=RuntimeErrorRule(),
lintable=lintable,
)
for task_handler in v:
# ignore empty tasks, `-`
Expand Down Expand Up @@ -408,23 +409,21 @@ def _validate_task_handler_action_for_role(self, th_action: dict[str, Any]) -> N
"""Verify that the task handler action is valid for role include."""
module = th_action["__ansible_module__"]

lintable = Lintable(
self.rules.options.lintables[0] if self.rules.options.lintables else ".",
)
if "name" not in th_action:
raise MatchError(
message=f"Failed to find required 'name' key in {module!s}",
rule=self.rules.rules[0],
lintable=Lintable(
(
self.rules.options.lintables[0]
if self.rules.options.lintables
else "."
),
),
lintable=lintable,
)

if not isinstance(th_action["name"], str):
raise MatchError(
message=f"Value assigned to 'name' key on '{module!s}' is not a string.",
rule=self.rules.rules[1],
lintable=lintable,
)

def roles_children(
Expand Down
2 changes: 1 addition & 1 deletion tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ setenv =
PRE_COMMIT_COLOR = always
# Number of expected test passes, safety measure for accidental skip of
# tests. Update value if you add/remove tests. (tox-extra)
PYTEST_REQPASS = 893
PYTEST_REQPASS = 894
FORCE_COLOR = 1
pre: PIP_PRE = 1
allowlist_externals =
Expand Down
Loading