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 subdirectories to be part of the task prefix #4143

Merged
merged 1 commit into from
May 13, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
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 @@ -73,7 +73,7 @@ jobs:
env:
# Number of expected test passes, safety measure for accidental skip of
# tests. Update value if you add/remove tests.
PYTEST_REQPASS: 865
PYTEST_REQPASS: 868
steps:
- uses: actions/checkout@v4
with:
Expand Down
7 changes: 7 additions & 0 deletions examples/playbooks/tasks/main.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
- name: This is correct
ansible.builtin.assert:
that: true
- name: A phony prefix | This is also correct
ansible.builtin.assert:
that: true
10 changes: 10 additions & 0 deletions examples/playbooks/tasks/partial_prefix/foo.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
- name: foo | This prefix is incomplete
ansible.builtin.assert:
that: true
- name: partial_prefix | This prefix is incomplete
ansible.builtin.assert:
that: true
- name: partial_prefix | foo | This is correct
ansible.builtin.assert:
that: true
10 changes: 10 additions & 0 deletions examples/playbooks/tasks/partial_prefix/main.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
---
- name: partial_prefix | main | This is correct
ansible.builtin.assert:
that: true
- name: main | This prefix is incomplete
ansible.builtin.assert:
that: true
- name: partial_prefix | This prefix is incomplete
ansible.builtin.assert:
that: true
15 changes: 12 additions & 3 deletions src/ansiblelint/rules/name.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,21 @@ If you want to ignore some of the messages above, you can add any of them to the

## name[prefix]

This rule applies only to included task files that are not named `main.yml`. It
suggests adding the stem of the file as a prefix to the task name.
This rule applies only to included task files that are not named `main.yml` or
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think this sentence can be a tad confusing until the next two paragraphs are read. I am open to suggestions here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cidrblock WDYT?

I personally do not have a strong opinion here, but I kinda like the idea of not forcing people to add | main | extra prefix for tasks/mains.yml which is like 95% of them.

are embedded within subdirectories. It suggests adding the stems of the file
path as a prefix to the task name.

For example, if you have a task named `Restart server` inside a file named
`tasks/deploy.yml`, this rule suggests renaming it to `deploy | Restart server`,
so it would be easier to identify where it comes from.
so it would be easier to identify where it comes from. If the file was named
`tasks/main.yml`, then the rule would have no effect.

For task files that are embedded within subdirectories, these subdirectories
will also be appended as part of the prefix. For example, if you have a task
named `Terminate server` inside a file named `tasks/foo/destroy.yml`, this rule
suggests renaming it to `foo | destroy | Terminate server`. If the file was
named `tasks/foo/main.yml` then the rule would recommend renaming the task to
`foo | main | Terminate server`.

For the moment, this sub-rule is just an **opt-in**, so you need to add it to
your `enable_list` to activate it.
Expand Down
88 changes: 84 additions & 4 deletions src/ansiblelint/rules/name.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
import sys
from typing import TYPE_CHECKING, Any

import wcmatch.pathlib
import wcmatch.wcmatch

from ansiblelint.constants import LINE_NUMBER_KEY
from ansiblelint.file_utils import Lintable
from ansiblelint.rules import AnsibleLintRule, TransformMixin
Expand Down Expand Up @@ -125,10 +128,15 @@
# stage one check prefix
effective_name = name
if self._collection and lintable:
prefix = self._collection.options.task_name_prefix.format(
stem=lintable.path.stem,
)
if lintable.kind == "tasks" and lintable.path.stem != "main":
full_stem = self._find_full_stem(lintable)
stems = [
self._collection.options.task_name_prefix.format(stem=stem)
for stem in wcmatch.pathlib.PurePath(
full_stem,
).parts
]
prefix = "".join(stems)
if lintable.kind == "tasks" and full_stem != "main":
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think any task file embedded within a subdirectory should be required to prefix each task with its stems. Even if the file was named main.yml.

if not name.startswith(prefix):
# For the moment in order to raise errors this rule needs to be
# enabled manually. Still, we do allow use of prefixes even without
Expand Down Expand Up @@ -170,6 +178,35 @@
)
return results

def _find_full_stem(self, lintable: Lintable) -> str:
lintable_dir = wcmatch.pathlib.PurePath(lintable.dir)
stem = lintable.path.stem
kind = str(lintable.kind)

stems = [lintable_dir.name]
lintable_dir = lintable_dir.parent
pathex = lintable_dir / stem
for entry in self.options.kinds:
for key, value in entry.items():
if kind == key:
glob = value

while pathex.globmatch(
glob,
flags=(
wcmatch.pathlib.GLOBSTAR
| wcmatch.pathlib.BRACE
| wcmatch.pathlib.DOTGLOB
),
):
stems.insert(0, lintable_dir.name)
lintable_dir = lintable_dir.parent
pathex = lintable_dir / stem

Check warning on line 204 in src/ansiblelint/rules/name.py

View check run for this annotation

Codecov / codecov/patch

src/ansiblelint/rules/name.py#L202-L204

Added lines #L202 - L204 were not covered by tests

if stems[0].startswith(kind):
del stems[0]
return str(wcmatch.pathlib.PurePath(*stems, stem))

def transform(
self,
match: MatchError,
Expand Down Expand Up @@ -244,6 +281,19 @@
errs = bad_runner.run()
assert len(errs) == 5

def test_name_prefix_positive(config_options: Options) -> None:
"""Positive test for name[prefix]."""
config_options.enable_list = ["name[prefix]"]
collection = RulesCollection(options=config_options)
collection.register(NameRule())
success = Lintable(
"examples/playbooks/tasks/main.yml",
kind="tasks",
)
good_runner = Runner(success, rules=collection)
results = good_runner.run()
assert len(results) == 0

def test_name_prefix_negative(config_options: Options) -> None:
"""Negative test for name[missing]."""
config_options.enable_list = ["name[prefix]"]
Expand All @@ -261,6 +311,36 @@
assert results[1].tag == "name[prefix]"
assert results[2].tag == "name[prefix]"

def test_name_prefix_negative_2(config_options: Options) -> None:
"""Negative test for name[prefix]."""
config_options.enable_list = ["name[prefix]"]
collection = RulesCollection(options=config_options)
collection.register(NameRule())
failure = Lintable(
"examples/playbooks/tasks/partial_prefix/foo.yml",
kind="tasks",
)
bad_runner = Runner(failure, rules=collection)
results = bad_runner.run()
assert len(results) == 2
assert results[0].tag == "name[prefix]"
assert results[1].tag == "name[prefix]"

def test_name_prefix_negative_3(config_options: Options) -> None:
"""Negative test for name[prefix]."""
config_options.enable_list = ["name[prefix]"]
collection = RulesCollection(options=config_options)
collection.register(NameRule())
failure = Lintable(
"examples/playbooks/tasks/partial_prefix/main.yml",
kind="tasks",
)
bad_runner = Runner(failure, rules=collection)
results = bad_runner.run()
assert len(results) == 2
assert results[0].tag == "name[prefix]"
assert results[1].tag == "name[prefix]"

def test_rule_name_lowercase() -> None:
"""Negative test for a task that starts with lowercase."""
collection = RulesCollection()
Expand Down
8 changes: 4 additions & 4 deletions test/test_file_path_evaluation.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@
"tasks/subtasks/subtask_1.yml": textwrap.dedent(
"""\
---
- name: subtask_1 | From subtask 1 import subtask 2
- name: subtasks | subtask_1 | From subtask 1 import subtask 2
ansible.builtin.import_tasks: tasks/subtasks/subtask_2.yml
""",
),
"tasks/subtasks/subtask_2.yml": textwrap.dedent(
"""\
---
- name: subtask_2 | From subtask 2 do something
- name: subtasks | subtask_2 | From subtask 2 do something
debug: # <-- expected to raise fqcn[action-core]
msg: |
Something...
Expand Down Expand Up @@ -87,14 +87,14 @@
"tasks/subtasks/subtask_1.yml": textwrap.dedent(
"""\
---
- name: subtask_1 | From subtask 1 import subtask 2
- name: subtasks | subtask_1 | From subtask 1 import subtask 2
ansible.builtin.include_tasks: tasks/subtasks/subtask_2.yml
""",
),
"tasks/subtasks/subtask_2.yml": textwrap.dedent(
"""\
---
- name: subtask_2 | From subtask 2 do something
- name: subtasks | subtask_2 | From subtask 2 do something
debug: # <-- expected to raise fqcn[action-core]
msg: |
Something...
Expand Down
Loading