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.

Sign up for GitHub

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

Jump to bottom

Update to append skipped rules for nested task #2113

Merged
merged 9 commits into from
May 16, 2022
Merged

Update to append skipped rules for nested task #2113

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9a4a1db
Update to extract tasks from nested blocks recursively.
nishipy May 10, 2022
b70f0a9
Add is_nested_task fix error on test_import
nishipy May 10, 2022
3032e9e
Merge branch 'main' into fix-1929
ssbarnea May 11, 2022
a07957c
Merge branch 'ansible:main' into fix-1929
nishipy May 13, 2022
01efe0a
Update get_nested_tasks()
nishipy May 14, 2022
64db1ef
Merge branch 'ansible:main' into fix-1929
nishipy May 14, 2022
7af8ed8
Add tests for ansiblelint.skiputils
nishipy May 14, 2022
b30d980
Merge branch 'main' into fix-1929
ssbarnea May 16, 2022
fdb2363
Merge branch 'main' into fix-1929
ssbarnea May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/tox.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,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: 644
PYTEST_REQPASS: 648

steps:
- name: Activate WSL1
Expand Down
9 changes: 9 additions & 0 deletions examples/playbooks/noqa-nested.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
- hosts: localhost
tasks:
- name: example of multi-level block
block:
- name: 2nd level
block:
- ansible.builtin.debug: # noqa unnamed-task
msg: "test unnamed task in block"
6 changes: 6 additions & 0 deletions examples/playbooks/noqa.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
- hosts: localhost
tasks:
- name: this would typically fire git-latest and partial-become
become_user: alice # noqa git-latest partial-become
git: src=/path/to/git/repo dest=checkout
16 changes: 14 additions & 2 deletions src/ansiblelint/skip_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import logging
from functools import lru_cache
from itertools import product
from typing import TYPE_CHECKING, Any, Generator, List, Optional, Sequence
from typing import TYPE_CHECKING, Any, Dict, Generator, List, Optional, Sequence

# Module 'ruamel.yaml' does not explicitly export attribute 'YAML'; implicit reexport disabled
from ruamel.yaml import YAML
Expand Down Expand Up @@ -149,9 +149,12 @@ def _get_tasks_from_blocks(task_blocks: Sequence[Any]) -> Generator[Any, None, N
"""Get list of tasks from list made of tasks and nested tasks."""

def get_nested_tasks(task: Any) -> Generator[Any, None, None]:
if not task or not is_nested_task(task):
return
for k in NESTED_TASK_KEYS:
if task and k in task and task[k]:
if k in task and task[k]:
for subtask in task[k]:
yield from get_nested_tasks(subtask)
yield subtask

for task in task_blocks:
Expand Down Expand Up @@ -193,3 +196,12 @@ def normalize_tag(tag: str) -> str:
used_old_tags[tag] = RENAMED_TAGS[tag]
return RENAMED_TAGS[tag]
return tag


def is_nested_task(task: Dict[str, Any]) -> bool:
"""Check if task includes block/always/rescue."""
for key in NESTED_TASK_KEYS:
if task.get(key):
return True

return False
Comment on lines +201 to +207
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a duplicate of what is in utils.py. Could you drop this and do:

from ansiblelint.utils import is_nested_task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Yes, it is a duplicate now.

If from ansiblelint.utils import is_nested_task is added, I faced the following error on test_import[ansiblelint.__main__]:

...
>       assert result.returncode == 0
E       assert 1 == 0
E        +  where 1 = CompletedProcess(args=['/usr/local/Caskroom/miniconda/base/envs/env39/bin/python', '-c', "import ansiblelint.__main__, sys; print('ansible' in sys.modules); sys.exit(0 if 'ansible' not in sys.modules else 1)"], returncode=1).returncode

I'm not sure but it can be caused because utils.py imports some modules related to ansible.
Anyway I think we need to move the is_nested_task from ansiblelint.utils to ansiblelint.skip_utils and add from ansiblelint.skip_utils import is_nested_task in utils.py.
I received the this comment so I leave both is_nested_task but I'd like to update as above later.
Does that make sense?

Copy link
Contributor Author

@nishipy nishipy May 14, 2022 •
edited

Choose a reason for hiding this comment

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

added some tests in commit 7af8ed8 but test_is_nested_task is a duplicate as well now.

183 changes: 182 additions & 1 deletion test/test_skiputils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
"""Validate ansiblelint.skip_utils."""
from pathlib import Path
from typing import TYPE_CHECKING, Any, Dict

import pytest

from ansiblelint.skip_utils import get_rule_skips_from_line
from ansiblelint.file_utils import Lintable
from ansiblelint.skip_utils import (
append_skipped_rules,
get_rule_skips_from_line,
is_nested_task,
)
from ansiblelint.testing import RunFromText

if TYPE_CHECKING:
from ansible.parsing.yaml.objects import AnsibleBaseYAMLObject

PLAYBOOK_WITH_NOQA = """\
---
- hosts: all
Expand Down Expand Up @@ -36,3 +47,173 @@ def test_playbook_noqa(default_text_runner: RunFromText) -> None:
results = default_text_runner.run_playbook(PLAYBOOK_WITH_NOQA)
# Should raise error at "SOME_VAR".
assert len(results) == 1


@pytest.mark.parametrize(
("lintable", "yaml", "expected_form"),
(
pytest.param(
Lintable("examples/playbooks/noqa.yml", kind="playbook"),
[
{
"hosts": "localhost",
"tasks": [
{
"name": "this would typically fire git-latest and partial-become",
"become_user": "alice",
"git": "src=/path/to/git/repo dest=checkout",
"__line__": 4,
"__file__": Path("examples/playbooks/noqa.yml"),
}
],
"__line__": 2,
"__file__": Path("examples/playbooks/noqa.yml"),
}
],
[
{
"hosts": "localhost",
"tasks": [
{
"name": "this would typically fire git-latest and partial-become",
"become_user": "alice",
"git": "src=/path/to/git/repo dest=checkout",
"__line__": 4,
"__file__": Path("examples/playbooks/noqa.yml"),
"skipped_rules": ["git-latest", "partial-become"],
}
],
"__line__": 2,
"__file__": Path("examples/playbooks/noqa.yml"),
}
],
),
pytest.param(
Lintable("examples/playbooks/noqa-nested.yml", kind="playbook"),
[
{
"hosts": "localhost",
"tasks": [
{
"name": "example of multi-level block",
"block": [
{
"name": "2nd level",
"block": [
{
"ansible.builtin.debug": {
"msg": "test unnamed task in block",
"__line__": 9,
"__file__": Path(
"examples/playbooks/noqa-nested.yml"
),
},
"__line__": 8,
"__file__": Path(
"examples/playbooks/noqa-nested.yml"
),
}
],
"__line__": 6,
"__file__": Path(
"examples/playbooks/noqa-nested.yml"
),
}
],
"__line__": 4,
"__file__": Path("examples/playbooks/noqa-nested.yml"),
}
],
"__line__": 2,
"__file__": Path("examples/playbooks/noqa-nested.yml"),
}
],
[
{
"hosts": "localhost",
"tasks": [
{
"name": "example of multi-level block",
"block": [
{
"name": "2nd level",
"block": [
{
"ansible.builtin.debug": {
"msg": "test unnamed task in block",
"__line__": 9,
"__file__": Path(
"examples/playbooks/noqa-nested.yml"
),
},
"__line__": 8,
"__file__": Path(
"examples/playbooks/noqa-nested.yml"
),
"skipped_rules": ["unnamed-task"],
}
],
"__line__": 6,
"__file__": Path(
"examples/playbooks/noqa-nested.yml"
),
"skipped_rules": ["unnamed-task"],
}
],
"__line__": 4,
"__file__": Path("examples/playbooks/noqa-nested.yml"),
"skipped_rules": ["unnamed-task"],
}
],
"__line__": 2,
"__file__": Path("examples/playbooks/noqa-nested.yml"),
}
],
),
),
)
def test_append_skipped_rules(
lintable: Lintable,
yaml: "AnsibleBaseYAMLObject",
expected_form: "AnsibleBaseYAMLObject",
) -> None:
"""Check that it appends skipped_rules properly."""
assert append_skipped_rules(yaml, lintable) == expected_form


@pytest.mark.parametrize(
("task", "expected"),
(
pytest.param(
dict(
name="ensure apache is at the latest version",
yum={"name": "httpd", "state": "latest"},
),
False,
),
pytest.param(
dict(
name="Attempt and graceful roll back",
block=[
{"name": "Force a failure", "ansible.builtin.command": "/bin/false"}
],
rescue=[
{
"name": "Force a failure in middle of recovery!",
"ansible.builtin.command": "/bin/false",
}
],
always=[
{
"name": "Always do this",
"ansible.builtin.debug": {"msg": "This always executes"},
}
],
),
True,
),
),
)
def test_is_nested_task(task: Dict[str, Any], expected: bool) -> None:
"""Test is_nested_task() returns expected bool."""
assert is_nested_task(task) == expected