Skip to content

Commit

Permalink
Enable var-name rule to detect read-only variables (#3462)
Browse files Browse the repository at this point in the history
  • Loading branch information
ssbarnea committed May 18, 2023
1 parent 129a7fc commit a782eaf
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 1 deletion.
3 changes: 3 additions & 0 deletions examples/playbooks/vars/rule_var_naming_fail.yml
Expand Up @@ -9,3 +9,6 @@ CamelCaseButErrorIgnored: true # noqa: var-naming
assert: true # invalid due to being Python reserved keyword
é: true # invalid due to non-ascii character
hosts: true # invalid as being Ansible reserved name
role_name: boo # invalid as being Ansible special magic variable
ansible_facts: {} # special variable that we allow to be written
ansible_python_interpreter: python3 # special variable that we allow to be written
10 changes: 10 additions & 0 deletions src/ansiblelint/rules/var_naming.md
Expand Up @@ -9,6 +9,11 @@ alphabetic or underscore `_` character.
For more information see the [creating valid variable names][var-names] topic in
Ansible documentation and [Naming things (Good Practices for Ansible)][cop].

You should also be fully aware of [special variables][magic-vars], also known as
magic variables, especially as most of them can only be read. While Ansible will
just ignore any attempt to set them, the linter will notify the user, so they
would not be confused about a line that does not effectively do anything.

Possible errors messages:

- `var-naming[non-string]`: Variables names must be strings.
Expand All @@ -19,6 +24,7 @@ Possible errors messages:
- `var-naming[no-role-prefix]`: Variables names from within roles should use
`role_name_` as a prefix.
- `var-naming[no-reserved]`: Variables names must not be Ansible reserved names.
- `var-naming[read-only]`: This special variable is read-only.

## Settings

Expand All @@ -40,6 +46,7 @@ var_naming_pattern: "^[a-z_][a-z0-9_]*$"
ALL_CAPS: bar # <- Contains only uppercase characters.
v@r!able: baz # <- Contains special characters.
hosts: [] # <- hosts is an Ansible reserved name
role_name: boo # <-- invalid as being Ansible special magic variable
```

## Correct Code
Expand All @@ -53,8 +60,11 @@ var_naming_pattern: "^[a-z_][a-z0-9_]*$"
no_caps: bar # <- Does not contains uppercase characters.
variable: baz # <- Does not contain special characters.
my_hosts: [] # <- Does not use a reserved names.
my_role_name: boo
```

[cop]: https://redhat-cop.github.io/automation-good-practices/#_naming_things
[var-names]:
https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_variables.html#creating-valid-variable-names
[magic-vars]:
https://docs.ansible.com/ansible/latest/reference_appendices/special_variables.html
68 changes: 67 additions & 1 deletion src/ansiblelint/rules/var_naming.py
Expand Up @@ -33,6 +33,64 @@ class VariableNamingRule(AnsibleLintRule):
re_pattern_str = options.var_naming_pattern or "^[a-z_][a-z0-9_]*$"
re_pattern = re.compile(re_pattern_str)
reserved_names = get_reserved_names()
# List of special variables that should be treated as read-only. This list
# does not include connection variables, which we expect users to tune in
# specific cases.
# https://docs.ansible.com/ansible/latest/reference_appendices/special_variables.html
read_only_names = {
"ansible_check_mode",
"ansible_collection_name",
"ansible_config_file",
"ansible_dependent_role_names",
"ansible_diff_mode",
"ansible_forks",
"ansible_index_var",
"ansible_inventory_sources",
"ansible_limit",
"ansible_local", # special fact
"ansible_loop",
"ansible_loop_var",
"ansible_parent_role_names",
"ansible_parent_role_paths",
"ansible_play_batch",
"ansible_play_hosts",
"ansible_play_hosts_all",
"ansible_play_name",
"ansible_play_role_names",
"ansible_playbook_python",
"ansible_role_name",
"ansible_role_names",
"ansible_run_tags",
"ansible_search_path",
"ansible_skip_tags",
"ansible_verbosity",
"ansible_version",
"group_names",
"groups",
"hostvars",
"inventory_dir",
"inventory_file",
"inventory_hostname",
"inventory_hostname_short",
"omit",
"play_hosts",
"playbook_dir",
"role_name",
"role_names",
"role_path",
}

# These special variables are used by Ansible but we allow users to set
# them as they might need it in certain cases.
allowed_special_names = {
"ansible_facts",
"ansible_become_user",
"ansible_connection",
"ansible_host",
"ansible_python_interpreter",
"ansible_user",
"ansible_remote_tmp", # no included in docs
}

# pylint: disable=too-many-return-statements
def get_var_naming_matcherror(
Expand All @@ -49,7 +107,7 @@ def get_var_naming_matcherror(
rule=self,
)

if ident in ANNOTATION_KEYS:
if ident in ANNOTATION_KEYS or ident in self.allowed_special_names:
return None

try:
Expand All @@ -75,6 +133,13 @@ def get_var_naming_matcherror(
rule=self,
)

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,
)

# We want to allow use of jinja2 templating for variable names
if "{{" in ident:
return MatchError(
Expand Down Expand Up @@ -251,6 +316,7 @@ def test_invalid_var_name_varsfile(
("var-naming[no-keyword]", 9),
("var-naming[non-ascii]", 10),
("var-naming[no-reserved]", 11),
("var-naming[read-only]", 12),
)
assert len(results) == len(expected_errors)
for idx, result in enumerate(results):
Expand Down

0 comments on commit a782eaf

Please sign in to comment.