Update to append skipped rules for nested task

[nishipy]

As described in issue #1929, currently we can't handle # noqa properly for nested tasks (e.g. block in block task).
To fix it, this PR update skip_utils._get_tasks_from_blocks() to extract tasks from nested blocks recursively.

Fixes: #1929

[ssbarnea]

@nishipy Thanks for looking in to this. I still have some questions we might want to answer before going further with this one.

We already know of a need to inherit some attributes from parents, playbook -> block -> ... -> task kind of stuff, with major example being the defaults. That means that updating the task normalization routine would be the best place to do this, so we do not have to write complex rules for checking outside a task.

Same routine can also store information about noqa skips, even inherited ones.

Also, do we really need/want to allow noqa to apply to children? If someone adds a # noqa: no-name to a block, it will not only silence that on the block itself but also on all tasks from within. This would make impossible to silence only the block. Now you can see why this might not be desirable.

I am planning to soon add a feature to allow adding noqa inside linter config per file, so people would not need to add lots of noqa in various places. Would this be a good-enough workaround?

Another problem that I see is that you moved the the implementation from one module to another and I do know that this is not used only for skips. Anyway, that is the easiest thing to fix, but don't do anything on this before we clarify the other bits.

[nishipy]

Thanks for the comment.

Another problem that I see is that you moved the implementation ...

I see your point. Fixed commit and remove the unnecessary change.

And let me check the other part later.

[nishipy]

We already know of a need to inherit some attributes from parents ...

I understood the status of this issue and that we should improve task normalization stuff.
Will you plan to update utils.normalize_task()?

Also, do we really need/want to allow noqa to apply to children? ...

I see what you mean. Adding # noqa to not each task but the whole block could make it useful.
If my understanding is correct, such behavior is not implemented now. When we improve the way to normalize tasks, it has to be added.

I am planning to soon add a feature to allow adding noqa inside linter config per file, so people would not need to add lots of noqa in various places. Would this be a good-enough workaround?

It's hard for us to open lots of files and add noqa tags in them so that feature sounds great! I'm looking forward to that.

[ssbarnea]

We need a test

[cognifloyd]

In addition to the test (add it to test/test_skiputils.py), I see a couple of things that would simplify this change. wdyt?

[cognifloyd]

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?

[nishipy]

Thank you. That is better and more efficient. Let me fix this part.

[cognifloyd]

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

from ansiblelint.utils import is_nested_task

[nishipy]

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?

[nishipy]

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