Ansible-lint's fixation on upper-case initial letters

Project Discussions
1

ansible-lint is an amazing tool! Following its recommendations has helped our large and growing configuration-as-code base age like wine rather than like milk.

And it’s necessarily opinionated, more so than any humans I know. It has to be, though, in order to nip sloppy practices in the bud before they flower and fruit into a harvest of technical debt. You can reclassify individual errors as warnings or even ignore them altogether in cases you decide either aren’t important to you or that you have not yet fixed across your projects. Ultimately you retain responsibility for the quality of your code base.

With ansible-lint having such strong opinions, the onus is on the gate-keepers of its rules for getting it right.

For example, when ansible-lint complained that our task names “should start with an uppercase letter” we groaned quite a bit as this violated our internal style guide at the time wherein any strings that were “ours” — such as variable, group, and task names — started with lower-case letters. We reclassified it as a warning and, over time, changed all our task names accordingly.

Some tasks are handlers. We were already unhappy notifying handlers by their task names, as those names were too free-form for our liking anyway. Now we needed to change them all to meet the name[casing] rule. Fortunately, handlers support listen: so a handler could go from this:

- name: apply pending changes

to a much more disciplined set of constructed notify strings, like this:

- name: Apply pending changes
  listen:
    - mw_tablinx_install_pending_changes
    - mw_kronos_bounce_config_tier

These more disciplined notify strings embody an indication of their containing roles and their relationships to notifying tasks, while the for-human-consumption task names satisfy ansible-lint’s name[casing] requirements. It’s a win-win all around.

Except that, after a recent upgrade, ansible-lint is now applying the same name[casing] rule to notify strings as it applies to task names. This is a case where, in my opinion, ansible-lint goes beyond opinionated; it’s just flat out wrong. These strings are not English (or pick your language) phrases. They are functional character strings. Yet because ansible-lint is treating them with the same rule it uses for task names, we can’t ignore the name[casing] rule for notify strings without also ignoring it for task names. Notify strings are much closer to variable and role/collection names than they are to task names. This feels like an opinionated tool expressing an opinion simply because it can rather than either to accomplish a positive goal or to avoid identifiable issues.

The easy fix would be to create a different rule for notify strings separate from those for task names. Then we could ignore it and move on. I do have to wonder, though, exactly what problem such a rule for notify strings is hoping to avoid, and whether such problems match those created by the same apparently aesthetics-based opinions-as-code embodied in ansible-lint.

4 Likes
2

This is where you went wrong. It was a pointless rule that also actively went against your existing conventions, so you should have ignored it entirely. You are not required to share a linter’s opinions.

1 Like
3

Irrelevant to the point, which is that “the onus is on the gate-keepers of [ansible-lint’s] rules for getting it right” — “it” being “which things to express an opinion about.” This is why I put this under Project Discussions rather than Get Help. I know how to ignore flaws, both in my own code and in ansible-lint itself. The intent is to help improve the latter.

Furthermore, I’m suggesting the ansible-lint project needs clearly articulated guidelines for including or excluding proposed rules, and for re-evaluating existing rules as the Ansible ecosystem evolves.

I’ve pointed out one particular rule which is flawed in at least two ways. Had such guidelines been in place, such a rule would never have made it to production, and there would be a mechanism for re-evaluating its implementation if not its very existence. Instead we have statements along the lines of, “You are not required to share a linter’s opinions” which supposedly provides blanket indemnity for any opinion-as-code that might be expressed. It’s a cop-out, and I believe the project should strive for a higher standard.

As for this particular rule: it manifests thus:

name[casing]: Task notify 'some_string' should start with an uppercase letter.

Problems with this rule:

  • It conflates “notify:” strings with handler task names with respect to “name[casing]”. You can’t ignore the “notify strings” issue without also ignoring the “task name” issue. There is insufficient granularity in the configuration because they are both tied to “name[casing]”.

  • Because notify: strings must match either the “name:” or one of the “listen:” strings of some handler, there is nothing a task author can to do “fix” a flagged “notify:” string. To be of any use, such a rule should be triggered in the handler definition, not the notifying task. Given that handlers may be defined in shared resources over which the task author has no control, this particular rule as implemented loses any degree of helpfulness and exists solely along the annoying axis.

  • Requiring both (handler) task names and notify strings (notification “topics”) to start with upper-case letters precludes quite useful innovation in generation of notification strings from the working environment, for example device names, storage or resource classes, etc. It’s as if notification strings are expected to function only as “additional handler names” , which is regrettably short-sighted.

4

I agree that the current way that ansible-lint comes up with new rules is very problematic. If ansible-lint realy is the Ansible linter, then there needs to be a transparent process of how new rules are created or existing rules are adjusted that allows all interested parties to participate.

Right now the only way to participate is if you monitor all PRs in the ansible-lint repository, since there is no other place where proposed rules or rule changes are discussed. There used to be a category in the GitHub Discussions for discussing new rules (ansible/ansible-lint New Rules · Discussions · GitHub), but that doesn’t seem to be very active, and also it’s not so easy to monitor since GH Discussions are used for other things as well, and you cannot subscribe to a single category. (I’m also not sure whether I really would want to watch the whole stream of all rule addition/change discussions, but rather the subset of the ones that have a chance of actually getting implemented. That sounds more manageable to me, as someone who is interested in linting rules, but doesn’t have that much free time for this topic.)

One thing about such discusisons is that it slows down the pace of how the linter moves forward. But I would argue that this is good thing, since it prevents implemented rules that are damaging, like @utoddl’s above example. In that case, the rule change was considered a bug (reported in `name[casing]` error should be raised for `notify` task param · Issue #4035 · ansible/ansible-lint · GitHub and fixed in Raise name[casing] violation for notify task param by cavcrosby · Pull Request #4149 · ansible/ansible-lint · GitHub). As you can see in the comments on the issue after the PR got merged, there are also others pointing out the same problems. I guess some folks would have pointed them out before merging if there would have been a public discussion that’s easy to find. I really don’t want to monitor all issues and PRs for the ansible-lint repo to find the relevant issues/PRs that discuss rule changes/additions, and my guess is that others don’t want to do that either.

Out of curiosity, does someone know how other linters like flake8 or pylint handle new rules?

5 Likes
5

I was a bit brusk earlier, redirecting back to the points I wanted to address. I may have come across as disagreeing with you, when in fact you were entirely correct.

It is a pointless rule, and it’s a stretch to think that following pointless guidelines would improve the long-term viability of our code base.

Nor is it the only pointless thing in ansible-lint’s quiver. In particular the admonition that variables in task names should occur only at the far right end seems a waste of entropy. What future calamity is that saving us from exactly?

It’s almost like the mechanisms they’ve come up with for implementing rules are so elegant (and, I’m not kidding, they are way cooler than something so mundane has any right to be) that it’s irresistible to throw in checks for any hint of inconsistency regardless of the pointlessness. I’m afraid it’s going to be rather difficult to rid the project of "teh silly" once it’s committed and published.

It’s okay for a tool to have opinions. The art is in its knowing which ones to keep to itself.

2 Likes
6

For some roles I’ve used vars to work around this, I probably should have simply added the rule to the skip list :roll_eyes: .

    - name: "{{ php_task_name }}"
      ansible.builtin.command: "phpquery -d -v '{{ php_version.version }}' -s '{{ php_sapi }}' -M"
      check_mode: false
      changed_when: false
      register: php_mods_existing
      vars:
        php_task_name: "Check existing mods for PHP {{ php_version.version }} {{ php_sapi }} SAPI"
3 Likes
7

I do think there should have been more discussion before my work/PR was merged in, but at this point, it would probably be helpful to bring these thoughts to the maintainers’ attention. I’m not entirely sure where the best place would be for this to also get the maintainers’ attention, but there is a related issue (`name[casing]` incorrectly catches `notify` triggering `listen` · Issue #4168 · ansible/ansible-lint · GitHub) where feedback is requested in concern to this new behavior.

3 Likes
8

@cavcrosby Felix actually linked to that issue earlier already. I went ahead and shared the forum thread on the issue and expressed my opinion on the matter over there.

I’m definitely on @utoddl 's side on this, that listeners for handlers (and in turn notifiers) should not be required to follow any particular casing or naming scheme.

Maybe if the new behavior was under a new rule, such as name[notify], this would be more acceptable since we can add that to the skip list without having to skip the previously existing behavior of the more desirable name[casing] rule.

2 Likes
9

To clarify, I actually posted a different link :slight_smile: I hadn’t seen that issue until @cavcrosby posted it here.

10

Ah, that’s my bad then. I could have sworn it was the same, but I see now they’re different numbers.