Skip to content

Commit 649f819

Browse files
committed
import ansible rules from ocp
- from repository: [ansible-collection-redhatci-ocp](https://github.com/redhatci/ansible-collection-redhatci-ocp/) paths: - `hack/rules/*.py` - `.ansible-lint` - upd metadata + filenames - disable some rules Signed-off-by: Maxim Kovgan <[email protected]>
1 parent 37140f4 commit 649f819

File tree

3 files changed

+339
-2
lines changed

3 files changed

+339
-2
lines changed

.ansible-lint

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,20 @@
22
skip_list:
33
- var-naming[no-role-prefix]
44

5+
# rulesdir:
6+
# - hack/rules
7+
8+
use_default_rules: true
9+
10+
display_relative_path: true
11+
512
# Define paths or files to ignore
613
exclude_paths:
7-
- "tests/dast"
8-
- "*collections*"
14+
- collections
15+
- .git*
16+
- .venv*
17+
- hack
18+
- plugins
19+
- roles/**/molecule
20+
- tests
21+
- tests/dast

hack/rules/openshift_kni.md

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
# openshift-kni rules
2+
3+
This rule checks for compliance with some coding conventions in the OCP
4+
collection
5+
6+
The `openshift-kni` rule has the following checks:
7+
8+
- `openshift-kni[no-role-prefix]` - Variables used within a role should have a
9+
prefix related to the role. This includes:
10+
1. Variables passed to `{include,import}_role` and `{include_import}_tasks`
11+
should be named `$prefix_$var`
12+
1. Facts saved from within roles should be named `$prefix_$fact`
13+
1. Variables registered from tasks to use in subsequent tasks should be named
14+
`_$prefix_$var` - notice the extra underscore to signal this variable is
15+
"private"
16+
17+
The rules to figure out the prefix are:
18+
1. If the role name is $SHORT (where $SHORT is < 6 characters) use the whole
19+
name
20+
1. If not, split the role name by underscores into words
21+
1. If there's a single word:
22+
- Check for digits pattern `<prefix><digits><suffix>` (e.g., `junit2json`)
23+
and create acronym using first letter of prefix + digits + first letter
24+
of suffix
25+
- Otherwise, use the first $SHORT characters
26+
1. Finally, if there are 2 or more words, build an acronym from the split
27+
words
28+
1. It is also possible to specify the full prefix of the role (doesn't matter
29+
if it's "too long") or prefix it with `global_` if the variable is
30+
intended for global scope.
31+
32+
Examples of valid prefixes:
33+
34+
| Role Name | Computed Prefix | Full Prefix | Global Prefix |
35+
|-------------------------|-----------------|--------------------------|---------------|
36+
| `sno` | `sno_` | `sno_` | `global_` |
37+
| `installer` | `instal_` | `installer_` | `global_` |
38+
| `junit2json` | `j2j_` | `junit2json_` | `global_` |
39+
| `web3server` | `w3s_` | `web3server_` | `global_` |
40+
| `validate_http_store` | `vhs_` | `validate_http_store_` | `global_` |
41+
| `oc_client_install` | `oci_` | `oc_client_install_` | `global_` |
42+
43+
!!! note
44+
45+
This rule overlaps with the `var-naming[no-role-prefix]` stock rule, disable
46+
it in your `.ansible-lint` config file or by passing a -x flag
47+
48+
## Problematic Code
49+
50+
```yaml
51+
---
52+
- name: Example playbook with bad variable naming
53+
hosts: all
54+
tasks:
55+
- name: Include the OC Client Install role
56+
ansible.builtin.include_role:
57+
name: oc_client_install
58+
vars:
59+
version: "4.15.0" # <-- Bad: no prefix
60+
install_dir: "/usr/local/bin" # <-- Bad: no prefix
61+
62+
- name: Include role with digits in name
63+
ansible.builtin.include_role:
64+
name: junit2json
65+
vars:
66+
config_file: "config.xml" # <-- Bad: no prefix
67+
68+
- name: Set facts without prefix
69+
ansible.builtin.set_fact:
70+
ocp_version: "4.15.0" # <-- Bad: no prefix for role context
71+
72+
- name: Command with bad register
73+
ansible.builtin.command: echo "test"
74+
register: result # <-- Bad: private var without prefix
75+
...
76+
```
77+
78+
## Correct Code
79+
80+
```yaml
81+
---
82+
- name: Example playbook with correct variable naming
83+
hosts: all
84+
tasks:
85+
- name: Include the OC Client Install role
86+
ansible.builtin.include_role:
87+
name: oc_client_install
88+
vars:
89+
oci_version: "4.15.0" # <-- Good: computed prefix
90+
oc_client_install_install_dir: "/usr/local/bin" # <-- Good: full prefix
91+
global_cleanup: true # <-- Good: global variable
92+
93+
- name: Include role with digits in name
94+
ansible.builtin.include_role:
95+
name: junit2json
96+
vars:
97+
j2j_config_file: "config.xml" # <-- Good: computed prefix with digits
98+
junit2json_output_dir: "/tmp" # <-- Good: full prefix
99+
100+
- name: Set facts with proper prefix
101+
ansible.builtin.set_fact:
102+
oci_ocp_version: "4.15.0" # <-- Good: role context prefix
103+
global_cluster_info: "some_value" # <-- Good: global scope
104+
105+
- name: Command with good register
106+
ansible.builtin.command: echo "test"
107+
register: _oci_command_result # <-- Good: private var with prefix
108+
...
109+
```
110+
111+
## Private Variables for Registered Tasks
112+
113+
Variables registered from tasks should use a leading underscore to indicate they are private to the role:
114+
115+
```yaml
116+
- name: Get cluster version
117+
ansible.builtin.command: oc version
118+
register: _oci_version_output # <-- Good: private with computed prefix
119+
120+
- name: Check installation status
121+
ansible.builtin.stat:
122+
path: /usr/local/bin/oc
123+
register: _oc_client_install_binary_stat # <-- Good: private with full prefix
124+
```

hack/rules/openshift_kni.py

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
# Copyright (C) 2024 Red Hat, Inc.
2+
#
3+
# Author: Jorge A Gallegos <[email protected]>
4+
#
5+
# Licensed under the Apache License, Version 2.0 (the "License"); you may
6+
# not use this file except in compliance with the License. You may obtain
7+
# a copy of the License at
8+
#
9+
# http://www.apache.org/licenses/LICENSE-2.0
10+
#
11+
# Unless required by applicable law or agreed to in writing, software
12+
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
13+
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
14+
# License for the specific language governing permissions and limitations
15+
# under the License.
16+
"""
17+
Specific rule definitions for coding conventions in Redhat OCP KNI
18+
"""
19+
20+
import logging
21+
import re
22+
23+
from ansiblelint.constants import ANNOTATION_KEYS, LINE_NUMBER_KEY
24+
from ansiblelint.errors import MatchError
25+
from ansiblelint.rules import AnsibleLintRule
26+
from ansiblelint.text import has_jinja
27+
from ansiblelint.file_utils import Lintable
28+
from ansiblelint.utils import Task
29+
30+
_logger = logging.getLogger(__name__)
31+
32+
33+
class CollectionNamingConvention(AnsibleLintRule):
34+
"""Rules for the RedHat OCP KNI naming convention"""
35+
36+
id = "openshift-kni"
37+
description = "RedHat OCP KNI naming convention"
38+
severity = "MEDIUM"
39+
tags = ["experimental", "idiom", "redhat", "openshift", "openshift-kni"]
40+
version_added = "historic"
41+
needs_raw_task = True
42+
# These special variables are used by Ansible but we allow users to set
43+
# them as they might need it in certain cases.
44+
allowed_special_names = {
45+
"ansible_facts",
46+
"ansible_become_user",
47+
"ansible_connection",
48+
"ansible_host",
49+
"ansible_python_interpreter",
50+
"ansible_user",
51+
"ansible_remote_tmp", # no included in docs
52+
}
53+
54+
def get_var_naming_matcherror(
55+
self, ident: str, *, role: str, private: bool = False
56+
) -> MatchError | None:
57+
"""Return a MatchError if the variable name doesn't match prefix."""
58+
59+
# don't try to match private keys
60+
if ident.startswith("__") and ident.endswith("__"):
61+
return None
62+
63+
# don't try to match special names either
64+
if ident in ANNOTATION_KEYS or ident in self.allowed_special_names:
65+
return None
66+
67+
# if the role is templated, we can't possibly know what prefix to use
68+
if has_jinja(role):
69+
_logger.warning(f"Role name is templated, can't analyze (role: {role})")
70+
return None
71+
72+
# finally, if we can't figure out the role name we can't figure out the
73+
# prefix
74+
if role.strip() == "":
75+
_logger.debug(f"Passed empty role name for {ident}")
76+
return None
77+
78+
#####################
79+
# PREFIX HEURISTICS #
80+
# vvvvvvvvvvvvvvvvv #
81+
#####################
82+
possible_prefix = set()
83+
# https://www.researchgate.net/figure/Average-word-length-in-the-English-language-Different-colours-indicate-the-results-for_fig1_230764201
84+
SHORT = 6
85+
# compute the prefix
86+
# if the role name is really short, just use the role name as prefix
87+
if len(role) < SHORT:
88+
computed_prefix = role
89+
else:
90+
parts = role.split("_")
91+
if len(parts) == 1:
92+
# if it's a single word, check for digits pattern first
93+
digits_match = re.search(r'^([a-zA-Z]+)(\d+)([a-zA-Z]+)$', role)
94+
if digits_match:
95+
# Special case: role contains digits in format <prefix><digits><suffix>
96+
prefix_part, digits_part, suffix_part = digits_match.groups()
97+
computed_prefix = f"{prefix_part[0] if prefix_part else ''}{digits_part}{suffix_part[0] if suffix_part else ''}"
98+
else:
99+
# Default: use the first few chars from it
100+
computed_prefix = parts[0][:SHORT]
101+
else:
102+
# else, use an acronym
103+
computed_prefix = "".join(_[0] for _ in parts)
104+
105+
# registering within roles should require "privatizing" variables
106+
if private:
107+
possible_prefix.add(f"_{role}")
108+
possible_prefix.add(f"_{computed_prefix}")
109+
else:
110+
possible_prefix.add("global")
111+
possible_prefix.add(f"{role}")
112+
possible_prefix.add(f"{computed_prefix}")
113+
114+
#####################
115+
# ^^^^^^^^^^^^^^^^^ #
116+
# PREFIX HEURISTICS #
117+
#####################
118+
119+
# If variable starts with any of the allowed prefixes, this is a valid
120+
# variable name
121+
for prefix in possible_prefix:
122+
if ident.startswith(f"{prefix}_"):
123+
return None
124+
125+
# fail if the task didnt' start with any of the allowed prefixes
126+
return MatchError(
127+
tag="openshift-kni[no-role-prefix]",
128+
message="Variable names should use a prefix related to the role. "
129+
+ f"({possible_prefix} are possible)",
130+
rule=self,
131+
)
132+
133+
def matchtask(
134+
self,
135+
task: Task,
136+
file: Lintable | None = None,
137+
) -> list[MatchError]:
138+
"""Return matches for task based variables."""
139+
results: list[MatchError] = []
140+
ansible_module = task["action"]["__ansible_module__"]
141+
142+
filename = "" if file is None else str(file.path)
143+
role_name = ""
144+
if file and file.parent and file.parent.kind == "role":
145+
role_name = file.parent.path.name
146+
147+
# If we're importing tasks
148+
if ansible_module in ("include_tasks", "import_tasks"):
149+
# If the task uses the 'vars' section to set variables
150+
our_vars = task.get("vars", {})
151+
for key in our_vars:
152+
match_error = self.get_var_naming_matcherror(key, role=role_name)
153+
if match_error:
154+
match_error.filename = filename
155+
match_error.lineno = our_vars[LINE_NUMBER_KEY]
156+
match_error.message += f" (vars: {key})"
157+
results.append(match_error)
158+
159+
# if the task imports a role, then prefix should match the target role
160+
elif ansible_module in ("include_role", "import_role"):
161+
ext_role = task.get("action")["name"].split(".")[-1]
162+
our_vars = task.get("vars", {})
163+
for key in our_vars:
164+
match_error = self.get_var_naming_matcherror(key, role=ext_role)
165+
if match_error:
166+
match_error.filename = filename
167+
match_error.lineno = our_vars[LINE_NUMBER_KEY]
168+
match_error.message += f" (vars: {key})"
169+
results.append(match_error)
170+
171+
# If the task uses the 'set_fact' module
172+
elif ansible_module == "set_fact":
173+
for key in filter(
174+
lambda x: isinstance(x, str)
175+
and not x.startswith("__")
176+
and x != "cacheable",
177+
task["action"].keys(),
178+
):
179+
match_error = self.get_var_naming_matcherror(key, role=role_name)
180+
if match_error:
181+
match_error.filename = filename
182+
match_error.lineno = task["action"][LINE_NUMBER_KEY]
183+
match_error.message += f" (set_fact: {key})"
184+
results.append(match_error)
185+
else:
186+
pass
187+
188+
# If the task registers a variable
189+
registered_var = task.get("register", None)
190+
if registered_var:
191+
match_error = self.get_var_naming_matcherror(
192+
registered_var, role=role_name, private=True
193+
)
194+
if match_error:
195+
match_error.message += f" (register: {registered_var})"
196+
match_error.filename = filename
197+
match_error.lineno = task[LINE_NUMBER_KEY]
198+
results.append(match_error)
199+
200+
return results

0 commit comments

Comments
 (0)