Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor fqcn to recommend use of canonical names #2604

Merged
merged 1 commit into from Oct 25, 2022

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Oct 15, 2022

Closes: #2572

Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rule suggests to use community.general.system.ufw instead of community.general.ufw. This is just plain wrong and dangerous!

@ssbarnea
Copy link
Member Author

@felixfontein We will discuss about at Fest, as that is very good opportunity to meet with most of the parts involved.

We are not in a pressure to push this so we will look at all possibilities.

All I know now, is that we do want to "suggest a single form of calling a module at one point in time", for consistency, also for UX when using search or auto-complete.

If I remember correct, likely that 99% of module affected are inside community.general collection, or other community collections. Use of "deep" modules is quite rare outside AFAIK. In fact I could try to get some stats on this myself as I already had a folder will all collections that published a version in the last 12 months.

@geerlingguy
Copy link

geerlingguy commented Oct 23, 2022

Just chiming in that I had no clue there were even modules with such deep structures. I've only ever used names as deep as namespace.collection.module.

As an end user, it seems weird that anything would recommend deeper module names (it just seems wrong to me).

I still don't even think it's an issue to use non-FQCN, because again—as an end user, that's one of the nice things about Ansible, 99% of the module I use don't have to have a FQCN to work ;)

(But I understand that the FQCN ship has sailed and people who like short names will just have to squelch lint in that case, as I already have on any of my older projects. I use FQCN on newer projects, but will probably only slowly update older playbooks just because there's no perceived benefit to me doing so, regardless of micro-optimizing internal Python redirects.)

@ssbarnea
Copy link
Member Author

Not sure if this will ruin your day but presence of FQCN will likely become mandatory for uploading content to galaxy, the main reason being that this will prevent upload of broken content (missing dependencies). Only by using FQCN we can know that the used modules are mentioned as dependencies, dependencies that do exist and are installable.

To give an example if you have collection foo -> bar, you will have to upload bar first, before being able to upload foo. Same will apply for version requirements, so people do not upload requirements that cannot be resolved.

@geerlingguy
Copy link

For new projects it's definitely not an issue; it's just older projects where the time to convert everything is not a few seconds. It would be nice if there were a command like ansible-playbook fqcn-convert or something (or a python script) that could just find and replace each module name using ansible's own remapping. But failing that I've been slowly working my way through some projects, and I can prioritize the ones which are more likely to end up in collections someday.

I assume roles (standalone) won't be affected by the fqcn-galaxy-requirement in Galaxy NG?

@ssbarnea
Copy link
Member Author

I modified the implementation to temporary prevent it from raising fqcn[canonical] for anything under c.g. and c.n., at least until we have some changes made there.

src/ansiblelint/rules/fqcn.py Show resolved Hide resolved
@ssbarnea ssbarnea merged commit f3a3fe4 into ansible:main Oct 25, 2022
@ssbarnea ssbarnea deleted the fix/fqcn-canonical branch October 25, 2022 18:13
@felixfontein
Copy link
Contributor

Thanks for adding the exceptions for c.g and c.n!

@erikgb
Copy link
Contributor

erikgb commented Oct 29, 2022

There seems to be something really wrong with this PR. After upgrading ansible-lint all may plays broke with errors like:

WARNING  Listing 4 violation(s) that are fatal
�]8;id=941877;https://ansible-lint.readthedocs.io/rules/fqcn/�\�[1;91mfqcn[action-core]�[0m�]8;;�\�[2m:�[0m �[91mUse FQCN for builtin module actions (ansible.legacy.hashivault_read).�[0m
�[34mplaybooks/keys-and-csrs.yml�[0m:12 �[2mUse `ansible.builtin.ansible.legacy.hashivault_read` or `ansible.legacy.ansible.legacy.hashivault_read` instead.�[0m

�]8;id=133341;https://ansible-lint.readthedocs.io/rules/fqcn/�\�[1;91mfqcn[action-core]�[0m�]8;;�\�[2m:�[0m �[91mUse FQCN for builtin module actions (ansible.legacy.hashivault_secret).�[0m
�[34mplaybooks/keys-and-csrs.yml�[0m:42 �[2mUse `ansible.builtin.ansible.legacy.hashivault_secret` or `ansible.legacy.ansible.legacy.hashivault_secret` instead.�[0m

�]8;id=48165;https://ansible-lint.readthedocs.io/rules/fqcn/�\�[1;91mfqcn[action-core]�[0m�]8;;�\�[2m:�[0m �[91mUse FQCN for builtin module actions (ansible.legacy.hashivault_secret).�[0m
�[34mplaybooks/roles/etcd_tls/tasks/main.yml�[0m:11 �[2mUse `ansible.builtin.ansible.legacy.hashivault_secret` or `ansible.legacy.ansible.legacy.hashivault_secret` instead.�[0m

�]8;id=970936;https://ansible-lint.readthedocs.io/rules/fqcn/�\�[1;91mfqcn[action-core]�[0m�]8;;�\�[2m:�[0m �[91mUse FQCN for builtin module actions (ansible.legacy.hashivault_secret).�[0m
�[34mplaybooks/roles/rke2_server/tasks/main.yml�[0m:128 �[2mUse `ansible.builtin.ansible.legacy.hashivault_secret` or `ansible.legacy.ansible.legacy.hashivault_secret` instead.�[0m

And none of the suggestions work. If I disable ansible-lint in my pipelines everything runs fine. What's the purpose of adding new breaking validation rules that still runs fine with ansible?

@felixfontein
Copy link
Contributor

@erikgb I guess this is about the modules in https://github.com/TerryHowe/ansible-modules-hashivault. How did you install these? This might be very relevant to reproduce the problems you are experiencing. (Both modules in library/ adjacent to a playbook and modules in collections do not seem to have this problem.)

@erikgb
Copy link
Contributor

erikgb commented Oct 29, 2022

@felixfontein Thanks for your reply. Ansible Modules Hashivault is installed with pip in our in-house ansible image. We have created an image that is used for running various Ansible plays in GitLab-pipelines. Any additional info needed?

@felixfontein
Copy link
Contributor

With that information it is easily reproducable, for example in a new container podman run --rm -it python:3.10 /bin/bash:

pip install ansible-modules-hashivault ansible-lint
cat > playbook.yml << EOF
---
- name: Test
  hosts: localhost
  tasks:
    - name: Test
      ansible.legacy.hashivault_secret:
        secret: giant
        data:
          foo: foe
          fie: fum
EOF
ansible-lint playbook.yml

pip install git+https://github.com/ansible-community/ansible-lint.git#egg=ansible-lint
ansible-lint playbook.yml

It happens with both with the latest released version and with the current development version.

@felixfontein
Copy link
Contributor

Note that ansible-modules-hashivault installs itself into the ansible-core installation, so the modules will be in /usr/local/lib/python3.10/site-packages/ansible/modules/hashivault (in the container). Having subdirectories in ansible.modules is something that was possible in the past, but has last happened for Ansible 2.9, and ansible-lint (and potentially parts of ansible-core itself) probably isn't aware about.

It would probably help a lot more if these hashivault modules would be shipped as a collection...

@felixfontein
Copy link
Contributor

In any case, with the above playbook, module == 'ansible.legacy.hashivault_secret' and self.module_aliases[module] == 'ansible.builtin.hashivault_secret', which causes the wrecked output.

When in the playbook hashivault_secret is used instead of ansible.legacy.hashivault_secret, we have module == 'hashivault_secret' and self.module_aliases[module] == 'ansible.builtin.hashivault_secret', and the message is correct.

@felixfontein
Copy link
Contributor

Well, and while debugging this I realized that this is totally unrelated to the hashivault modules :) Simply use ansible.legay.copy in a playbook instead of ansible.builtin.copy and you get exactly the same screwed-up message.

@felixfontein
Copy link
Contributor

#2634 fixes this.

@erikgb
Copy link
Contributor

erikgb commented Oct 29, 2022

Thanks a lot, @felixfontein! I really appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants