no-changed-when behavior is inconsistent

[nre-ableton]

Summary

This issue is the same as #1894, but since I don't have permission to re-open issues, I'm filing a new one (with the sample code updated for new rules such as key-order). 🙂 I made two comments on the closed issue, but since it's a closed issue it is possible that they were not seen. For bookkeeping purposes, if the project admins want to close this issue as a dupe and reopen #1894, I'd be fine with that.

Anyways, the original issue was claimed to have been fixed in 6.8.0b1, but is still easily reproducible in 6.8.4 with the below codeblock.

Issue Type

• Bug Report

Ansible and Ansible Lint details

$ ansible --version
ansible [core 2.13.5]
  config file = None
  configured module search path = ['/home/nre/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/nre/.cache/virtualenv/ansible-role-amtool/lib/python3.10/site-packages/ansible
  ansible collection location = /home/nre/.ansible/collections:/usr/share/ansible/collections
  executable location = /home/nre/.cache/virtualenv/ansible-role-amtool/bin/ansible
  python version = 3.10.6 (main, Sep 14 2022, 13:37:48) [GCC 11.2.0]
  jinja version = 3.1.2
  libyaml = True

$ ansible-lint --version
ansible-lint 6.8.4 using ansible 2.13.5

• ansible installation method: pip
• ansible-lint installation method: pip

OS / ENVIRONMENT

Ubuntu Linux 22.04

STEPS TO REPRODUCE

For an example, see the build-sources.yml task file of the ableton.ccache role here. An abbreviated version of this file follows:

---
- name: Check ccache version
  ansible.builtin.command: "ccache --version"
  environment:
    PATH: "{{ ccache_install_prefix }}/bin:{{ ansible_pkg_mgr_path }}"
  changed_when: false
  failed_when: false
  register: ccache_version_cmd

- name: Build ccache from sources
  when: >
    ccache_version_cmd.rc != 0 or
    ccache_version not in ccache_version_cmd.stdout_lines[0]
  block:
    - name: Run configure script
      ansible.builtin.command: "./configure --prefix={{ ccache_install_prefix }}"
      args:
        chdir: "{{ ccache_build_dir }}/ccache-{{ ccache_version }}"

    - name: Build ccache
      ansible.builtin.command: "make -j{{ ccache_make_jobs }}"
      args:
        chdir: "{{ ccache_build_dir }}/ccache-{{ ccache_version }}"

    - name: Install ccache
      become: true
      ansible.builtin.command: "make install"
      args:
        chdir: "{{ ccache_build_dir }}/ccache-{{ ccache_version }}"

In a block, conditions are applied to each task implicitly. But for the above code, ansible-lint emits a no-changed-when violation because it doesn't realize that the command tasks actually do have a condition attached to them. The result of running ansible-lint on the above code is:

WARNING  Listing 3 violation(s) that are fatal
tasks/main.yml:15: no-changed-when: Commands should not change things if nothing needs doing.
tasks/main.yml:20: no-changed-when: Commands should not change things if nothing needs doing.
tasks/main.yml:25: no-changed-when: Commands should not change things if nothing needs doing.

Desired Behavior

no-changed-when shouldn't be raised when tasks are inside of a block, and that block has a when condition.

[bluikko]

no-changed-when shouldn't be raised when tasks are inside of a block, and that block has a when condition.

I have always thought that this rule is about changed_when and not about when. The rule expects changed_when to be present to inform whether the command task changed something or not.

[nre-ableton]

@bluikko No, this is about the block, not changed_when.

Please consider the above example given for this issue. In that example, all of these tasks would pass ansible-lint were they not in the block, like so:

---
- name: Check ccache version
  ansible.builtin.command: "ccache --version"
  environment:
    PATH: "{{ ccache_install_prefix }}/bin:{{ ansible_pkg_mgr_path }}"
  changed_when: false
  failed_when: false
  register: ccache_version_cmd

- name: Run configure script
  ansible.builtin.command: "./configure --prefix={{ ccache_install_prefix }}"
  args:
    chdir: "{{ ccache_build_dir }}/ccache-{{ ccache_version }}"
  when: >
    ccache_version_cmd.rc != 0 or
    ccache_version not in ccache_version_cmd.stdout_lines[0]

- name: Build ccache
  ansible.builtin.command: "make -j{{ ccache_make_jobs }}"
  args:
    chdir: "{{ ccache_build_dir }}/ccache-{{ ccache_version }}"
  when: >
    ccache_version_cmd.rc != 0 or
    ccache_version not in ccache_version_cmd.stdout_lines[0]

- name: Install ccache
  become: true
  ansible.builtin.command: "make install"
  args:
    chdir: "{{ ccache_build_dir }}/ccache-{{ ccache_version }}"
  when: >
    ccache_version_cmd.rc != 0 or
    ccache_version not in ccache_version_cmd.stdout_lines[0]

The purpose of the block is of course to avoid copy/pasting the same when condition, but ansible-lint doesn't (yet) understand that the conditions for the block apply to all tasks inside of it.

[nre-ableton]

Update: this behavior still exists in ansible-lint 6.13.0.

[ssbarnea]

This rule is about changed_when alone nothing else. It does not matter if the task has or not a when condition and if it inside a loop of block.

Please check https://ansible-lint.readthedocs.io/rules/no-changed-when/ and see that there is nothing about a blocks or when blocks in it.

@nre-ableton Is there anything we can do to make this rule less confusing?

[nre-ableton]

It does not matter if the task has or not a when condition ...

@ssbarnea Well, the documentation you linked to above does state:

You should use the when clause to run tasks only if a check returns a particular result.

As you of course know, one purpose of a block is to group tasks to avoid having to copy/paste when conditions. As the Ansible documentation states (emphasis mine):

For example, a when statement is applied to the tasks within a block, not to the block itself.

This is the root of what I believe is ansible-lint's incorrect behavior. It examines tasks in a block but doesn't apply the block's conditions to these tasks, even though Ansible's documentation clearly states that this is how block works.

So let me make a much simpler example. In this playbook, First task and Second task are equivalent. Define foo to false, and neither task will run, but leave it as true and both will run.

---
- name: Run test tasks
  hosts: localhost
  gather_facts: false

  tasks:
    - name: Set foo
      ansible.builtin.set_fact:
        foo: true

    - name: First task
      ansible.builtin.command: "uname"
      when: foo

    - name: Block
      when: foo
      block:
        - name: Second task
          ansible.builtin.command: "uname"

However, ansible-lint raises an error only for the second task when examining this playbook: ansible/playbooks/sandbox.yml:20: no-changed-when: Commands should not change things if nothing needs doing. It doesn't complain about the first one because of the when condition.

But clearly, the second task is also conditional, and "does not change things if nothing needs doing", and ansible-lint doesn't understand this.

The reason I am so insistent about this bug being fixed is because I really like no-changed-when. It's a great rule, and I think it is a great tool to maintain idempotent playbooks and roles. However, I also really dislike copy-pasted code, and hence our playbooks use a lot of blocks to avoid this. As such, our projects are littered with # noqa no-changed-when suppressions that I wish could be removed.

Edit: I think the root of our misunderstanding is that (I believe) you see no-changed-when as a tool to control the changed state of tasks, and I am using it to determine whether these tasks should be run in the first place. Do you think it would make sense to have another rule to check that command/shell tasks should not be run unnecessarily? These modules in particular aren't idempotent, since Ansible has no way of knowing if these commands need to be run.

[bluikko]

It doesn't complain about the first one because of the when condition

Isn't this the bug? Both tasks should raise error because both of them are missing changed_when?

[ssbarnea]

@nre-ableton @bluikko Please review #3050 which should address all issues I identified on this thread.

Please note that new version will not look at when: conditions at all, it will only look for changed_when.

[bluikko]

Please review #3050 which should address all issues I identified on this thread.

I like the change and running the modified rule works fine.

I thought this is how it worked already until @nre-ableton raised this issue for the first time - but I had not looked at the rule source where the when is indeed listed. Having where in the rule never made sense to me and the rationale for the accompanying one line in documentation was never clear to me.

Edit: my reason is that why would having a when absolve the task from needing to state if it changed something? It just does not make sense to me (or I am just not seeing it). Surely changed_when is needed regardless of the task having a when on it.

[nre-ableton]

@ssbarnea Thanks, I'll have a more thorough look at that PR today. My first impression is that it'll probably work.

However, I think that it is still important to not execute shell/command tasks unless they are necessary, since these tasks are by their nature not idempotent. Perhaps we could introduce a new rule for this. Should I make a separate feature request for that?

[bluikko]

The rule does not seem to trigger for a task in role's handlers file but I suspect that may be a known limitation?

[bluikko]

@ssbarnea Excuse me for the spam but documentation in src/ansiblelint/rules/no_changed_when.py lines 41-42 would need removal of the when workaround.

[ssbarnea]

@bluikko Not spam at all, I just removed it. Let me know if you spot other things in it and thanks!

[ssbarnea]

@ssbarnea Thanks, I'll have a more thorough look at that PR today. My first impression is that it'll probably work.

However, I think that it is still important to not execute shell/command tasks unless they are necessary, since these tasks are by their nature not idempotent. Perhaps we could introduce a new rule for this. Should I make a separate feature request for that?

I would not say that they are not idempotent, just that their idempotency cannot be determined by Ansible itself, it needs a human review of the executed content.

[nre-ableton]

I would not say that they are not idempotent, just that their idempotency cannot be determined by Ansible itself, it needs a human review of the executed content.

Yes, agreed, and I think that ansible-lint could be a good tool to help enforce that. I'll make a feature request.

[nre-ableton]

See #3058