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

Upgrade fqcn-builtins rule into fqcn #2505

Merged
merged 3 commits into from Sep 29, 2022
Merged

Conversation

ssbarnea
Copy link
Member

@ssbarnea ssbarnea commented Sep 27, 2022

  • rename tag from fqcn-builtins to fqcn[action-core] and add alias
  • update documentation with examples
  • add fqcn[action] for those that are not using known builtins
  • documented fqcn[action-redirect] for redirects (implementation in follow-up)
  • added the two new checks default warn_list

Copy link
Member

@trishnaguha trishnaguha left a comment

Choose a reason for hiding this comment

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

@ssbarnea Can we also update this doc with FQCN?

@ssbarnea
Copy link
Member Author

@ssbarnea Can we also update this doc with FQCN?

This includes changing documentation.

@ssbarnea ssbarnea force-pushed the fix/fqcn branch 3 times, most recently from 93bc18d to df153ce Compare September 28, 2022 14:04
@samccann
Copy link

@ssbarnea - why change to action-internal? From an external perspective (aka I don't deal with lint much - when you say fqcn-builtin - I know what you're talking about - the builtin modules in core. Changing it to action-internal, frankly, made me go check to see do we have some 'action plugins' I didn't know about?

@ssbarnea
Copy link
Member Author

@samccann That was after I had a chat yesterday with @bcoca and looked at the fact that the remediation of the error is to add either ansible.legacy. or ansible.builtin. prefixes. Using builtin was seen bit misleading, especially as the default behavior of ansible is to use the legacy one when the fqcn is absent. Especially as this rule was out for a long period of time I do find builtin easier to understand. Few others from my team mentioned the same, so we might be going for fqcn[builtin-action] to make even more natural.

Keep in mind that in the near future we will also see fqcn[filter] and fqcn[filter-builtin] as filters have the same challenge. Let me go back to him to see if that would be ok.

If someone ever claims that naming is easy, they should start to contribute to the linter. Sometimes we need to spend more time on naming than in implementing the code for it.

@bcoca
Copy link
Member

bcoca commented Sep 28, 2022

filters, tests, lookups, strategy .. in the end any plugin that can appear in a playbook

@samccann
Copy link

Ok read the docs section in the PR a bit more. I understand the need to distiguish between ansible.builtin and ansible.legacy but now I'm confused why we are calling modules actions now?

@bcoca
Copy link
Member

bcoca commented Sep 28, 2022

modules are the code that 'most of the time' runs because of actions, tasks have always had actions:

action: module=X was the original way to write them (also local_action: module=X), then for shorthand we added action: X ... then even shorter X: but it is always 'the task action' .. which uses an action plugin that CAN but does not require, using a module (though most of the time you end up using a module).

ssbarnea and others added 3 commits September 29, 2022 13:03
- rename tag from fqcn-builtins to fqcn[builtin]
- add alias for old name
- update documentation
- add fqcn[module] for those that are not using known builtins.
hswong3i added a commit to alvistack/ansible-role-swap that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-postgres that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-locales that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-kube_kubeadm that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-kernel that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-ceph_common that referenced this pull request Sep 30, 2022
@hswong3i
Copy link
Contributor

@ssbarnea I have a role (https://github.com/alvistack/ansible-role-ceph_mon) with custom library:

$ tree tasks/ library/
tasks/
├── debian.yml
├── main.yml
├── redhat.yml
└── suse.yml
library/
└── ceph_key.py

And so I reference my custom library from tasks/main.yml as below (https://github.com/alvistack/ansible-role-ceph_mon/blob/master/tasks/main.yml#L109-L126):

- name: create mon. keyring
  ceph_key:
    name: "{{ item.name }}"
    dest: "{{ item.dest }}"
    caps: "{{ item.caps | default(omit) }}"
    owner: "{{ item.owner | default('ceph') }}"
    group: "{{ item.group | default('ceph') }}"
    mode: "{{ item.mode | default('0400') }}"
    state: "present"
    import_key: false
  loop:
    - name: "mon."
      dest: "/var/lib/ceph/tmp/ceph.mon.keyring"
      caps: { mon: "allow *" }
  when: inventory_hostname == ansible_play_hosts[0]
  delegate_to: "{{ ansible_play_hosts[0] }}"
  notify:
    - ceph_mon | systemctl restart ceph-mon.target

With this new rule I could only manage to disable the check for it now with # noqa fqcn[action].

Any suggestion for not doing such skip, but rewrite to a correct syntax with our up-to-date standard?

hswong3i added a commit to alvistack/ansible-role-ceph_mon that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-ceph_mgr that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-ceph_osd that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-ceph_mds that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-ceph_rgw that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-containerd that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-cri_o that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-docker that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-etcd that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-flatpak that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-git that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-hibernate that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/ansible-role-kube_master that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/vagrant-ubuntu that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/vagrant-devel that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/vagrant-rhel that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/vagrant-centos that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/vagrant-ceph that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/vagrant-debian that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/vagrant-fedora that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/vagrant-kubernetes that referenced this pull request Sep 30, 2022
hswong3i added a commit to alvistack/vagrant-opensuse that referenced this pull request Sep 30, 2022
ssbarnea added a commit to ssbarnea/ansible-lint that referenced this pull request Oct 5, 2022
* Upgrade fqcn-builtins rule into fqcn

- rename tag from fqcn-builtins to fqcn[builtin]
- add alias for old name
- update documentation
- add fqcn[module] for those that are not using known builtins.

* dnaro suggestion one

* More fixes

Co-authored-by: Don Naro <dnaro@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants