ansible-lint --fix should uppercase all handlers passed to notify

[cavcrosby]

Summary

When running ansible-lint --fix on a playbook that contains task(s) with the notify parameter using a list of handlers as its value, these handlers are only fixed for a name[casing] violation if they exist under handlers as a task. Related to #4028.

Issue Type

• Bug Report

OS / ENVIRONMENT

ansible-lint 24.2.3 using ansible-core:2.16.6 ansible-compat:4.1.11 ruamel-yaml:0.18.6 ruamel-yaml-clib:0.2.8

• ansible installation method: pip
• ansible-lint installation method: pip (also tested main branch)

STEPS TO REPRODUCE

Setup

mkdir -p ./playbooks

cat << _EOF_ > "./playbooks/foo.yml"
---
- name: This is my playbook
  hosts: localhost
  tasks:
    - name: Task that always changes
      ansible.builtin.debug:
        msg: I always change!
      changed_when: true
      notify: my handler

    - name: Task with notify as list
      ansible.builtin.debug:
        msg: I always change!
      changed_when: true
      notify:
        - my handler 1
        - my handler
        - my handler 2

    - name: Task without notify
      ansible.builtin.debug:
        msg: I always change!
      changed_when: true

  handlers:
    - name: my handler
      ansible.builtin.debug:
        msg: I never run :(
_EOF_

Run

ansible-lint --fix "./playbooks/foo.yml"

Desired Behavior

I would have expected that ansible-lint --fix would have renamed all handlers passed to notify to begin with an upper case letter, regardless of their existence under handlers as a task.

Actual Behavior

The actual behavior is that the my handler handler does get fixed for name[casing], but the my handler 1 and my handler 2 handlers do not.

Without verbosity

Modified 1 files.
Passed: 0 failure(s), 0 warning(s) on 1 files. Last profile that met the validation criteria was 'production'.

With verbosity

DEBUG    Logging initialized to level 10
INFO     Identified / as project root due file system root.
DEBUG    Options: Options(_skip_ansible_syntax_check=False, cache_dir=PosixPath('/home/conner/.cache/ansible-compat/e3b0c4'), colored=False, configured=True, cwd=PosixPath('/tmp'), display_relative_path=True, exclude_paths=['.cache', '.git', '.hg', '.svn', '.tox'], format=None, lintables=['./playbooks/foo.yml'], list_rules=False, list_tags=False, write_list=['all'], parseable=False, quiet=0, rulesdirs=[PosixPath('/home/conner/.pyenv/versions/ansible-lint/lib/python3.10/site-packages/ansiblelint/rules')], skip_list=[], tags=[], verbosity=5, warn_list=['experimental', 'jinja', 'fqcn'], mock_filters=[], mock_modules=[], mock_roles=[], loop_var_prefix=None, only_builtins_allow_collections=[], only_builtins_allow_modules=[], var_naming_pattern=None, offline=None, project_dir='..', extra_vars=None, enable_list=[], skip_action_validation=True, strict=False, rules={}, profile=None, task_name_prefix='{stem} | ', sarif_file=None, config_file=None, generate_ignore=False, rulesdir=[], use_default_rules=False, version=False, list_profiles=False, ignore_file=None, max_tasks=100, max_block_depth=20)
DEBUG    CWD: /tmp
DEBUG    Logging initialized to level 10
INFO     Set ANSIBLE_LIBRARY=/home/conner/.cache/ansible-compat/cf10d3/modules:/home/conner/.ansible/plugins/modules:/usr/share/ansible/plugins/modules
INFO     Set ANSIBLE_COLLECTIONS_PATH=/home/conner/.cache/ansible-compat/cf10d3/collections:/home/conner/.ansible/collections:/usr/share/ansible/collections
INFO     Set ANSIBLE_ROLES_PATH=/home/conner/.cache/ansible-compat/cf10d3/roles:/home/conner/.ansible/roles:/usr/share/ansible/roles:/etc/ansible/roles
DEBUG    Effective yamllint rules used: {'anchors': {'level': 'error', 'forbid-undeclared-aliases': True, 'forbid-duplicated-anchors': False, 'forbid-unused-anchors': False}, 'braces': {'level': 'error', 'forbid': False, 'min-spaces-inside': 0, 'max-spaces-inside': 1, 'min-spaces-inside-empty': -1, 'max-spaces-inside-empty': -1}, 'brackets': {'level': 'error', 'forbid': False, 'min-spaces-inside': 0, 'max-spaces-inside': 0, 'min-spaces-inside-empty': -1, 'max-spaces-inside-empty': -1}, 'colons': {'level': 'error', 'max-spaces-before': 0, 'max-spaces-after': 1}, 'commas': {'level': 'error', 'max-spaces-before': 0, 'min-spaces-after': 1, 'max-spaces-after': 1}, 'comments': {'level': 'warning', 'require-starting-space': True, 'ignore-shebangs': True, 'min-spaces-from-content': 1}, 'comments-indentation': False, 'document-end': False, 'document-start': False, 'empty-lines': {'level': 'error', 'max': 2, 'max-start': 0, 'max-end': 0}, 'empty-values': False, 'float-values': False, 'hyphens': {'level': 'error', 'max-spaces-after': 1}, 'indentation': {'level': 'error', 'spaces': 'consistent', 'indent-sequences': True, 'check-multi-line-strings': False}, 'key-duplicates': {'level': 'error', 'forbid-duplicated-merge-keys': False}, 'key-ordering': False, 'line-length': {'level': 'error', 'max': 160, 'allow-non-breakable-words': True, 'allow-non-breakable-inline-mappings': False}, 'new-line-at-end-of-file': {'level': 'error'}, 'new-lines': {'level': 'error', 'type': 'unix'}, 'octal-values': {'forbid-implicit-octal': True, 'forbid-explicit-octal': True, 'level': 'error'}, 'quoted-strings': False, 'trailing-spaces': {'level': 'error'}, 'truthy': {'level': 'warning', 'allowed-values': ['true', 'false'], 'check-keys': True}}
DEBUG    Logging initialized to level 10
INFO     Set ANSIBLE_LIBRARY=/home/conner/.cache/ansible-compat/cf10d3/modules:/home/conner/.ansible/plugins/modules:/usr/share/ansible/plugins/modules
INFO     Set ANSIBLE_COLLECTIONS_PATH=/home/conner/.cache/ansible-compat/cf10d3/collections:/home/conner/.ansible/collections:/usr/share/ansible/collections
INFO     Set ANSIBLE_ROLES_PATH=/home/conner/.cache/ansible-compat/cf10d3/roles:/home/conner/.ansible/roles:/usr/share/ansible/roles:/etc/ansible/roles
INFO     Executing syntax check on playbook playbooks/foo.yml (0.39s)
DEBUG    Examining playbooks/foo.yml of type playbook
DEBUG    Running rule internal-error
DEBUG    Running rule load-failure
DEBUG    Running rule parser-error
DEBUG    Running rule warning
DEBUG    Running rule yaml
DEBUG    Running rule args
DEBUG    Running rule avoid-implicit
DEBUG    Running rule command-instead-of-module
DEBUG    Running rule command-instead-of-shell
DEBUG    Running rule complexity
DEBUG    Running rule deprecated-bare-vars
DEBUG    Running rule deprecated-local-action
DEBUG    Running rule deprecated-module
DEBUG    Running rule fqcn
DEBUG    Running rule galaxy
DEBUG    Running rule ignore-errors
DEBUG    Running rule inline-env-var
DEBUG    Running rule jinja
DEBUG    Running rule key-order
DEBUG    Running rule latest
DEBUG    Running rule literal-compare
DEBUG    Running rule loop-var-prefix
DEBUG    Running rule meta-incorrect
DEBUG    Running rule meta-no-tags
DEBUG    Running rule meta-runtime
DEBUG    Running rule meta-video-links
DEBUG    Running rule name
DEBUG    Running rule no-changed-when
DEBUG    Running rule no-free-form
DEBUG    Running rule no-handler
DEBUG    Running rule no-jinja-when
DEBUG    Running rule no-relative-paths
DEBUG    Running rule no-tabs
DEBUG    Running rule package-latest
DEBUG    Running rule partial-become
DEBUG    Running rule playbook-extension
DEBUG    Running rule risky-file-permissions
DEBUG    Running rule risky-octal
DEBUG    Running rule risky-shell-pipe
DEBUG    Running rule role-name
DEBUG    Running rule run-once
DEBUG    Running rule sanity
DEBUG    Running rule schema
DEBUG    Running rule var-naming
DEBUG    Begin fixing: 1 matches
DEBUG    Dumping playbooks/foo.yml (playbook) using YAML ((1, 1))
DEBUG    Fixing: (1 of 1)  (All names should start with an uppercase letter.) matched playbooks/foo.yml:26 Task/Handler: my handler
DEBUG    Fixed, removed:  (All names should start with an uppercase letter.) matched playbooks/foo.yml:26 Task/Handler: my handler
DEBUG    Attempting to release lock 137747239368784 on /home/conner/.cache/ansible-compat/e3b0c4/.lock
DEBUG    Lock 137747239368784 released on /home/conner/.cache/ansible-compat/e3b0c4/.lock

DEBUG    Determined rule-profile order: {'internal-error': (0, 'min'), 'load-failure': (1, 'min'), 'parser-error': (2, 'min'), 'syntax-check': (3, 'min'), 'command-instead-of-module': (4, 'basic'), 'command-instead-of-shell': (5, 'basic'), 'deprecated-bare-vars': (6, 'basic'), 'deprecated-local-action': (7, 'basic'), 'deprecated-module': (8, 'basic'), 'inline-env-var': (9, 'basic'), 'key-order': (10, 'basic'), 'literal-compare': (11, 'basic'), 'jinja': (12, 'basic'), 'no-free-form': (13, 'basic'), 'no-jinja-when': (14, 'basic'), 'no-tabs': (15, 'basic'), 'partial-become': (16, 'basic'), 'playbook-extension': (17, 'basic'), 'role-name': (18, 'basic'), 'schema': (19, 'basic'), 'name': (20, 'basic'), 'var-naming': (21, 'basic'), 'yaml': (22, 'basic'), 'name': (23, 'moderate'), 'name': (24, 'moderate'), 'name': (25, 'moderate'), 'spell-var-name': (26, 'moderate'), 'avoid-implicit': (27, 'safety'), 'latest': (28, 'safety'), 'package-latest': (29, 'safety'), 'risky-file-permissions': (30, 'safety'), 'risky-octal': (31, 'safety'), 'risky-shell-pipe': (32, 'safety'), 'galaxy': (33, 'shared'), 'ignore-errors': (34, 'shared'), 'layout': (35, 'shared'), 'meta-incorrect': (36, 'shared'), 'meta-no-tags': (37, 'shared'), 'meta-video-links': (38, 'shared'), 'meta-version': (39, 'shared'), 'meta-runtime': (40, 'shared'), 'no-changed-when': (41, 'shared'), 'no-changelog': (42, 'shared'), 'no-handler': (43, 'shared'), 'no-relative-paths': (44, 'shared'), 'max-block-depth': (45, 'shared'), 'max-tasks': (46, 'shared'), 'unsafe-loop': (47, 'shared'), 'avoid-dot-notation': (48, 'production'), 'sanity': (49, 'production'), 'fqcn': (50, 'production'), 'import-task-no-when': (51, 'production'), 'meta-no-dependencies': (52, 'production'), 'single-entry-point': (53, 'production'), 'use-loop': (54, 'production')}
Modified 1 files.
Passed: 0 failure(s), 0 warning(s) on 1 files. Last profile that met the validation criteria was 'production'.

[Qalthos]

I'm not entirely sure what you want to happen here, so I'm going to walk through this to make sure I understand the situation correctly.

We update handler names in notify after updating the task name it points to. We do this because the handler is looked up by its name, and so they have to match. By the same token, if a task wants to notify a handler that is not in the scope of files ansible-lint is fixing, we can't update it, because that would break the connection to the handler, assuming it exists.

With #4149 merged, the casing issue is now called out when present in a notify, but there doesn't seem to be anything further we can safely do about it with an autofix.

[cavcrosby]

I believe what I was inquiring about was that any handler in notify should be updated to resolve casing issues, even if there isn't a matching name to be found. When working on #4149, I remember being somewhat puzzled, seeing only matching handlers being updated in notify when using --fix.

I do think now that, with your explanation, this may not be a good idea. If a handler comes from a separate collection and is used somewhere in a playbook under notify, fixing this in the playbook would break the connection. Hence, I'll close this out.