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 831ef53
Show file tree
Hide file tree
Showing 22 changed files with 127 additions and 37 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
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 @@
---
# 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
24 changes: 23 additions & 1 deletion 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 in ("yaml", None):
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 Expand Up @@ -497,4 +515,8 @@ def expand_dirs_in_lintables(lintables: set[Lintable]) -> None:
if item.path.is_dir():
for filename in all_files:
if filename.startswith(str(item.path)):
lintables.add(Lintable(filename))
try:
lintables.add(Lintable(filename))
except Exception as exc:
breakpoint()
pass
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.
57 changes: 45 additions & 12 deletions test/test_file_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,20 +147,23 @@ def test_discover_lintables_umlaut(monkeypatch: MonkeyPatch) -> None:
("tasks/run_test_playbook.yml", "tasks"),
("foo/playbook.yml", "playbook"),
("playbooks/foo.yml", "playbook"),
("playbooks/roles/foo.yml", "yaml"),
("examples/roles/foo.yml", "yaml"),
# the only yml file that is not a playbook inside molecule/ folders
(".config/molecule/config.yml", "yaml"), # molecule shared config
("examples/.config/molecule/config.yml", "yaml"), # molecule shared config
(
"roles/foo/molecule/scenario1/base.yml",
"test/schemas/test/molecule/cluster/base.yml",
"yaml",
), # molecule scenario base config
(
"roles/foo/molecule/scenario1/molecule.yml",
"test/schemas/test/molecule/cluster/molecule.yml",
"yaml",
), # molecule scenario config
("roles/foo/molecule/scenario2/foobar.yml", "playbook"), # custom playbook name
(
"roles/foo/molecule/scenario3/converge.yml",
"test/schemas/test/molecule/cluster/foobar.yml",
"playbook",
), # custom playbook name
(
"test/schemas/test/molecule/cluster/converge.yml",
"playbook",
), # common playbook name
(
Expand Down Expand Up @@ -212,9 +215,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 @@ -308,10 +308,8 @@ def test_lintable_content_setter_with_bad_types(updated_content: Any) -> None:

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 = Lintable(tmp_path / "new.yaml")

lintable.content = BASIC_PLAYBOOK
assert lintable.content == BASIC_PLAYBOOK
Expand Down Expand Up @@ -460,3 +458,38 @@ def test_bug_2513(
results = Runner(filename, rules=default_rules_collection).run()
assert len(results) == 1
assert results[0].rule.id == "name"


@pytest.mark.parametrize(
("path", "kind"),
(
pytest.param(
"examples/playbooks/rulebook.yml", "playbook", id="0"
), # playbooks folder should determite kind
pytest.param(
"examples/rulebooks/rulebook.yml", "rulebook", id="1"
), # content should determine it as a rulebook
pytest.param(
"examples/yamllint/valid.yml", "yaml", id="2"
), # empty yaml is valid yaml, not assuming anything else
pytest.param(
"examples/other/guess-1.yml", "playbook", id="3"
), # content should determine is as a playplay
pytest.param(
"examples/playbooks/tasks/passing_task.yml", "tasks", id="4"
), # content should determine is tasks
pytest.param("examples/collection/galaxy.yml", "galaxy", id="5"),
pytest.param("examples/meta/runtime.yml", "meta-runtime", id="6"),
pytest.param("examples/meta/changelogs/changelog.yaml", "changelog", id="7"),
pytest.param("examples/inventory/inventory.yml", "inventory", id="8"),
pytest.param("examples/inventory/production.yml", "inventory", id="9"),
pytest.param("examples/playbooks/vars/empty_vars.yml", "vars", id="10"),
pytest.param(
"examples/playbooks/vars/subfolder/settings.yaml", "vars", id="11"
),
),
)
def test_lintable_guess_kind(path: str, kind: str) -> None:
"""Test that Lintable can guess kind of file."""
lintable = Lintable(path)
assert lintable.kind == kind
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
3 changes: 1 addition & 2 deletions test/test_yaml_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@
@pytest.fixture(name="empty_lintable")
def fixture_empty_lintable() -> Lintable:
"""Return a Lintable with no contents."""
lintable = Lintable("__empty_file__.yaml")
lintable._content = "" # pylint: disable=protected-access
lintable = Lintable("__empty_file__.yaml", content="")
return lintable


Expand Down

0 comments on commit 831ef53

Please sign in to comment.