Skip to content

Commit 96ad2c2

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` Signed-off-by: Maxim Kovgan <[email protected]>
1 parent 37140f4 commit 96ad2c2

File tree

3 files changed

+279
-2
lines changed

3 files changed

+279
-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/redhat_ci.md

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
# redhat-ci
2+
3+
This rule checks for compliance with some coding conventions in the OCP
4+
collection
5+
6+
The `redhat-ci` rule has the following checks:
7+
8+
- `redhat-ci[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, use the first $SHORT characters
22+
1. Finally, if there are 2 or more words, build an acronym from the split
23+
words
24+
1. It is also possible to specify the full prefix of the role (doesn't matter
25+
if it's "too long") or prefix it with `global_` if the variable is
26+
intended for global scope.
27+
28+
Examples of valid prefixes:
29+
30+
| Role Name | Prefix |
31+
|-------------------------|---------------|
32+
| `sno` | `sno_` |
33+
| `installer` | `insta_` |
34+
| `validate_http_store` | `vhs_` |
35+
| `node_prep` | `node_prep_` |
36+
| `provision_registry` | `global_var` |
37+
38+
!!! note
39+
40+
This rule overlaps with the var-naming[no-role-prefix] stock rule, disable
41+
it in your .ansible-lint config file or by passing a -x flag
42+
43+
## Problematic Code
44+
45+
```yaml
46+
---
47+
- name: Example playbook
48+
hosts: all
49+
tasks:
50+
- name: Include the Node Prep role
51+
ansible.builtin.include_role:
52+
name: node_prep
53+
vars:
54+
org_id: my_org # <-- The variable passed does not have a prefix
55+
my_global_var: some_value # <-- This variable is "exported"
56+
...
57+
```
58+
59+
## Correct Code
60+
61+
```yaml
62+
---
63+
- name: Example playbook
64+
hosts: all
65+
tasks:
66+
- name: Include the Node Prep role
67+
ansible.builtin.include_role:
68+
name: node_prep
69+
vars:
70+
np_org_id: my_org # <-- Variable has a prefix related to the role
71+
global_var: some_value # <-- Variable is correctly identified as global
72+
...
73+
```

hack/rules/redhat_ci.py

Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
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 CI
18+
"""
19+
20+
import logging
21+
22+
from ansiblelint.constants import ANNOTATION_KEYS, LINE_NUMBER_KEY
23+
from ansiblelint.errors import MatchError
24+
from ansiblelint.rules import AnsibleLintRule
25+
from ansiblelint.text import has_jinja
26+
from ansiblelint.utils import Lintable, Task
27+
28+
_logger = logging.getLogger(__name__)
29+
30+
31+
class CollectionNamingConvention(AnsibleLintRule):
32+
"""Rules for the RedHat OCP Collection naming convention"""
33+
34+
id = "redhat-ci"
35+
description = "RedHat OCP Collection naming convention"
36+
severity = "MEDIUM"
37+
tags = ["experimental", "idiom", "redhat"]
38+
varsion_added = "historic"
39+
needs_raw_task = True
40+
# These special variables are used by Ansible but we allow users to set
41+
# them as they might need it in certain cases.
42+
allowed_special_names = {
43+
"ansible_facts",
44+
"ansible_become_user",
45+
"ansible_connection",
46+
"ansible_host",
47+
"ansible_python_interpreter",
48+
"ansible_user",
49+
"ansible_remote_tmp", # no included in docs
50+
}
51+
52+
def get_var_naming_matcherror(
53+
self, ident: str, *, role: str, private: bool = False
54+
) -> MatchError | None:
55+
"""Return a MatchError if the variable name doesn't match prefix."""
56+
57+
# don't try to match private keys
58+
if ident.startswith("__") and ident.endswith("__"):
59+
return None
60+
61+
# don't try to match special names either
62+
if ident in ANNOTATION_KEYS or ident in self.allowed_special_names:
63+
return None
64+
65+
# if the role is templated, we can't possibly know what prefix to use
66+
if has_jinja(role):
67+
_logger.warning(f"Role name is templated, can't analyze (role: {role})")
68+
return None
69+
70+
# finally, if we can't figure out the role name we can't figure out the
71+
# prefix
72+
if role.strip() == "":
73+
_logger.debug(f"Passed empty role name for {ident}")
74+
return None
75+
76+
#####################
77+
# PREFIX HEURISTICS #
78+
# vvvvvvvvvvvvvvvvv #
79+
#####################
80+
possible_prefix = set()
81+
# https://www.researchgate.net/figure/Average-word-length-in-the-English-language-Different-colours-indicate-the-results-for_fig1_230764201
82+
SHORT = 6
83+
# compute the prefix
84+
# if the role name is really short, just use the role name as prefix
85+
if len(role) < SHORT:
86+
computed_prefix = role
87+
else:
88+
parts = role.split("_")
89+
if len(parts) == 1:
90+
# if it's a single word, use the first few chars from it
91+
computed_prefix = parts[0][:SHORT]
92+
else:
93+
# else, use an acronym
94+
computed_prefix = "".join(_[0] for _ in parts)
95+
96+
# registering within roles should require "privatizing" variables
97+
if private:
98+
possible_prefix.add(f"_{role}")
99+
possible_prefix.add(f"_{computed_prefix}")
100+
else:
101+
possible_prefix.add("global")
102+
possible_prefix.add(f"{role}")
103+
possible_prefix.add(f"{computed_prefix}")
104+
105+
#####################
106+
# ^^^^^^^^^^^^^^^^^ #
107+
# PREFIX HEURISTICS #
108+
#####################
109+
110+
# If variable starts with any of the allowed prefixes, this is a valid
111+
# variable name
112+
for prefix in possible_prefix:
113+
if ident.startswith(f"{prefix}_"):
114+
return None
115+
116+
# fail if the task didnt' start with any of the allowed prefixes
117+
return MatchError(
118+
tag="redhat-ci[no-role-prefix]",
119+
message="Variable names should use a prefix related to the role. "
120+
+ f"({possible_prefix} are possible)",
121+
rule=self,
122+
)
123+
124+
def matchtask(
125+
self,
126+
task: Task,
127+
file: Lintable | None = None,
128+
) -> list[MatchError]:
129+
"""Return matches for task based variables."""
130+
results: list[MatchError] = []
131+
ansible_module = task["action"]["__ansible_module__"]
132+
133+
filename = "" if file is None else str(file.path)
134+
role_name = ""
135+
if file and file.parent and file.parent.kind == "role":
136+
role_name = file.parent.path.name
137+
138+
# If we're importing tasks
139+
if ansible_module in ("include_tasks", "import_tasks"):
140+
# If the task uses the 'vars' section to set variables
141+
our_vars = task.get("vars", {})
142+
for key in our_vars:
143+
match_error = self.get_var_naming_matcherror(key, role=role_name)
144+
if match_error:
145+
match_error.filename = filename
146+
match_error.lineno = our_vars[LINE_NUMBER_KEY]
147+
match_error.message += f" (vars: {key})"
148+
results.append(match_error)
149+
150+
# if the task imports a role, then prefix should match the target role
151+
elif ansible_module in ("include_role", "import_role"):
152+
ext_role = task.get("action")["name"].split(".")[-1]
153+
our_vars = task.get("vars", {})
154+
for key in our_vars:
155+
match_error = self.get_var_naming_matcherror(key, role=ext_role)
156+
if match_error:
157+
match_error.filename = filename
158+
match_error.lineno = our_vars[LINE_NUMBER_KEY]
159+
match_error.message += f" (vars: {key})"
160+
results.append(match_error)
161+
162+
# If the task uses the 'set_fact' module
163+
elif ansible_module == "set_fact":
164+
for key in filter(
165+
lambda x: isinstance(x, str)
166+
and not x.startswith("__")
167+
and x != "cacheable",
168+
task["action"].keys(),
169+
):
170+
match_error = self.get_var_naming_matcherror(key, role=role_name)
171+
if match_error:
172+
match_error.filename = filename
173+
match_error.lineno = task["action"][LINE_NUMBER_KEY]
174+
match_error.message += f" (set_fact: {key})"
175+
results.append(match_error)
176+
else:
177+
pass
178+
179+
# If the task registers a variable
180+
registered_var = task.get("register", None)
181+
if registered_var:
182+
match_error = self.get_var_naming_matcherror(
183+
registered_var, role=role_name, private=True
184+
)
185+
if match_error:
186+
match_error.message += f" (register: {registered_var})"
187+
match_error.filename = filename
188+
match_error.lineno = task[LINE_NUMBER_KEY]
189+
results.append(match_error)
190+
191+
return results

0 commit comments

Comments
 (0)