Make sure all tasks get evaluated by matchtask including block/always/rescue and nested tasks

[nishipy]

Fix #1895.

[ssbarnea]

My impression is that the correct implementation would be to use matchtask as this is expected to be called for each task and avoid trying to traverse tasks from match play, especially as blocks can be nested.

matchplay should only look at play level stuff and not go into tasks/pre_tasks/post_tasks, as these are supported to be handled by matchtask.

Note: I am not 100% sure that is correct, but that I how i remember it to be aimed to work.

[nishipy]

Thank you for your point. I did some testing and confirmed that matchplay do not go into tasks/pre_tasks/post_taks or roles section. As you mentioned, we need to improve matchtask.

However, it seems that the task which is one of the inputs of macthtask doesn't include information related to block and block vars. e.g. for the following playbook, the inputted task will be like:

- hosts: 'localhost'
  gather_facts: true
  pre_tasks:
    - block:
      - name: 'Pre task'
        debug: msg="{{ FOO }}"
      vars:
        FOO: fooo1

{'delegate_to': <class 'ansible.utils.sentinel.Sentinel'>, 'name': 'Pre task', '__line__': 5, '__file__': 'check_test_role.yml', 'skipped_rules': [], 'action': {'__ansible_module__': 'debug', '__ansible_module_original__': 'debug', '__ansible_arguments__': [], 'msg': '{{ FOO }}'}, '__ansible_action_type__': 'meta'}

[nishipy]

Therefore we might have to overwrite the matchtasks (not but matchtask).

[ssbarnea]

Therefore we might have to overwrite the matchtasks (not but matchtask).

In fact I imagine that you might want to modify original matchtasks and make it call matchtask for each task in the tree, no need to override it.

@cognifloyd WDYT?

[cognifloyd]

Yes. Please use matchtask when evaluating per-task vars. Any rules that use matchplay to access tasks need to be refactored. Several bug fixes for nested blocks have been merged, so those NIH rules are not benefiting from those fixes, and probably have blind spots.

If matchtask is not emitting block tasks, then we should modify the base matchtasks so that it does. If it was skipping the block tasks on purpose, then we should add an attribute for rules to opt-in to receiving the block/rescue/always task.

I did something similar when I added access to the raw task.

[nishipy]

Thanks, understood. I will not override it but modify the base matchtasks.
Let me implement that and get back to you later.

[ssbarnea]

Thanks, understood. I will not override it but modify the base matchtasks. Let me implement that and get back to you later.

If you succeed you will make lots of people happy because it will fix similar bugs on lots of rules. Do not get scared if you see some test failing, you will likely have to update the tests when you implement it.

[nishipy]

The iter_tasks_in_file(), which is called by matchtasks, seems not to return the block information. I think of two ideas to resolve it:

1. Modify iter_tasks_in_file() to accomplish that
2. Create a new function (like get_blocks) that will return a list of blocks
   I like 2nd one and just started to implement but could you give me your opinion or other ideas if you have?

[cognifloyd]

Hmm. What about modifying ansiblelint.utils.get_action_tasks()?

get_action_tasks() is only used in ansiblelint.yaml_utils.iter_tasks_in_file().
iter_tasks_in_file() is only used in ansiblelint.rules.AnsibleLintRule.matchtasks().

[cognifloyd]

Or even ansiblelint.utils.extract_from_list() which is called from get_action_tasks().

[cognifloyd]

I'm not sure which approach will be most promising. Thank you for working on this / experimenting to figure out how it might be fixed!

[nishipy]

Thank you for the information! Let me dig into these functions.

[nishipy]

FYI, I found ansiblelint.skip_utils._get_task_blocks_from_playbook which seems to return a list of blocks from playbook. This is private function but maybe useful for us.

[nishipy]

Updated matchtasks to handle the blocks in pre_tasks/tasks/post_tasks of playbooks.
If we will input the following playbook, we currently get no warnings.

- hosts: 'localhost'
  gather_facts: true
  pre_tasks:
    - name: 'block in pre_tasks'
      block:
        - name: 'pre_task 1'
          ansible.builtin.debug: msg='{{ VAR_PRETASKS }}'
      vars:
        VAR_PRETASKS: 'this is pre_task'
  tasks:
    - name: 'block in tasks'
      block:
        - name: 'task 1'
          ansible.builtin.debug: msg='{{ VAR_TASKS }}'
      vars:
        VAR_TASKS: 'this is task'
  post_tasks:
    - name: 'block in post_tasks'
      block:
        - name: 'post task 1'
          ansible.builtin.debug: msg='{{ VAR_POSTTASK }}'
      vars:
        VAR_POSTTASK: 'this is post_task'

These commit fix it and we can see some violations as follows:

WARNING  Listing 3 violation(s) that are fatal
var-naming: Task defines variables within 'vars' section that violates variable naming standards
test.yml:4 Block: block in pre_tasks

var-naming: Task defines variables within 'vars' section that violates variable naming standards
test.yml:11 Block: block in tasks

var-naming: Task defines variables within 'vars' section that violates variable naming standards
test.yml:18 Block: block in post_tasks

[nishipy]

Hi @cognifloyd, I've added tests for normalize_task_v2 and is_nested_task so could you review that? Thanks!

[cognifloyd]

I just expanded the test for var-naming to make sure this addresses #1895. And I separated out the remaining issue into #2104 and #2105.