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

Add rule for variable naming #1518

Merged
merged 1 commit into from May 21, 2021
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
5 changes: 5 additions & 0 deletions .ansible-lint
Expand Up @@ -20,6 +20,11 @@ mock_roles:
# Enable checking of loop variable prefixes in roles
loop_var_prefix: "{role}_"

# Enforce variable names to follow pattern below, in addition to Ansible own
# requirements, like avoiding python identifiers. To disable add `var-naming`
# to skip_list.
# var_naming_pattern: "^[a-z_][a-z0-9_]*$"

use_default_rules: true
# Load custom rules from this specific folder
# rulesdir:
Expand Down
6 changes: 5 additions & 1 deletion src/ansiblelint/cli.py
Expand Up @@ -324,7 +324,11 @@ def merge_config(file_config: Dict[Any, Any], cli_config: Namespace) -> Namespac
'enable_list': [],
}

scalar_map = {"loop_var_prefix": None, "project_dir": "."}
scalar_map = {
"loop_var_prefix": None,
"project_dir": ".",
"var_naming_pattern": "^[a-z_][a-z0-9_]*$",
}

if not file_config:
# use defaults if we don't have a config file and the commandline
Expand Down
1 change: 1 addition & 0 deletions src/ansiblelint/config.py
Expand Up @@ -79,6 +79,7 @@
mock_modules=[],
mock_roles=[],
loop_var_prefix=None,
var_naming_pattern=None,
offline=False,
project_dir=".", # default should be valid folder (do not use None here)
extra_vars=None,
Expand Down
151 changes: 151 additions & 0 deletions src/ansiblelint/rules/VariableNamingRule.py
@@ -0,0 +1,151 @@
import keyword
import re
import sys
from functools import lru_cache
from typing import TYPE_CHECKING, Any, Dict, List, Optional, Pattern, Union

from ansiblelint.config import options
from ansiblelint.file_utils import Lintable
from ansiblelint.rules import AnsibleLintRule
from ansiblelint.utils import parse_yaml_from_file

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


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
"""


# properties/parameters are prefixed and postfixed with `__`
def is_property(k: str) -> bool:
"""Check if key is a property."""
return k.startswith('__') and k.endswith('__')


class VariableNamingRule(AnsibleLintRule):
id = 'var-naming'
base_msg = 'All variables should be named using only lowercase and underscores'
shortdesc = base_msg
description = 'All variables should be named using only lowercase and underscores'
severity = (
'MEDIUM' # ansible-lint displays severity when with --parseable-severity option
)
tags = ['idiom', 'experimental']
version_added = 'v5.0.10'

@lru_cache()
def re_pattern(self) -> Pattern[str]:
return re.compile(options.var_naming_pattern)

def is_invalid_variable_name(self, ident: str) -> bool:
"""Check if variable name is using right pattern."""
# Based on https://github.com/ansible/ansible/blob/devel/lib/ansible/utils/vars.py#L235
if not isinstance(ident, str):
return False

try:
ident.encode('ascii')
except UnicodeEncodeError:
return False

if keyword.iskeyword(ident):
return False

# previous tests should not be triggered as they would have raised a
# syntax-error when we loaded the files but we keep them here as a
# safety measure.
return not bool(self.re_pattern().match(ident))

def matchplay(
self, file: "Lintable", data: "odict[str, Any]"
) -> List["MatchError"]:
"""Return matches found for a specific playbook."""
results = []

# 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(
self.create_matcherror(
filename=file,
linenumber=our_vars['__line__'],
message="Play defines variable '"
+ key
+ "' within 'vars' section that violates variable naming standards",
)
)

return results

def matchtask(
self, task: Dict[str, Any], file: Optional[Lintable] = None
) -> Union[bool, str]:
"""Return matches for task based variables."""
# If the task uses the 'vars' section to set variables
our_vars = task.get('vars', {})
for key in our_vars.keys():
if self.is_invalid_variable_name(key):
return "Task defines variables within 'vars' section that violates variable naming standards"

# If the task uses the 'set_fact' module
ansible_module = task['action']['__ansible_module__']
ansible_action = task['action']
if ansible_module == 'set_fact':
for key in ansible_action.keys():
if self.is_invalid_variable_name(key):
return "Task uses 'set_fact' to define variables that violates variable naming standards"

# If the task registers a variable
registered_var = task.get('register', None)
if registered_var and self.is_invalid_variable_name(registered_var):
return "Task registers a variable that violates variable naming standards"

return False

def matchyaml(self, file: Lintable) -> List["MatchError"]:
"""Return matches for variables defined in vars files."""
results: List["MatchError"] = []
meta_data: Dict[str, Any] = {}

if 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(
self.create_matcherror(
filename=file,
# linenumber=vars['__line__'],
message="File defines variable '"
+ key
+ "' that violates variable naming standards",
)
)
else:
results.extend(super().matchyaml(file))
return results


# testing code to be loaded only with pytest or when executed the rule file
if "pytest" in sys.modules:

import pytest

@pytest.mark.parametrize(
'rule_runner', (VariableNamingRule,), indirect=['rule_runner']
)
def test_invalid_var_name_playbook(rule_runner: Any) -> None:
"""Test rule matches."""
results = rule_runner.run_playbook(FAIL_PLAY)
assert len(results) == 2
for result in results:
assert result.rule.id == VariableNamingRule.id
2 changes: 1 addition & 1 deletion test/TestRulesCollection.py
Expand Up @@ -128,4 +128,4 @@ def test_rules_id_format() -> None:
assert rule_id_re.match(
rule.id
), f"R rule id {rule.id} did not match our required format."
assert len(rules) == 40
assert len(rules) == 41