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

Allow a prefix in subtasks names #2740

Merged
merged 1 commit into from
Dec 2, 2022
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
4 changes: 4 additions & 0 deletions .ansible-lint
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ enable_list:
- empty-string-compare # opt-in
- no-log-password # opt-in
- no-same-owner # opt-in
- name[prefix] # opt-in
# add yaml here if you want to avoid ignoring yaml checks when yamllint
# library is missing. Normally its absence just skips using that rule.
- yaml
Expand Down Expand Up @@ -109,3 +110,6 @@ kinds:
# List of additions modules to allow in only-builtins rule.
# only_builtins_allow_modules:
# - example_module

# Allow setting custom prefix for name[prefix] rule
task_name_prefix: "{stem} | "
1 change: 0 additions & 1 deletion .config/dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,6 @@ pipx
pkgcache # linux
pkgs
placefolder
playholder
pluggy
pluginmanager
podman
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ jobs:
WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:TOX_PARALLEL_NO_SPINNER
# Number of expected test passes, safety measure for accidental skip of
# tests. Update value if you add/remove tests.
PYTEST_REQPASS: 724
PYTEST_REQPASS: 725

steps:
- name: Activate WSL1
Expand Down
4 changes: 2 additions & 2 deletions examples/playbooks/tasks/include-in-block-inner.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
---
- name: I am a block
- name: include-in-block-inner | I am a block
block:
- name: Include tasks from inside a block
- name: simple_task | Include tasks from inside a block
ansible.builtin.include_tasks: simple_task.yml
13 changes: 13 additions & 0 deletions examples/playbooks/tasks/name-prefix-fail.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
- name: name-prefix-fail | this is not correctly capitalized
ansible.builtin.assert:
that: true
- name: This is missing the prefix
ansible.builtin.assert:
that: true
- name: name | This prefix is incomplete
ansible.builtin.assert:
that: true
- name: name-prefix-fail | This is correctly named
ansible.builtin.assert:
that: true
2 changes: 1 addition & 1 deletion examples/playbooks/tasks/nestedincludes.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
---
- name: One include
- name: simple_task | One include
include_tasks: tasks/simple_task.yml
2 changes: 1 addition & 1 deletion examples/playbooks/tasks/passing_task.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
---
- name: Simple task to include which generates no errors
- name: passing_task | Simple task to include which generates no errors
ansible.builtin.assert:
that: true
4 changes: 2 additions & 2 deletions examples/playbooks/tasks/simple_task.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
---
# unnamed-task: All tasks should be named
- ansible.builtin.assert:
- name: simple_task | This is named
ansible.builtin.assert:
fail_msg: foo
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
---
- name: Test Fixture
- name: included_file | Test Fixture
ansible.builtin.debug:
msg: "was found & included"
2 changes: 1 addition & 1 deletion examples/roles/include_relative/tasks/included_file.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
---
- name: Sample debug task
- name: included_file | Sample debug task
ansible.builtin.debug:
msg: "was found & included"
12 changes: 6 additions & 6 deletions examples/roles/loop_var_prefix/tasks/fail.yml
Original file line number Diff line number Diff line change
@@ -1,36 +1,36 @@
---
# 5 expected loop-var-prefix failures at 3, 9, 19, 26, 33
- name: That should trigger loop-var-prefix
- name: fail | That should trigger loop-var-prefix
ansible.builtin.debug:
var: item
loop:
- foo
- bar
- name: That should fail due to wrong prefix
- name: fail | That should fail due to wrong prefix
ansible.builtin.debug:
var: zz_item
loop:
- foo
- bar
loop_control:
loop_var: zz_item
- name: Using a block
- name: fail | Using a block
block:
- name: That should also not pass
- name: fail | That should also not pass
ansible.builtin.debug:
var: item
loop:
- apples
- oranges
rescue:
- name: That should also not pass
- name: fail | That should also not pass
ansible.builtin.debug:
var: item
loop:
- avocados
- kiwis
always:
- name: That should also not pass
- name: fail | That should also not pass
ansible.builtin.debug:
var: item
loop:
Expand Down
6 changes: 3 additions & 3 deletions examples/roles/loop_var_prefix/tasks/pass.yml
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
---
# 0 expected loop-var-prefix failures
- name: That should pass
- name: pass | That should pass
ansible.builtin.debug:
var: loop_var_prefix_item
loop:
- foo
- bar
loop_control:
loop_var: loop_var_prefix_item
- name: Using a block
- name: pass | Using a block
block:
- name: That should also pass
- name: pass | That should also pass
ansible.builtin.debug:
var: loop_var_prefix_item
loop:
Expand Down
30 changes: 15 additions & 15 deletions examples/roles/role_for_no_same_owner/tasks/fail.yml
Original file line number Diff line number Diff line change
@@ -1,69 +1,69 @@
---
- name: Block
- name: fail | Block
block:
- name: Synchronize-in-block
- name: fail | Synchronize-in-block
ansible.posix.synchronize:
src: dummy
dest: dummy

- name: Synchronize
- name: fail | Synchronize
ansible.posix.synchronize:
src: dummy
dest: dummy

- name: Nested-block
- name: fail | Nested-block
block:
- name: Synchronize
- name: fail | Synchronize
block:
- name: Synchronize-in-deep-block
- name: fail | Synchronize-in-deep-block
ansible.posix.synchronize:
src: dummy
dest: dummy
rescue:
- name: Synchronize-in-rescue
- name: fail | Synchronize-in-rescue
ansible.posix.synchronize:
src: dummy
dest: dummy
always:
- name: Synchronize-in-always
- name: fail | Synchronize-in-always
ansible.posix.synchronize:
src: dummy
dest: dummy

- name: Unarchive-bz2
- name: fail | Unarchive-bz2
ansible.builtin.unarchive:
src: "{{ file }}.tar.bz2"
dest: dummy

- name: Unarchive delegated
- name: fail | Unarchive delegated
ansible.builtin.unarchive:
src: "{{ file }}.tar.bz2"
dest: dummy
delegate_to: localhost

- name: Unarchive-gz
- name: fail | Unarchive-gz
ansible.builtin.unarchive:
src: "{{ file }}.tar.gz"
dest: dummy

- name: Unarchive-tar
- name: fail | Unarchive-tar
ansible.builtin.unarchive:
src: "{{ file }}.tar"
dest: dummy

- name: Unarchive-xz
- name: fail | Unarchive-xz
ansible.builtin.unarchive:
src: "{{ file }}.tar.xz"
dest: dummy

- name: Unarchive-zip
- name: fail | Unarchive-zip
ansible.builtin.unarchive:
src: "{{ file }}.zip"
dest: dummy
extra_opts:
- -X

- name: Unarchive-zip-same-owner
- name: fail | Unarchive-zip-same-owner
ansible.builtin.unarchive:
src: "{{ file }}.zip"
dest: dummy
Expand Down
10 changes: 5 additions & 5 deletions examples/roles/role_for_no_same_owner/tasks/pass.yml
Original file line number Diff line number Diff line change
@@ -1,32 +1,32 @@
---
- name: Synchronize-delegate
- name: pass | Synchronize-delegate
ansible.posix.synchronize:
src: dummy
dest: dummy
delegate_to: localhost

- name: Synchronize-no-same-owner
- name: pass | Synchronize-no-same-owner
ansible.posix.synchronize:
src: dummy
dest: dummy
owner: false
group: false

- name: Unarchive-no-same-owner
- name: pass | Unarchive-no-same-owner
ansible.builtin.unarchive:
src: "{{ file }}.tar.gz"
dest: dummy
extra_opts:
- --no-same-owner

- name: Unarchive-remote-src
- name: pass | Unarchive-remote-src
ansible.builtin.unarchive:
src: "{{ file }}.tar.gz"
dest: dummy
extra_opts:
- --no-same-owner

- name: Unarchive-unknown-file-ending
- name: pass | Unarchive-unknown-file-ending
ansible.builtin.unarchive:
src: "{{ file }}"
dest: dummy
3 changes: 3 additions & 0 deletions src/ansiblelint/_internal/rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
if TYPE_CHECKING:
from ansiblelint.errors import MatchError
from ansiblelint.file_utils import Lintable
from ansiblelint.rules import RulesCollection

_logger = logging.getLogger(__name__)
LOAD_FAILURE_MD = """\
Expand Down Expand Up @@ -48,6 +49,8 @@ class BaseRule:
# _order 5 implicit for normal rules
_order: int = 5
_help: str | None = None
# Added when a rule is registered into a collection, gives access to options
_collection: RulesCollection | None = None

@property
def help(self) -> str:
Expand Down
2 changes: 2 additions & 0 deletions src/ansiblelint/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"jinja[spacing]", # warning until we resolve all reported false-positives
"name[casing]",
"name[play]",
"name[prefix]",
"role-name",
"warning[empty-playbook]", # because ansible considers it warning only
"role-name[path]", # too new
Expand Down Expand Up @@ -132,6 +133,7 @@
strict=False,
rules={}, # Placeholder to set and keep configurations for each rule.
profile=None,
task_name_prefix="{stem} | ",
)

# Used to store detected tag deprecations
Expand Down
1 change: 1 addition & 0 deletions src/ansiblelint/rules/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ def register(self, obj: AnsibleLintRule, conditional: bool = False) -> None:
self.options.list_tags,
]
):
obj._collection = self # pylint: disable=protected-access
self.rules.append(obj)

def __iter__(self) -> Iterator[BaseRule]:
Expand Down
33 changes: 28 additions & 5 deletions src/ansiblelint/rules/name.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,15 @@

This rule identifies several problems related to the naming of tasks and plays.
This is important because these names are the primary way to **identify** and
**document** executed operations on console, logs or web interface.
**document** executed operations on the console, logs or web interface.

This rule can produce messages such:
This rule can produce messages as:

- `name[casing]` - All names should start with an uppercase letter for
languages that support it.
- `name[missing]` - All tasks should be named.
- `name[play]` - All plays should be named.
- `name[prefix]` - Prefix task names in sub-tasks files. (opt-in)
- `name[template]` - Jinja templates should only be at the end of 'name'. This
helps with the identification of tasks inside the source code when they fail.
The use of templating inside `name` keys is discouraged as there
Expand All @@ -18,21 +19,43 @@ This rule can produce messages such:
If you want to ignore some of the messages above, you can add any of them to
the `skip_list`.

## 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.

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.

For the moment, this sub-rule is just an **opt-in**, so you need to add it to
your `enable_list` to activate it.

```{note}
This rule was designed by [Red Hat Community of Practice](https://redhat-cop.github.io/automation-good-practices/#_prefix_task_names_in_sub_tasks_files_of_roles). The reasoning behind it being
that in a complex roles or playbooks with multiple (sub-)tasks file, it becomes
difficult to understand which task belongs to which file. Adding a prefix, in
combination with the role’s name automatically added by Ansible, makes it a
lot easier to follow and troubleshoot a role play.
```

## Problematic code

```yaml
---
- hosts: localhost # <-- playbook missing a name key
- hosts: localhost # <-- playbook name[play]
tasks:
- name: create placefolder file # <-- not starting with a capital letter
- name: create placefolder file # <-- name[casing] due lack of capital letter
ansible.builtin.command: touch /tmp/.placeholder
```

## Correct code

```yaml
---
- name: Play for creating playholder
- name: Play for creating placeholder
hosts: localhost
tasks:
- name: Create placeholder file
Expand Down
Loading