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

Use fully qualified collection names #1567

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

wbclark
Copy link
Contributor

@wbclark wbclark commented Aug 24, 2022

No description provided.

@wbclark wbclark marked this pull request as draft August 24, 2022 18:32
@wbclark wbclark changed the title Use fully qualified collection names in playbooks/ Use fully qualified collection names Aug 29, 2022
@wbclark wbclark marked this pull request as ready for review August 29, 2022 01:55
@wbclark
Copy link
Contributor Author

wbclark commented Aug 29, 2022

  • Rebased on latest master
  • Also use FQCNs in roles/
  • A couple additional commits to appease ansible-lint

This is now ready for review

@ehelms ehelms requested a review from evgeni August 29, 2022 12:34
@ehelms
Copy link
Member

ehelms commented Aug 29, 2022

Is there a known test that checks and ensures FQCN is used that can be added to a Github Action?

@wbclark
Copy link
Contributor Author

wbclark commented Aug 29, 2022

Is there a known test that checks and ensures FQCN is used that can be added to a Github Action?

The closest I can find is https://ansible-lint.readthedocs.io/en/latest/rules/fqcn-builtins/

That will check for ansible.builtin.$modulename but AFAICT it does not care about using FQCNs like community.general.$modulename, ansible.posix.$modulename, or theforeman.foreman.$modulename.

In the case of roles/forklift_versions/library/forklift_versions.py we have a module which is local to this repository so it seems fine not to use an FQCN for that case.

@evgeni
Copy link
Member

evgeni commented Aug 30, 2022

I must admit I am not a fan of fqcn-builtins and writing out ansible.builtin.foo. The only "good" reason for the long notation is that it allows having a local foo and make that NOT override the built in one, but I actually think that everyone who will have a local foo WANTS to override ansible.builtin.foo intentionally.

That said, the PR looks good and I will not block it just because I find that rule silly ;-)

roles/umask/tasks/main.yml Outdated Show resolved Hide resolved
@wbclark
Copy link
Contributor Author

wbclark commented Aug 31, 2022

@ehelms @evgeni @ekohl Thanks for the comments. Please let me know if there is any additional feedback.

@ekohl
Copy link
Member

ekohl commented Aug 31, 2022

I can follow @evgeni's reasoning what fqcn-builtins is a very strict rule with minimal benefit. Have you considered disabling the lint rule instead?

@wbclark
Copy link
Contributor Author

wbclark commented Aug 31, 2022

I can follow @evgeni's reasoning what fqcn-builtins is a very strict rule with minimal benefit. Have you considered disabling the lint rule instead?

My preference is to default to the long notation. For one thing, it makes it a bit easier to find documentation and have confidence that I'm look at exactly the right module docs; it's also helpful for recognizing dependencies on other collections.

Over time, as more projects use ansible-lint 6 or newer, it could become jarring to other contributors to see only module short names here. They may grow used to using FQCNs and submit PRs adhering to that standard. If we did not ask them to stick to short names, then we would end up in the worst case scenario -- a basically random and unplanned mixture.

In the interest of avoiding a mixture of standards, it seems easier to just enable the rule and let CI enforce it so that reviewers can focus on the more important things.

Also we could disable the rule in CI, but I am not sure whether there exists a simple way to enforce ansible-lint configuration in the user's local repository when that is likely going to be installed inside their venv.

Anyway, I was using the long notation already in other roles I was working on, and wanted to submit some of that work here but saw that the standards were all over the place. I'm learning a new (to me) editor at the moment (kakoune, which is great) so this yak shaving was a weekend project to build skills and muscle memory with that editor, and to try to achieve some consistency between the many playbooks and roles in this repository before I start adding to them.

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

Successfully merging this pull request may close these issues.

4 participants