Skip to content

Commit

Permalink
Add name[play] to highlight plays without name
Browse files Browse the repository at this point in the history
From now on, we will be require plays to have names, same as task,
as these are also displayed by Ansible and help documenting them.

Related: #2171
  • Loading branch information
ssbarnea committed Aug 17, 2022
1 parent 382a15a commit 0be4238
Show file tree
Hide file tree
Showing 8 changed files with 84 additions and 26 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,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: 661
PYTEST_REQPASS: 662

steps:
- name: Activate WSL1
Expand Down
3 changes: 3 additions & 0 deletions examples/playbooks/play-name-missing.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
- hosts: localhost # <-- name[missing]
tasks: []
10 changes: 5 additions & 5 deletions examples/playbooks/task-has-name-failure.yml
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
---
- hosts: all
- hosts: all # <-- name[missing]
tasks:
- ansible.builtin.command: echo "no name" # <-- 1
- ansible.builtin.command: echo "no name" # <-- name[missing]
changed_when: false
- name: "" # <-- 2
- name: "" # <-- name[missing]
ansible.builtin.command: echo "empty name"
changed_when: false
- ansible.builtin.debug: # <-- 3
- ansible.builtin.debug: # <-- name[missing]
msg: Debug without a name
- ansible.builtin.meta: flush_handlers # <-- 4
- ansible.builtin.meta: flush_handlers # <-- name[missing]
2 changes: 1 addition & 1 deletion src/ansiblelint/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

from ansiblelint.loaders import yaml_from_file

DEFAULT_WARN_LIST = ["experimental", "name[casing]", "role-name"]
DEFAULT_WARN_LIST = ["experimental", "name[casing]", "name[play]", "role-name"]

DEFAULT_KINDS = [
# Do not sort this list, order matters.
Expand Down
2 changes: 1 addition & 1 deletion src/ansiblelint/rules/meta_no_tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ def matchyaml(self, file: Lintable) -> List[MatchError]:
def test_valid_tag_rule(rule_runner: Any) -> None:
"""Test rule matches."""
results = rule_runner.run_role_meta_main(META_TAG_VALID)
assert "Use 'galaxy_tags' rather than 'categories'" in str(results)
assert "Use 'galaxy_tags' rather than 'categories'" in str(results), results
assert "Expected 'categories' to be a list" in str(results)
assert "invalid: 'my s q l'" in str(results)
assert "invalid: 'MYTAG'" in str(results)
26 changes: 17 additions & 9 deletions src/ansiblelint/rules/name.md
Original file line number Diff line number Diff line change
@@ -1,24 +1,32 @@
## name

This rule identifies several problems related to task naming. This is important
because these names are the primary way to identify executed tasks on console,
logs or web interface. Their role is also to document what a task is supposed
to do.
This rule identifies several problems related to naming of tasks and plays.
This is important because these names are the primary way to identify executed
operations on console, logs or web interface. Their role is also to document
what Ansible is supposed to do.

- `name[missing]` - All tasks are required to have a name
- `name[casing]` - All task names should start with a capital letter
This rule can produce messages such:

- `name[casing]` - All names should start with an uppercase letter.
- `name[missing]` - All tasks should be named.
- `name[play]` - All plays should be named.

### Problematic code

```yaml
---
- ansible.builtin.command: touch /tmp/.placeholder
- hosts: localhost
tasks:
- ansible.builtin.command: touch /tmp/.placeholder
```

### Correct code

```yaml
---
- name: Create placeholder file
ansible.builtin.command: touch /tmp/.placeholder
- name: Play for creating playholder
hosts: localhost
tasks:
- name: Create placeholder file
ansible.builtin.command: touch /tmp/.placeholder
```
62 changes: 54 additions & 8 deletions src/ansiblelint/rules/name.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
from __future__ import annotations

import sys
from typing import TYPE_CHECKING, Any, Dict, Union
from typing import TYPE_CHECKING, Any, Dict, List, Union

from ansiblelint.errors import MatchError
from ansiblelint.rules import AnsibleLintRule
Expand All @@ -11,20 +11,47 @@
if TYPE_CHECKING:
from typing import Optional

from ansiblelint.constants import odict
from ansiblelint.file_utils import Lintable


class NameRule(AnsibleLintRule):
"""Rule for checking task names and their content."""
"""Rule for checking tasks and plays names."""

id = "name"
description = (
"All tasks should have a distinct name for readability "
"All tasks and plays should have a distinct name for readability "
"and for ``--start-at-task`` to work"
)
severity = "MEDIUM"
tags = ["idiom"]
version_added = "historic"
version_added = "v6.5.0 (last update)"

def matchplay(
self, file: "Lintable", data: "odict[str, Any]"
) -> List["MatchError"]:
"""Return matches found for a specific play (entry in playbook)."""
if file.kind != "playbook":
return []
if "name" not in data and not any(
key in data
for key in ["import_playbook", "ansible.builtin.import_playbook"]
):
return [
self.create_matcherror(
message="All plays should be named.",
linenumber=data[LINE_NUMBER_KEY],
tag="name[play]",
filename=file,
)
]
# pylint: disable=assignment-from-none
match = self._check_name(
data["name"], filename=file, linenumber=data[LINE_NUMBER_KEY]
)
if match:
return [match]
return []

def matchtask(
self, task: Dict[str, Any], file: Optional[Lintable] = None
Expand All @@ -37,14 +64,23 @@ def matchtask(
tag="name[missing]",
filename=file,
)
return (
self._check_name(name, filename=file, linenumber=task[LINE_NUMBER_KEY])
or False
)

# pylint: disable=useless-return
def _check_name(
self, name: str, filename: "Optional[Lintable]", linenumber: int
) -> "Optional[MatchError]":
if not name[0].isupper():
return self.create_matcherror(
message="All names should start with an uppercase letter.",
linenumber=task[LINE_NUMBER_KEY],
linenumber=linenumber,
tag="name[casing]",
filename=file,
filename=filename,
)
return False
return None


if "pytest" in sys.modules: # noqa: C901
Expand All @@ -67,7 +103,7 @@ def test_file_negative() -> None:
failure = "examples/playbooks/task-has-name-failure.yml"
bad_runner = Runner(failure, rules=collection)
errs = bad_runner.run()
assert len(errs) == 4
assert len(errs) == 5

def test_rule_name_lowercase() -> None:
"""Negative test for a task that starts with lowercase."""
Expand All @@ -79,3 +115,13 @@ def test_rule_name_lowercase() -> None:
assert len(errs) == 1
assert errs[0].tag == "name[casing]"
assert errs[0].rule.id == "name"

def test_name_play() -> None:
"""Positive test for name[play]."""
collection = RulesCollection()
collection.register(NameRule())
success = "examples/playbooks/play-name-missing.yml"
errs = Runner(success, rules=collection).run()
assert len(errs) == 1
assert errs[0].tag == "name[play]"
assert errs[0].rule.id == "name"
3 changes: 2 additions & 1 deletion test/test_skiputils.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@

PLAYBOOK_WITH_NOQA = """\
---
- hosts: all
- name: Fixture
hosts: all
vars:
SOME_VAR_NOQA: "Foo" # noqa var-naming
SOME_VAR: "Bar"
Expand Down

0 comments on commit 0be4238

Please sign in to comment.