Skip to content

Commit

Permalink
Determine if passed arguments are playbooks or not
Browse files Browse the repository at this point in the history
From now on, ansible-lint will no longer assume that a passed argument
must be a playbook. This addressed multiple bug reports where people
were confused that the linter reported errors when they passed a
taskfile as an argument.

The downside is that an invalid playbook that is a valid YAML file
might not raise an error and just report a warning and be treated as
a generic yaml file.

Fixes: #2892
  • Loading branch information
ssbarnea committed Jan 19, 2023
1 parent 48629ad commit 29c61c2
Show file tree
Hide file tree
Showing 23 changed files with 155 additions and 73 deletions.
2 changes: 2 additions & 0 deletions .config/dictionary.txt
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,8 @@ robertdebock
rolepath
roundtrip
ruamel
rulebook
rulebooks
ruledirs
rulesdir
rulesdirs
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/tox.yml
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ jobs:
WSLENV: FORCE_COLOR:PYTEST_REQPASS:TOXENV:GITHUB_STEP_SUMMARY
# Number of expected test passes, safety measure for accidental skip of
# tests. Update value if you add/remove tests.
PYTEST_REQPASS: 755
PYTEST_REQPASS: 767

steps:
- name: Activate WSL1
Expand Down
Empty file.
3 changes: 3 additions & 0 deletions examples/other/guess-1.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
- name: Minimal yaml to determine it as a playbook
hosts: localhost
21 changes: 21 additions & 0 deletions examples/playbooks/rulebook.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
# That file is not a valid playbook but it is a valid rulebook that was
# mistakenly put under a playbook directory.
- name: Demo rules with kafka as source
hosts: localhost
sources:
- name: kafka
kafka:
topic: eda
host: localhost
port: 9092
group_id: testing
rules:
- name:
condition: event.i is defined
action:
debug:
- name:
condition: event.stop == true
action:
shutdown:
Empty file added examples/roles/foo.yml
Empty file.
19 changes: 19 additions & 0 deletions examples/rulebooks/rulebook.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
---
- name: Demo rules with kafka as source
hosts: localhost
sources:
- name: kafka
kafka:
topic: eda
host: localhost
port: 9092
group_id: testing
rules:
- name:
condition: event.i is defined
action:
debug:
- name:
condition: event.stop == true
action:
shutdown:
1 change: 1 addition & 0 deletions src/ansiblelint/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
{"galaxy": "**/galaxy.yml"}, # Galaxy collection meta
{"reno": "**/releasenotes/*/*.{yaml,yml}"}, # reno release notes
{"tasks": "**/tasks/**/*.{yaml,yml}"},
{"rulebook": "**/rulebooks/*.{yml,yaml"},
{"playbook": "**/playbooks/*.{yml,yaml}"},
{"playbook": "**/*playbook*.{yml,yaml}"},
{"role": "**/roles/*/"},
Expand Down
1 change: 1 addition & 0 deletions src/ansiblelint/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ def main():

FileType = Literal[
"playbook",
"rulebook",
"meta", # role meta
"meta-runtime",
"tasks", # includes pre_tasks, post_tasks
Expand Down
18 changes: 18 additions & 0 deletions src/ansiblelint/file_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,24 @@ def __init__(
self.base_kind = base_kind or kind_from_path(self.path, base=True)
self.abspath = self.path.expanduser().absolute()

if self.kind == "yaml":
self._guess_kind()

def _guess_kind(self) -> None:
if self.kind == "yaml":
if isinstance(self.data, list) and "hosts" in self.data[0]:
if "rules" not in self.data[0]:
self.kind = "playbook"
else:
self.kind = "rulebook"
# we we failed to guess the more specific kind, we warn user
if self.kind == "yaml":
_logger.warning(
"Passed '%s' positional argument was identified as generic '%s' file kind.",
self.name,
self.kind,
)

def __getitem__(self, key: Any) -> Any:
"""Provide compatibility subscriptable support."""
if key == "path":
Expand Down
2 changes: 0 additions & 2 deletions src/ansiblelint/rules/name.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,12 @@ def _check_name(
return results
else:
effective_name = name[len(prefix) :]
# breakpoint()

if (
effective_name[0].isalpha()
and effective_name[0].islower()
and not effective_name[0].isupper()
):
# breakpoint()
results.append(
self.create_matcherror(
message="All names should start with an uppercase letter.",
Expand Down
1 change: 0 additions & 1 deletion src/ansiblelint/schemas/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ def refresh_schemas(min_age_seconds: int = 3600 * 24) -> int:
_logger.debug("Checking for updated schemas...")

changed = 0
# breakpoint()
for kind, data in JSON_SCHEMAS.items():
url = data["url"]
if "#" in url:
Expand Down
8 changes: 0 additions & 8 deletions src/ansiblelint/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -846,14 +846,6 @@ def get_lintables(
if args:
for arg in args:
lintable = Lintable(arg)
if lintable.kind in ("yaml", None):
_logger.warning(
"Overriding detected file kind '%s' with 'playbook' "
"for given positional argument: %s",
lintable.kind,
arg,
)
lintable = Lintable(arg, kind="playbook")
lintables.append(lintable)
else:

Expand Down
2 changes: 1 addition & 1 deletion test/local-content/test-collection.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,6 @@
- name: Use module from local collection
testns.test_collection.test_module_2:
- name: Use filter from local collection
assert:
ansible.builtin.assert:
that:
- 1 | testns.test_collection.test_filter(2) == '1:2'
Empty file.
Empty file.
Empty file.
125 changes: 77 additions & 48 deletions test/test_file_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,60 +144,94 @@ def test_discover_lintables_umlaut(monkeypatch: MonkeyPatch) -> None:
@pytest.mark.parametrize(
("path", "kind"),
(
("tasks/run_test_playbook.yml", "tasks"),
("foo/playbook.yml", "playbook"),
("playbooks/foo.yml", "playbook"),
("playbooks/roles/foo.yml", "yaml"),
pytest.param("tasks/run_test_playbook.yml", "tasks", id="0"),
pytest.param("foo/playbook.yml", "playbook", id="1"),
pytest.param("playbooks/foo.yml", "playbook", id="2"),
pytest.param("examples/roles/foo.yml", "yaml", id="3"),
# the only yml file that is not a playbook inside molecule/ folders
(".config/molecule/config.yml", "yaml"), # molecule shared config
(
"roles/foo/molecule/scenario1/base.yml",
"yaml",
pytest.param(
"examples/.config/molecule/config.yml", "yaml", id="4"
), # molecule shared config
pytest.param(
"test/schemas/test/molecule/cluster/base.yml", "yaml", id="5"
), # molecule scenario base config
(
"roles/foo/molecule/scenario1/molecule.yml",
"yaml",
pytest.param(
"test/schemas/test/molecule/cluster/molecule.yml", "yaml", id="6"
), # molecule scenario config
("roles/foo/molecule/scenario2/foobar.yml", "playbook"), # custom playbook name
(
"roles/foo/molecule/scenario3/converge.yml",
"playbook",
pytest.param(
"test/schemas/test/molecule/cluster/foobar.yml", "playbook", id="7"
), # custom playbook name
pytest.param(
"test/schemas/test/molecule/cluster/converge.yml", "playbook", id="8"
), # common playbook name
(
"roles/foo/molecule/scenario3/requirements.yml",
"requirements",
pytest.param(
"roles/foo/molecule/scenario3/requirements.yml", "requirements", id="9"
), # requirements
(
"roles/foo/molecule/scenario3/collections.yml",
"requirements",
pytest.param(
"roles/foo/molecule/scenario3/collections.yml", "requirements", id="10"
), # requirements
("roles/foo/meta/argument_specs.yml", "arg_specs"), # role argument specs
pytest.param(
"roles/foo/meta/argument_specs.yml", "arg_specs", id="11"
), # role argument specs
# tasks files:
("tasks/directory with spaces/main.yml", "tasks"), # tasks
("tasks/requirements.yml", "tasks"), # tasks
pytest.param("tasks/directory with spaces/main.yml", "tasks", id="12"), # tasks
pytest.param("tasks/requirements.yml", "tasks", id="13"), # tasks
# requirements (we do not support includes yet)
("requirements.yml", "requirements"), # collection requirements
("roles/foo/meta/requirements.yml", "requirements"), # inside role requirements
pytest.param(
"requirements.yml", "requirements", id="14"
), # collection requirements
pytest.param(
"roles/foo/meta/requirements.yml", "requirements", id="15"
), # inside role requirements
# Undeterminable files:
("test/fixtures/unknown-type.yml", "yaml"),
("releasenotes/notes/run-playbooks-refactor.yaml", "reno"), # reno
("examples/host_vars/localhost.yml", "vars"),
("examples/group_vars/all.yml", "vars"),
("examples/playbooks/vars/other.yml", "vars"),
("examples/playbooks/vars/subfolder/settings.yml", "vars"), # deep vars
("molecule/scenario/collections.yml", "requirements"), # deprecated 2.8 format
(
"../roles/geerlingguy.mysql/tasks/configure.yml",
"tasks",
pytest.param("test/fixtures/unknown-type.yml", "yaml", id="16"),
pytest.param(
"releasenotes/notes/run-playbooks-refactor.yaml", "reno", id="17"
), # reno
pytest.param("examples/host_vars/localhost.yml", "vars", id="18"),
pytest.param("examples/group_vars/all.yml", "vars", id="19"),
pytest.param("examples/playbooks/vars/other.yml", "vars", id="20"),
pytest.param(
"examples/playbooks/vars/subfolder/settings.yml", "vars", id="21"
), # deep vars
pytest.param(
"molecule/scenario/collections.yml", "requirements", id="22"
), # deprecated 2.8 format
pytest.param(
"../roles/geerlingguy.mysql/tasks/configure.yml", "tasks", id="23"
), # relative path involved
("galaxy.yml", "galaxy"),
("foo.j2.yml", "jinja2"),
("foo.yml.j2", "jinja2"),
("foo.j2.yaml", "jinja2"),
("foo.yaml.j2", "jinja2"),
pytest.param("galaxy.yml", "galaxy", id="24"),
pytest.param("foo.j2.yml", "jinja2", id="25"),
pytest.param("foo.yml.j2", "jinja2", id="26"),
pytest.param("foo.j2.yaml", "jinja2", id="27"),
pytest.param("foo.yaml.j2", "jinja2", id="28"),
pytest.param(
"examples/playbooks/rulebook.yml", "playbook", id="29"
), # playbooks folder should determine kind
pytest.param(
"examples/rulebooks/rulebook.yml", "rulebook", id="30"
), # content should determine it as a rulebook
pytest.param(
"examples/yamllint/valid.yml", "yaml", id="31"
), # empty yaml is valid yaml, not assuming anything else
pytest.param(
"examples/other/guess-1.yml", "playbook", id="32"
), # content should determine is as a play
pytest.param(
"examples/playbooks/tasks/passing_task.yml", "tasks", id="33"
), # content should determine is tasks
pytest.param("examples/collection/galaxy.yml", "galaxy", id="34"),
pytest.param("examples/meta/runtime.yml", "meta-runtime", id="35"),
pytest.param("examples/meta/changelogs/changelog.yaml", "changelog", id="36"),
pytest.param("examples/inventory/inventory.yml", "inventory", id="37"),
pytest.param("examples/inventory/production.yml", "inventory", id="38"),
pytest.param("examples/playbooks/vars/empty_vars.yml", "vars", id="39"),
pytest.param(
"examples/playbooks/vars/subfolder/settings.yaml", "vars", id="40"
),
),
)
def test_default_kinds(monkeypatch: MonkeyPatch, path: str, kind: FileType) -> None:
def test_kinds(monkeypatch: MonkeyPatch, path: str, kind: FileType) -> None:
"""Verify auto-detection logic based on DEFAULT_KINDS."""
options = cli.get_config([])

Expand All @@ -212,9 +246,6 @@ def mockreturn(options: Namespace) -> dict[str, Any]:

monkeypatch.setattr(file_utils, "discover_lintables", mockreturn)
result = file_utils.discover_lintables(options)
# get_lintable could return additional files and we only care to see
# that the given file is among the returned list.
assert lintable_detected.name in result
assert lintable_detected.kind == result[lintable_expected.name]


Expand Down Expand Up @@ -310,9 +341,7 @@ def test_lintable_with_new_file(tmp_path: Path) -> None:
"""Validate ``Lintable.updated`` for a new file."""
lintable = Lintable(tmp_path / "new.yaml")

with pytest.raises(FileNotFoundError):
_ = lintable.content

lintable.content = BASIC_PLAYBOOK
lintable.content = BASIC_PLAYBOOK
assert lintable.content == BASIC_PLAYBOOK

Expand Down
6 changes: 3 additions & 3 deletions test/test_formatter.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def test_format_coloured_string() -> None:
message="message",
linenumber=1,
details=DETAILS,
filename=Lintable("filename.yml"),
filename=Lintable("filename.yml", content=""),
rule=rule,
)
formatter.format(match)
Expand All @@ -50,7 +50,7 @@ def test_unicode_format_string() -> None:
message="\U0001f427",
linenumber=1,
details=DETAILS,
filename=Lintable("filename.yml"),
filename=Lintable("filename.yml", content=""),
rule=rule,
)
formatter.format(match)
Expand All @@ -62,7 +62,7 @@ def test_dict_format_line() -> None:
message="xyz",
linenumber=1,
details={"hello": "world"}, # type: ignore
filename=Lintable("filename.yml"),
filename=Lintable("filename.yml", content=""),
rule=rule,
)
formatter.format(match)
6 changes: 3 additions & 3 deletions test/test_formatter_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def setup_class(self) -> None:
message="message",
linenumber=1,
details="hello",
filename=Lintable("filename.yml"),
filename=Lintable("filename.yml", content=""),
rule=self.rule,
)
)
Expand All @@ -41,7 +41,7 @@ def setup_class(self) -> None:
message="message",
linenumber=2,
details="hello",
filename=Lintable("filename.yml"),
filename=Lintable("filename.yml", content=""),
rule=self.rule,
)
)
Expand Down Expand Up @@ -103,7 +103,7 @@ def test_validate_codeclimate_schema_with_positions(self) -> None:
linenumber=1,
column=42,
details="hello",
filename=Lintable("filename.yml"),
filename=Lintable("filename.yml", content=""),
rule=self.rule,
)
]
Expand Down
4 changes: 2 additions & 2 deletions test/test_formatter_sarif.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def setup_class(self) -> None:
linenumber=1,
column=10,
details="hello",
filename=Lintable("filename.yml"),
filename=Lintable("filename.yml", content=""),
rule=self.rule,
tag="yaml[test]",
)
Expand All @@ -46,7 +46,7 @@ def setup_class(self) -> None:
message="message",
linenumber=2,
details="hello",
filename=Lintable("filename.yml"),
filename=Lintable("filename.yml", content=""),
rule=self.rule,
tag="yaml[test]",
)
Expand Down
4 changes: 2 additions & 2 deletions test/test_matcherrror.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ def test_matcherror_invalid() -> None:
(MatchError("z"), MatchError("a")),
# filenames takes priority in sorting
(
MatchError("a", filename=Lintable("b")),
MatchError("a", filename=Lintable("a")),
MatchError("a", filename=Lintable("b", content="")),
MatchError("a", filename=Lintable("a", content="")),
),
# rule id partial-become > rule id no-changed-when
(
Expand Down

0 comments on commit 29c61c2

Please sign in to comment.