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 detection of play roles vars missing prefix #3765

Merged
merged 4 commits into from
Sep 28, 2023
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
14 changes: 1 addition & 13 deletions .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -67,22 +67,10 @@ jobs:
# proof that we failed to catch a bug by not running it. Using
# distribution should be preferred instead of custom builds.
env:
# vars safe to be passed to wsl:
WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY
# Number of expected test passes, safety measure for accidental skip of
# tests. Update value if you add/remove tests.
PYTEST_REQPASS: 827
PYTEST_REQPASS: 828
steps:
- name: Activate WSL1
if: "contains(matrix.shell, 'wsl')"
uses: Vampire/setup-wsl@v2

- name: MacOS workaround for https://github.com/actions/virtual-environments/issues/1187
if: ${{ matrix.os == 'macOS-latest' }}
run: |
sudo sysctl -w net.link.generic.system.hwcksum_tx=0
sudo sysctl -w net.link.generic.system.hwcksum_rx=0

- uses: actions/checkout@v4
with:
fetch-depth: 0 # needed by setuptools-scm
Expand Down
51 changes: 51 additions & 0 deletions examples/playbooks/role_vars_prefix_detection.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
---
- name: Test role-prefix
hosts: localhost
connection: local
roles:
- role_vars_prefix_detection

- role: role_vars_prefix_detection
var1: val1

- role: role_vars_prefix_detection
var1: val1
become: true
vars:
var2: val2

- role: role_vars_prefix_detection
become: true
environment:
FOO: /bar/barr
role_vars_prefix_detection_var1: val1

- role: role_vars_prefix_detection
vars:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side blurb, I would think one would want to put role variables under the vars key instead of mixing them in the same dictionary as the role name. This would prevent confusion with a playbook's role keywords.

var1: val1

- role: role_vars_prefix_detection
become: true
environment:
BAR: /baz
vars:
var1: val1

- role: role_vars_prefix_detection
become: true
environment:
BAR: /baz
vars:
role_vars_prefix_detection_var1: val1
tasks:
- name: Include1
ansible.builtin.include_role:
name: role_vars_prefix_detection
vars:
var1: val1

- name: Include2
ansible.builtin.include_role:
name: role_vars_prefix_detection
vars:
role_vars_prefix_detection_var2: val2
30 changes: 30 additions & 0 deletions src/ansiblelint/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,36 @@ def main():
"pre_tasks",
"post_tasks",
]
PLAYBOOK_ROLE_KEYWORDS = [
Copy link
Contributor Author

@cavcrosby cavcrosby Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"any_errors_fatal",
"become",
"become_exe",
"become_flags",
"become_method",
"become_user",
"check_mode",
"collections",
"connection",
"debugger",
"delegate_facts",
"delegate_to",
"diff",
"environment",
"ignore_errors",
"ignore_unreachable",
"module_defaults",
"name",
"role",
"no_log",
"port",
"remote_user",
"run_once",
"tags",
"throttle",
"timeout",
"vars",
"when",
]
NESTED_TASK_KEYS = [
"block",
"always",
Expand Down
60 changes: 59 additions & 1 deletion src/ansiblelint/rules/var_naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@
from ansible.vars.reserved import get_reserved_names

from ansiblelint.config import options
from ansiblelint.constants import ANNOTATION_KEYS, LINE_NUMBER_KEY, RC
from ansiblelint.constants import (
ANNOTATION_KEYS,
LINE_NUMBER_KEY,
PLAYBOOK_ROLE_KEYWORDS,
RC,
)
from ansiblelint.errors import MatchError
from ansiblelint.file_utils import Lintable
from ansiblelint.rules import AnsibleLintRule, RulesCollection
Expand Down Expand Up @@ -193,6 +198,37 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]:
else our_vars[LINE_NUMBER_KEY]
)
raw_results.append(match_error)
roles = data.get("roles", [])
for role in roles:
if isinstance(role, AnsibleUnicode):
continue
role_fqcn = role.get("role", role.get("name"))
prefix = role_fqcn.split("/" if "/" in role_fqcn else ".")[-1]
for key in list(role.keys()):
if key not in PLAYBOOK_ROLE_KEYWORDS:
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})"
match_error.lineno = (
key.ansible_pos[1]
if isinstance(key, AnsibleUnicode)
else role[LINE_NUMBER_KEY]
)
raw_results.append(match_error)

our_vars = role.get("vars", {})
for key in our_vars:
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})"
match_error.lineno = (
key.ansible_pos[1]
if isinstance(key, AnsibleUnicode)
else our_vars[LINE_NUMBER_KEY]
)
raw_results.append(match_error)
if raw_results:
lines = file.content.splitlines()
for match in raw_results:
Expand Down Expand Up @@ -360,6 +396,28 @@ def test_var_naming_with_role_prefix(
for result in results:
assert result.tag == "var-naming[no-role-prefix]"

def test_var_naming_with_role_prefix_plays(
default_rules_collection: RulesCollection,
) -> None:
"""Test rule matches."""
results = Runner(
Lintable("examples/playbooks/role_vars_prefix_detection.yml"),
rules=default_rules_collection,
exclude_paths=["examples/roles/role_vars_prefix_detection"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exclude_paths was set because otherwise the runner picks up the violations from the role too, I presume this happens because the playbook invokes said role.

).run()
expected_errors = (
("var-naming[no-role-prefix]", 9),
("var-naming[no-role-prefix]", 12),
("var-naming[no-role-prefix]", 15),
("var-naming[no-role-prefix]", 25),
("var-naming[no-role-prefix]", 32),
("var-naming[no-role-prefix]", 45),
)
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_pattern() -> None:
"""Test rule matches."""
role_path = "examples/roles/var_naming_pattern/tasks/main.yml"
Expand Down
Loading