Consider reverting fqcn-builtins rule default

[geerlingguy]

Summary

In version 6.0.0, the new fqcn-builtins rule was made implicit (see #1908), which throws a fatal error in almost all my projects (except for some of the newest ones where I do use FQCN for everything):

Error: fqcn-builtins Use FQCN for builtin actions.

I would argue this is a nitpicky rule for a few reasons, and at most should default to an info/suggestion, maybe warning or opt-in if you're really strict about it.

I wasn't going to post an issue for it, but after thinking it over more, I thought I'd at least post here with my reasoning so it would be here even if the issue is closed as a wontfix.

Yes, it's easy enough to throw fqcn-builtins into my skip_list. And yes, if you're starting with Ansible today and copy and paste examples from the docs which typically use FQCN, it's probably not that big a deal.

But I still would argue it's perfectly valid as a default behavior to use module shorthand for the builtins, because having to type in ansible.builtin (even with a good IDE filling it in...) a zillion times for the many playbooks that have a few command, package, pause, debug, etc. in them is excruciating.

Because shorthand references are allowed—and I don't think deprecating them will happen any time soon—I don't think it's that important to force the usage of FQCN's for builtins. Especially since in the same linted playbook, other FQCNs aren't enforced (e.g. if I'm also using the kubernetes.core collection, and I reference the k8s_info module, there's no warning saying "must use FQCN for collection kubernetes.core".

So at this point I could follow ansible-lint's suggestions and use FQCN for builtins, but not for other collections, and my playbook would look worse than if I just used shorthand for everything...

Issue Type

• Bug Report

Ansible and Ansible Lint details

ansible --version

ansible-lint --version

• ansible installation method: one of source, pip, OS package
• ansible-lint installation method: one of source, pip, OS package

OS / ENVIRONMENT

macOS, Ubuntu, and Debian (all).

STEPS TO REPRODUCE

Have a task like:

- name: Run .osx dotfiles.
  command: "{{ osx_script }}"
  changed_when: false

Run the playbook. Get a warning like:

Error: fqcn-builtins Use FQCN for builtin actions.

Desired Behavior

Instead of an error, it should be a Warning or Info-level message, so it doesn't fail a lint run.

Actual Behavior

It throws an error and a failure.

[geerlingguy]

I see from the original issue where the rule was added (#1614), @tadeboro had the same thought:

I like the rule (for the playbooks I help maintain, we use FQCNs in all tasks). But this rule should definitely be disabled by default because as far as I know, there is no plan to deprecate bare name usage.

[konstruktoid]

I don't mind fqcn-builtins and will enable it myself if it's disabled, but your arguments make sense, and you got valid points.
Especially since bare name usage isn't being deprecated in the near future, and when it it, just change the severity.

[MarkusTeufelberger]

I'd rather have an "always fqcn" rule than limiting this to builtin modules tbh. - it seems like a weird half solution as you already mentioned.

[acdha]

Strong +1 unless there's a tool to trivially rewrite all of those playbooks. I suspect most projects will otherwise do what we just did and add fqcn-builtins to the skip list so the build passes again and probably won't revisit until the short forms are actually deprecated upstream.

[ssbarnea]

Strong +1 unless there's a tool to trivially rewrite all of those playbooks. I suspect most projects will otherwise do what we just did and add fqcn-builtins to the skip list so the build passes again and probably won't revisit until the short forms are actually deprecated upstream.

That features is already in the pipeline, @cognifloyd is working on it and we should be seeing it as a PR soon. As the entire rewrite feature is new, it takes time to make all required changes in order to allow auto-fixes.

We will require fqcn everywhere, the builtin was only the first step. One major reason that seems to be missed by most is the security aspect. When not using FQCN, the shadowing can produce very weird bugs, sometimes even failing in production.

Use of short form was marked already as discouraged by core and documentation was updated to make use of full form. Core has a very conservative approach with discouraged to deprecated to removal taking many years. Linter on the other hand, comes before these steps, so in the future we will likely see even more rules that are not yet documented as discouraged by core, but they will be documented by us.

As ansible-lint is becoming part of AAP, it now has an entire Ansible team (devtools) working on it and in direct connection with other sister teams such core, galaxy, partners and platform. We review any rule change and its impact on different type of users, much stricter than what was years ago.

We also plan to introduce profiles, like a sets of rules you want to follow. This would make it easier as less likely to need to modify you skip_list on each upgrade.

Still, the implicit linter run will likely be very picky. If someone cannot cope with fixing their own creations, maybe it is time to reconsider their supportability. I know for sure that disabling linters on ancient codebases is not uncommon.

If a role or collection is just published as open-source, it does not mean it is maintained, tested or even working. Code that was a good example to follow in 2012, might be the opposite in 2022.

I recently had to archive a couple of projects and handle few more to others interested in taking care of their maintenance, as I was no longer able to scale and I am very happy with that moves, especially as did not want to become a project hoarder or collapse under stress.

In the end, fqcn-builtins is annoying but pretty basic to address. This is one example of what a maintainer is supposed to do in order to keep his code relevant.

So we are not going to remove or disable rules just because it creates work for others. Still, if we make mistakes and introduce really buggy rules or ones that prove to do active damage, we will disable and even remove them. Check the release notes of v6, you will find one such case where an active rule was made opt-in.

[acdha]

That features is already in the pipeline, @cognifloyd is working on it and we should be seeing it as a PR soon. As the entire rewrite feature is new, it takes time to make all required changes in order to allow auto-fixes. … So we are not going to remove or disable rules just because it creates work for others.

It's great to hear that automatic fixes are coming. I understand that some maintenance is expected over time but would caution around making the developer experience worse to address minor security edge-cases — for example, we use Ansible pretty heavily but we have very little use of modules which aren't built-in and that's been true of almost every Ansible project I've worked on. For that common case, this isn't just normal maintenance where there's a benefit of some sort but but since it touches everything it still needs to be reviewed and tested like an actual improvement would be.

In cases like this where the benefit is minor and long-term, it would be better to wait until the automatic rewrite functionality has been released so the cost of the change is lower than the benefit.

[geerlingguy]

In the end, fqcn-builtins is annoying but pretty basic to address. This is one example of what a maintainer is supposed to do in order to keep his code relevant.

I... don't think that making my production code both harder to read and harder to maintain helps keep it relevant—quite the contrary.

If someone were to introduce a way for a contributed collection to override a builtin, that would IMO be a severe security vulnerability in Ansible, and if that were allowed to happen and not be marked a bug, I'd probably forsake using Ansible in the future.

The key is until and unless Ansible plans to deprecate the short form usage of builtins, I'm not going to default adopt FQCNs for them except where it makes sense.

I still feel like the entire Collections transition was done hastily and with more regard towards AAP/Enterprise consumption, and less for end-user/community usage (case in point: roles and Galaxy are still the neglected step-child in this whole thing, but I'm now veering far off the original course for this issue).

It sounds like you're trying to say "if you don't use FQCN for builtins, you're using old fashioned code"... but what I'm hearing is the same argument people keep trying to make that "if your roles are standalone and not part of a collection, that's old fashioned."

No. It's efficient, and not convoluted/overloaded.

[MarkusTeufelberger]

If someone were to introduce a way for a contributed collection to override a builtin, that would IMO be a severe security vulnerability in Ansible, and if that were allowed to happen and not be marked a bug, I'd probably forsake using Ansible in the future.

If you specify ansible.builtin.ini_file, Ansible will execute https://docs.ansible.com/ansible/latest/collections/community/general/ini_file_module.html without any warnings as far as I'm aware. You'll need to add -vv to see the actual line redirecting (type: modules) ansible.builtin.ini_file to community.general.ini_file

IMHO this rule should at least also resolve these redirects and warn about them, since ansible.builtin is not a stable collection.

Edit: It could be actually relevant to know if the implicit/hidden ansible.builtin.foo --> collection.name.foo redirection is being used or if someone already resolved this and pinned down the dependency (https://github.com/ansible/ansible/blob/devel/lib/ansible/config/ansible_builtin_runtime.yml seems to be the list of actually not ansible.builtin modules)

[jorhett]

Something that in general I think would help is if Ansible lint could produce warnings for old code, and errors for new code such that we can fail the job for new additions without preventing bugfixes on old roles without major rewrites.

[ssbarnea]

Closing as there are no plans to revert this decision. Regarding the suggestion to treat existing code differently, you can search for progressive move in documentation.

[rkrisztian]

FQDNs will destroy the readability of Ansible playbooks. What we need is not FQDNs but imports. Imagine Java code without imports...