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.

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

key-order warning when using block with when condition #2509

Closed
netsandbox opened this issue Sep 28, 2022 · 7 comments
Closed

key-order warning when using block with when condition #2509

netsandbox opened this issue Sep 28, 2022 · 7 comments
Labels

Comments

@netsandbox
Copy link
Contributor

netsandbox commented Sep 28, 2022

Summary

key-order warning when using block with when condition.

Issue Type
  • Bug Report
Ansible and Ansible Lint details
ansible --version
ansible [core 2.12.9]
  config file = /home/ansible/code/git/ansible/ansible.cfg
  configured module search path = ['/home/ansible/code/git/ansible/modules']
  ansible python module location = /home/ansible/code/git/ansible/venv/lib/python3.8/site-packages/ansible
  ansible collection location = /home/ansible/code/git/ansible/collections
  executable location = /home/ansible/code/git/ansible/venv/bin/ansible
  python version = 3.8.10 (default, Jun 22 2022, 20:18:18) [GCC 9.4.0]
  jinja version = 3.1.2
  libyaml = True

ansible-lint --version
ansible-lint 6.7.0 using ansible 2.12.9
  • ansible installation method: pip
  • ansible-lint installation method: pip
OS / ENVIRONMENT

NA

STEPS TO REPRODUCE

Create playbook based on https://docs.ansible.com/ansible/latest/user_guide/playbooks_blocks.html#grouping-tasks-with-blocks:

- name: Test
  hosts: all
  tasks:
  - name: Handle the error
    block:
      - name: Print a message
        ansible.builtin.debug:
          msg: 'I execute normally'
    when: ansible_facts.distribution == 'CentOS'
$ ansible-lint playbook.yml 
WARNING  Listing 1 violation(s) that are fatal
key-order: You can improve the task key order to: name, when, block (key-order[task])
playbook.yml:4 Task/Handler: Handle the error
Desired Behavior

No warning for the above playbook.

Actual Behavior
WARNING  Listing 1 violation(s) that are fatal
key-order: You can improve the task key order to: name, when, block (key-order[task])
playbooks/test.yml:4 Task/Handler: Handle the error
@netsandbox netsandbox added bug new Triage required labels Sep 28, 2022
@ssbarnea
Copy link
Member

This is not a bug, it is like this on purpose and documented on https://ansible-lint.readthedocs.io/rules/key-order/

We discussed a lot about the subject and the decision was that block/rescue/always cannot have anything after them, not even when or tags. Documentation explains why.

@ssbarnea ssbarnea closed this as not planned Won't fix, can't repro, duplicate, stale Sep 28, 2022
@ssbarnea ssbarnea removed the new Triage required label Sep 28, 2022
@netsandbox
Copy link
Contributor Author

@ssbarnea sorry for the noise, understand now.

But would be great if you would add an example for block with when on https://ansible-lint.readthedocs.io/rules/key-order/
and when also the examples in the Ansible docs on https://docs.ansible.com/ansible/latest/user_guide/playbooks_blocks.html#grouping-tasks-with-blocks would be updated.

@cidrblock
Copy link
Contributor

I'll add a bit of unsolicited commentary here because we did talk about this for a while. In the case of a block, the tags and when clause are applied to each task within the block. For readability, having it at the top make it easier to see the details about the block itself. This becomes increasing important as the block size gets bigger and the tasks have large data structures...

@ssbarnea
Copy link
Member

We were talking about updating docs for that reason! The challenge just started as we did not establish a full order for all possible keys yet. But updating official examples is a very good idea.

@apatard
Copy link
Contributor

apatard commented Sep 28, 2022

I'll add a bit of unsolicited commentary here because we did talk about this for a while. In the case of a block, the tags and when clause are applied to each task within the block. For readability, having it at the top make it easier to see the details about the block itself. This becomes increasing important as the block size gets bigger and the tasks have large data structures...

Is there any rule/recommendation about block size ? Unless always/rescue/... are needed, if the block is getting big, wouldn't it be better/cleaner to just put the tasks in a different file ?

@ssbarnea
Copy link
Member

@apatard Look at https://ansible-lint.readthedocs.io/profiles/#shared and you will see two planned rules there related to these. They are not implemented yet, but they are planned. We still need to decide what is too-big, and for that we will need to run some data-collection scripts on what was published so far and get some feedback from the users. I am inclined to believe that 100 might be a decent candidate. In the the main reason we did not implement these two rules is because we did not had a number to pick. Once we have the magic number, doing it would be quite easy.

@apatard
Copy link
Contributor

apatard commented Sep 28, 2022

@ssbarnea 100 tasks in a block ? sounds a lot. Maybe you meant 10 ? Maybe open a poll to get the number ? Once this is decided, if the number is "small", will the check be reconsidered for block (as asked by this bug) ?

ssbarnea added a commit to ssbarnea/ansible-lint that referenced this issue Sep 28, 2022
ssbarnea added a commit that referenced this issue Sep 29, 2022
ssbarnea added a commit to ssbarnea/ansible-lint that referenced this issue Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants