name[casing] incorrectly catches notify triggering listen

[guidograzioli]

Summary

The following raises case[naming] with "Task notify 'restart amq_broker' should start with an uppercase letter."

- name: Create acceptor configuration in broker.xml
  middleware_automation.common.xml:
    path: "{{ activemq.instance_home }}/etc/broker.xml"
    xpath: /conf:configuration/core:core/core:acceptors
    input_type: xml
    set_children: "{{ acceptors }}"
    namespaces:
      conf: urn:activemq
      core: urn:activemq:core
    pretty_print: yes
  become: True
  no_log: True
  notify:
    - restart amq_broker

It should not, as the handler is defined as:

- name: "Restart handler"
  ansible.builtin.include_tasks: restart.yml
  listen: "restart amq_broker"

Do not see how or why a style guideline should apply to an internal string

Issue Type

• Bug Report

OS / ENVIRONMENT

ansible-lint --version

ansible-lint 24.5.0 using ansible-core:2.16.6 ansible-compat:24.5.1 ruamel-yaml:0.18.6 ruamel-yaml-clib:0.2.8

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

STEPS TO REPRODUCE

See: https://github.com/ansible-middleware/amq/actions/runs/9107358687/job/25036108750#step:7:1

Desired Behavior

No listen name is reported, name[casing] is not skipped in .ansible-lint

Actual Behavior

Listen name is reported

[ssbarnea]

This bug was introduced by #4149 but we need to consider all the implications before making another change here.

Temporary using noqa: case[naming] on the notify: lines that needs to use lowercase could be the best workaround.

PS. We are still debating internally regarding what to do about this one.

[ssbarnea]

@guidograzioli We had an internal meeting about this, no clear decision yet but reverting the change is on the table. Any suggestions would be welcomed here as that is not an easy to fix bug, any option that we considered still means a problem for some users.

• We cannot programatically know if a listener is defined somewhere (fact)
• Should auto fix name[casing] touch notify and listen?
• We could force everyone to use capital for listen too, as that would make auto-fix safe. Still, it seems bit ugly as the listeners are more like tags/labels and not sentences as task names. Is like forcing this only for technical reasons.

[guidograzioli]

my $.02:

• --fix must touch the task name only: it fixes what it reported. If notify is lowercase, and the task is uppercase with no listen, then it is not a style guideline that is triggered, it's a coding error (not for the linter)
• notify and listen together: they have absolutely zero need for a coding style check on the string. A validity check would be nice (not possible because you cannot know programmtically when a listener is defined), however in any case out of scope for a linter
• in general, I guess work on case is still pending, so it's good to revert the check on notify/listen as a temporary fix. ie: it does not make much sense to force an uppercase letter when other positive good-cases could be checked first (example here: https://github.com/ansible-middleware/ansible-lint-custom-rules/blob/main/rules/TaskHasValidNameRule.py#L36 test that task name start with an imperative action verb)

thanks for asking!

[Denney-tech]

There is also chatter on the forum relating to this.

In my opinion, listeners should not need to follow any strict casing standard. I also think handler names should optionally be excluded, so if the linter can distinguish handlers from regular tasks, it would be nice to have optional skip/warn rules for handlers. (something like name[handler] maybe?)

Similarly to listeners, notify doesn't need to follow any strict casing standards either, especially since ansible-lint doesn't programmatically know if the listeners exist and therefore if the notify has any matches.

edit: Or place this new test for notifiers/listeners under name[notify] so that it can be more easily skipped for those of us that want to.

[kapsh]

Pretty questionable feature. I've always seen listen keyword as a means to have stable identifier instead of handler name which can be modified for clarity, readability, etc. As identifiers, they better follow python-ish naming conventions like "restart_foo_service" — no whitespaces or weird symbols, so less error prone, looks consistent with modules names and tags (and this maybe would be a good rule). Now ansible-lint wants to break relations between notifiers and listeners (I don't think it can --fix both listen and notify automatically? downgraded from 24.5 too hastily to check this) and "Restart_foo_service" simply looks awkward.

In other words, handler topics shouldn't be treated the same way as task names, and assuming codebase is linted, handler names are already checked for name[missing] and case[naming].

[cidrblock]

I tend to agree with the above. I think we should pull the check as the topic should be considered more of an id where the task name is there for readability and documentation.

Ideally, we would be able to suggest that topic are used rather than task names but there's some complexity there that can be addressed easily.

There's a draft up to revert it: #4205 (review)

[cidrblock]

and.... It doesn't always get said, but TY everyone for all the feedback and conversation around this one.

[ssbarnea]

We just merged #4205 which removes the modification of notify and listen. It will be released in few minutes.