Skip to content
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
2 changes: 1 addition & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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: 884
PYTEST_REQPASS: 886
steps:
- uses: actions/checkout@v4
with:
Expand Down
10 changes: 10 additions & 0 deletions examples/playbooks/bug-4095.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
- name: Test task
hosts: localhost
connection: local
tasks:
- name: Reproduce bug related bug 4095
ansible.builtin.include_role:
name: local.testcollection.bug4095
vars:
var1: val1
71 changes: 53 additions & 18 deletions src/ansiblelint/rules/var_naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import keyword
import re
import sys
from typing import TYPE_CHECKING, Any
from typing import TYPE_CHECKING, Any, NamedTuple

from ansible.parsing.yaml.objects import AnsibleUnicode
from ansible.vars.reserved import get_reserved_names
Expand All @@ -22,13 +22,20 @@
from ansiblelint.rules import AnsibleLintRule, RulesCollection
from ansiblelint.runner import Runner
from ansiblelint.skip_utils import get_rule_skips_from_line
from ansiblelint.text import has_jinja, is_fqcn_or_name
from ansiblelint.text import has_jinja, is_fqcn, is_fqcn_or_name
from ansiblelint.utils import parse_yaml_from_file

if TYPE_CHECKING:
from ansiblelint.utils import Task


class Prefix(NamedTuple):
"""Prefix."""

value: str = ""
from_fqcn: bool = False


class VariableNamingRule(AnsibleLintRule):
"""All variables should be named using only lowercase and underscores."""

Expand Down Expand Up @@ -109,7 +116,7 @@ def get_var_naming_matcherror(
self,
ident: str,
*,
prefix: str = "",
prefix: Prefix | None = None,
) -> MatchError | None:
"""Return a MatchError if the variable name is not valid, otherwise None."""
if not isinstance(ident, str): # pragma: no cover
Expand Down Expand Up @@ -160,7 +167,9 @@ def get_var_naming_matcherror(
rule=self,
)

if not bool(self.re_pattern.match(ident)):
if not bool(self.re_pattern.match(ident)) and (
not prefix or not prefix.from_fqcn
):
return MatchError(
tag="var-naming[pattern]",
message=f"Variables names should match {self.re_pattern_str} regex. ({ident})",
Expand All @@ -169,13 +178,13 @@ def get_var_naming_matcherror(

if (
prefix
and not ident.lstrip("_").startswith(f"{prefix}_")
and not has_jinja(prefix)
and is_fqcn_or_name(prefix)
and not ident.lstrip("_").startswith(f"{prefix.value}_")
and not has_jinja(prefix.value)
and is_fqcn_or_name(prefix.value)
):
return MatchError(
tag="var-naming[no-role-prefix]",
message=f"Variables names from within roles should use {prefix}_ as a prefix.",
message=f"Variables names from within roles should use {prefix.value}_ as a prefix.",
rule=self,
)
return None
Expand Down Expand Up @@ -204,10 +213,13 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
if isinstance(role, AnsibleUnicode):
continue
role_fqcn = role.get("role", role.get("name"))
prefix = role_fqcn.split("/" if "/" in role_fqcn else ".")[-1]
prefix = self._parse_prefix(role_fqcn)
for key in list(role.keys()):
if key not in PLAYBOOK_ROLE_KEYWORDS:
match_error = self.get_var_naming_matcherror(key, prefix=prefix)
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
)
if match_error:
match_error.filename = str(file.path)
match_error.message += f" (vars: {key})"
Expand All @@ -220,7 +232,10 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:

our_vars = role.get("vars", {})
for key in our_vars:
match_error = self.get_var_naming_matcherror(key, prefix=prefix)
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
)
if match_error:
match_error.filename = str(file.path)
match_error.message += f" (vars: {key})"
Expand Down Expand Up @@ -250,22 +265,25 @@ def matchtask(
) -> list[MatchError]:
"""Return matches for task based variables."""
results = []
prefix = ""
prefix = Prefix()
filename = "" if file is None else str(file.path)
if file and file.parent and file.parent.kind == "role":
prefix = file.parent.path.name
prefix = Prefix(file.parent.path.name)
ansible_module = task["action"]["__ansible_module__"]
# If the task uses the 'vars' section to set variables
our_vars = task.get("vars", {})
if ansible_module in ("include_role", "import_role"):
action = task["action"]
if isinstance(action, dict):
role_fqcn = action.get("name", "")
prefix = role_fqcn.split("/" if "/" in role_fqcn else ".")[-1]
prefix = self._parse_prefix(role_fqcn)
else:
prefix = ""
prefix = Prefix()
for key in our_vars:
match_error = self.get_var_naming_matcherror(key, prefix=prefix)
match_error = self.get_var_naming_matcherror(
key,
prefix=prefix,
)
if match_error:
match_error.filename = filename
match_error.lineno = our_vars[LINE_NUMBER_KEY]
Expand All @@ -290,7 +308,10 @@ def matchtask(
# If the task registers a variable
registered_var = task.get("register", None)
if registered_var:
match_error = self.get_var_naming_matcherror(registered_var, prefix=prefix)
match_error = self.get_var_naming_matcherror(
registered_var,
prefix=prefix,
)
if match_error:
match_error.message += f" (register: {registered_var})"
match_error.filename = filename
Expand All @@ -309,7 +330,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]:
if str(file.kind) == "vars" and file.data:
meta_data = parse_yaml_from_file(str(file.path))
for key in meta_data:
prefix = file.role if file.role else ""
prefix = Prefix(file.role) if file.role else Prefix()
match_error = self.get_var_naming_matcherror(key, prefix=prefix)
if match_error:
match_error.filename = filename
Expand All @@ -330,6 +351,9 @@ def matchyaml(self, file: Lintable) -> list[MatchError]:
results.extend(super().matchyaml(file))
return results

def _parse_prefix(self, fqcn: str) -> Prefix:
return Prefix("" if "." in fqcn else fqcn.split("/")[-1], is_fqcn(fqcn))


# testing code to be loaded only with pytest or when executed the rule file
if "pytest" in sys.modules:
Expand Down Expand Up @@ -434,6 +458,17 @@ def test_var_naming_with_pattern() -> None:
assert result.returncode == RC.SUCCESS
assert "var-naming" not in result.stdout

def test_var_naming_with_pattern_foreign_role() -> None:
"""Test rule matches."""
role_path = "examples/playbooks/bug-4095.yml"
conf_path = "examples/roles/var_naming_pattern/.ansible-lint"
result = run_ansible_lint(
f"--config-file={conf_path}",
role_path,
)
assert result.returncode == RC.SUCCESS
assert "var-naming" not in result.stdout

def test_var_naming_with_include_tasks_and_vars() -> None:
"""Test with include tasks and vars."""
role_path = "examples/roles/var_naming_pattern/tasks/include_task_with_vars.yml"
Expand Down
7 changes: 7 additions & 0 deletions src/ansiblelint/text.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,10 @@ def has_glob(value: str) -> bool:
def is_fqcn_or_name(value: str) -> bool:
"""Return true if a string seems to be a module/filter old name or a fully qualified one."""
return bool(isinstance(value, str) and RE_IS_FQCN_OR_NAME.search(value))


@cache
def is_fqcn(value: str) -> bool:
"""Return true if a string seems to be a fully qualified collection name."""
match = RE_IS_FQCN_OR_NAME.search(value)
return bool(isinstance(value, str) and match and match.group(1))
33 changes: 26 additions & 7 deletions src/ansiblelint/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@
from ansiblelint.errors import MatchError
from ansiblelint.file_utils import Lintable, discover_lintables
from ansiblelint.skip_utils import is_nested_task
from ansiblelint.text import has_jinja, removeprefix
from ansiblelint.text import has_jinja, is_fqcn, removeprefix

if TYPE_CHECKING:
from ansiblelint.rules import RulesCollection
Expand Down Expand Up @@ -505,21 +505,40 @@ def _get_task_handler_children_for_tasks_or_playbooks(

def _rolepath(basedir: str, role: str) -> str | None:
role_path = None
namespace_name, collection_name, role_name = (
role.split(".") if is_fqcn(role) else ("", "", role)
)

possible_paths = [
# if included from a playbook
path_dwim(basedir, os.path.join("roles", role)),
path_dwim(basedir, role),
path_dwim(basedir, os.path.join("roles", role_name)),
path_dwim(basedir, role_name),
# if included from roles/[role]/meta/main.yml
path_dwim(basedir, os.path.join("..", "..", "..", "roles", role)),
path_dwim(basedir, os.path.join("..", "..", role)),
path_dwim(basedir, os.path.join("..", "..", "..", "roles", role_name)),
path_dwim(basedir, os.path.join("..", "..", role_name)),
# if checking a role in the current directory
path_dwim(basedir, os.path.join("..", role)),
path_dwim(basedir, os.path.join("..", role_name)),
]

for loc in get_app(cached=True).runtime.config.default_roles_path:
loc = os.path.expanduser(loc)
possible_paths.append(path_dwim(loc, role))
possible_paths.append(path_dwim(loc, role_name))

if namespace_name and collection_name:
for loc in get_app(cached=True).runtime.config.collections_paths:
loc = os.path.expanduser(loc)
possible_paths.append(
path_dwim(
loc,
os.path.join(
"ansible_collections",
namespace_name,
collection_name,
"roles",
role_name,
),
),
)

possible_paths.append(path_dwim(basedir, ""))

Expand Down
10 changes: 10 additions & 0 deletions test/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -480,3 +480,13 @@ def test_find_children_in_module(default_rules_collection: RulesCollection) -> N
# Child correctly looks like a YAML file
assert child.base_kind == "text/yaml"
assert child.content.startswith("---")


def test_find_children_in_playbook(default_rules_collection: RulesCollection) -> None:
"""Verify correct function of find_children() in playbooks."""
lintable = Lintable("examples/playbooks/bug-4095.yml")
children = Runner(
rules=default_rules_collection,
).find_children(lintable)
assert len(children) == 1
assert children[0].role == "bug4095"