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 var-naming rule to show line numbers and apply noqa #2090

Merged
merged 2 commits into from
May 3, 2022
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.
Jump to
Jump to file
Failed to load files.
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 @@ -156,7 +156,7 @@ jobs:
WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:TOX_PARALLEL_NO_SPINNER
# Number of expected test passes, safety measure for accidental skip of
# tests. Update value if you add/remove tests.
PYTEST_REQPASS: 608
PYTEST_REQPASS: 609

steps:
- name: Activate WSL1
Expand Down
78 changes: 71 additions & 7 deletions src/ansiblelint/rules/var_naming.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,49 @@
import sys
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Union

from ansible.parsing.yaml.objects import AnsibleUnicode

from ansiblelint.config import options
from ansiblelint.file_utils import Lintable
from ansiblelint.rules import AnsibleLintRule
from ansiblelint.skip_utils import get_rule_skips_from_line
from ansiblelint.utils import LINE_NUMBER_KEY, parse_yaml_from_file

if TYPE_CHECKING:
from ansiblelint.constants import odict
from ansiblelint.errors import MatchError


FAIL_PLAY = """
# Should raise var-naming at line [4, 8].
FAIL_PLAY = """---
- hosts: localhost
vars:
CamelCaseIsBad: false # invalid
this_is_valid: # valid because content is a dict, not a variable
CamelCase: ...
ALL_CAPS: ...
ALL_CAPS_ARE_BAD_TOO: ... # invalid
CamelCaseButErrorIgnored: true # noqa: var-naming

tasks:
- name: foo
ansible.builtin.set_fact:
"{{ 'test_' }}var": "value" # valid
- name: bar
ansible.builtin.set_fact:
CamelCaseButErrorIgnored: true # noqa: var-naming
"""


# Should raise var-naming at line [2, 6].
FAIL_VARS = """---
CamelCaseIsBad: false # invalid
this_is_valid: # valid because content is a dict, not a variable
CamelCase: ...
ALL_CAPS: ...
ALL_CAPS_ARE_BAD_TOO: ... # invalid
"{{ 'test_' }}var": "value" # valid
CamelCaseButErrorIgnored: true # noqa: var-naming
"""


Expand Down Expand Up @@ -72,21 +92,31 @@ def matchplay(
self, file: "Lintable", data: "odict[str, Any]"
) -> List["MatchError"]:
"""Return matches found for a specific playbook."""
results = []
results: List["MatchError"] = []
raw_results: List["MatchError"] = []

# If the Play uses the 'vars' section to set variables
our_vars = data.get("vars", {})
for key in our_vars.keys():
if self.is_invalid_variable_name(key):
results.append(
raw_results.append(
self.create_matcherror(
filename=file,
linenumber=our_vars[LINE_NUMBER_KEY],
linenumber=key.ansible_pos[1]
if isinstance(key, AnsibleUnicode)
else our_vars[LINE_NUMBER_KEY],
message="Play defines variable '"
+ key
+ "' within 'vars' section that violates variable naming standards",
)
)
if raw_results:
lines = file.content.splitlines()
for match in raw_results:
# linenumber starts with 1, not zero
skip_list = get_rule_skips_from_line(lines[match.linenumber - 1])
if match.rule.id not in skip_list and match.tag not in skip_list:
results.append(match)

return results

Expand Down Expand Up @@ -118,21 +148,29 @@ def matchtask(
def matchyaml(self, file: Lintable) -> List["MatchError"]:
"""Return matches for variables defined in vars files."""
results: List["MatchError"] = []
meta_data: Dict[str, Any] = {}
raw_results: List["MatchError"] = []
meta_data: Dict[AnsibleUnicode, Any] = {}

if str(file.kind) == "vars":
meta_data = parse_yaml_from_file(str(file.path))
for key in meta_data.keys():
if self.is_invalid_variable_name(key):
results.append(
raw_results.append(
self.create_matcherror(
filename=file,
# linenumber=vars[LINE_NUMBER_KEY],
linenumber=key.ansible_pos[1],
message="File defines variable '"
+ key
+ "' that violates variable naming standards",
)
)
if raw_results:
lines = file.content.splitlines()
for match in raw_results:
# linenumber starts with 1, not zero
skip_list = get_rule_skips_from_line(lines[match.linenumber - 1])
if match.rule.id not in skip_list and match.tag not in skip_list:
results.append(match)
else:
results.extend(super().matchyaml(file))
return results
Expand All @@ -154,3 +192,29 @@ def test_invalid_var_name_playbook(rule_runner: RunFromText) -> None:
assert len(results) == 2
for result in results:
assert result.rule.id == VariableNamingRule.id

# list unexpected error lines or non-matching error lines
expected_error_lines = [4, 8]
lines = [i.linenumber for i in results]
error_lines_difference = list(
set(expected_error_lines).symmetric_difference(set(lines))
)
assert len(error_lines_difference) == 0

@pytest.mark.parametrize(
"rule_runner", (VariableNamingRule,), indirect=["rule_runner"]
)
def test_invalid_var_name_varsfile(rule_runner: RunFromText) -> None:
"""Test rule matches."""
results = rule_runner.run_role_defaults_main(FAIL_VARS)
assert len(results) == 2
for result in results:
assert result.rule.id == VariableNamingRule.id

# list unexpected error lines or non-matching error lines
expected_error_lines = [2, 6]
lines = [i.linenumber for i in results]
error_lines_difference = list(
set(expected_error_lines).symmetric_difference(set(lines))
)
assert len(error_lines_difference) == 0
11 changes: 11 additions & 0 deletions src/ansiblelint/testing/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,17 @@ def run_role_meta_main(self, meta_main_text: str) -> List[MatchError]:
shutil.rmtree(role_path)
return results

def run_role_defaults_main(self, defaults_main_text: str) -> List[MatchError]:
"""Lints received text as vars file in defaults."""
role_path = tempfile.mkdtemp(prefix="role_")
defaults_path = os.path.join(role_path, "defaults")
os.makedirs(defaults_path)
with open(os.path.join(defaults_path, "main.yml"), "w", encoding="utf-8") as fh:
fh.write(defaults_main_text)
results = self._call_runner(role_path)
shutil.rmtree(role_path)
return results


def run_ansible_lint(
*argv: str,
Expand Down
3 changes: 2 additions & 1 deletion test/test_skiputils.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,5 @@ def test_get_rule_skips_from_line(line: str, expected: str) -> None:
def test_playbook_noqa(default_text_runner: RunFromText) -> None:
"""Check that noqa is properly taken into account on vars and tasks."""
results = default_text_runner.run_playbook(PLAYBOOK_WITH_NOQA)
assert len(results) == 2
# Should raise error at "SOME_VAR".
assert len(results) == 1