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

Changes from 3 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
15 changes: 14 additions & 1 deletion 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,13 @@ 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:
return
Copy link
Contributor

Choose a reason for hiding this comment

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

If you do this:

Suggested change
if not task:
return
if not task or not is_nested_task(task):
return

Then that could slightly simplify the rest of the logic like this:

    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 k in task and task[k]:
                for subtask in task[k]:
                    yield from get_nested_tasks(subtask)
                    yield subtask

    for task in task_blocks:
        yield from get_nested_tasks(task)
        yield task

wdyt?

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. That is better and more efficient. Let me fix this part.

for k in NESTED_TASK_KEYS:
if task and k in task and task[k]:
for subtask in task[k]:
if is_nested_task(subtask):
yield from get_nested_tasks(subtask)
yield subtask

for task in task_blocks:
Expand Down Expand Up @@ -193,3 +197,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.