search

geerlingguy / ansible-role-repo-epel

Ansible Role - EPEL Repository for RHEL/CentOS
https://galaxy.ansible.com/geerlingguy/repo-epel/
MIT License
185 stars 149 forks source link

Use namespaced ansible_facts #44

Closed cognifloyd closed 3 months ago

cognifloyd commented 3 years ago

If ansible's INJECT_FACTS_AS_VARS is disabled, then this role will not work. So, use the namespaced version under ansible_facts.

https://docs.ansible.com/ansible/2.8/reference_appendices/config.html#inject-facts-as-vars

geerlingguy commented 3 years ago

@cognifloyd - This is the first time I've ever seen INJECT_FACTS_AS_VARS; what's the use case for that? There are thousands of roles (and hundreds I maintain) using the built-in facts with ansible_ prefix, and if this is something that's in common use, I'm surprised nobody else has ever complained about it...

Most of the blog posts and docs online refer to the ansible_ facts too (e.g. the seminal result that is always at the top of Google results: https://raymii.org/s/tutorials/Ansible_-_Only_if_on_specific_distribution_or_distribution_version.html).

cognifloyd commented 3 years ago

This was introuduced in Ansible 2.5 (released in 2018). Here's the relevant section of 2.5 changelog's Major Changes (added in https://github.com/ansible/ansible/commit/9c629f8a1ccadd0e07c6f7ba8953a021db6a176c):

Added fact namespacing; from now on facts will be available under ansible_facts namespace (for example: ansible_facts.os_distribution) without the ansible_ prefix. They will continue to be added into the main namespace directly, but now with a configuration toggle to enable this. This is currently on by default, but in the future it will default to off.

Reasoning for this change is in https://github.com/ansible/ansible/pull/18445#issuecomment-259807665

it avoids collisions with existing vars by making sure all facts are restricted to ansible_facts.

And here's a section of the 2.5 porting guide. However the doc change wasn't added to the 2.5 porting guide until 2.7:

I follow the porting guides. As such, I switch to new behavior as soon as possible so that new things I write have the longest life span possible; I ensure that I have this setting as True when I run my playbooks so that I'm actively working with the new behavior instead of being passively surprised when the default changes on me.

This change is safe for ansible 2.5+. Only roles that need to support <2.4 (EOL) need to continue supporting the old fact names. So, the choice is to remain backwards-compatible and risk forwards-compatibility, or to switch to the namespaced facts and be fully forwards-compatible but only backwards compatible down to ansible 2.5.

If you don't want to use the namespaced vars, that's a fair choice. In that case, I will look for an alternative role, or continue to maintain another EPEL role.

cognifloyd commented 3 years ago

Also, the minimum ansible version was bumped in #45 from 2.4 to 2.5 to accomodate the version test (instead of version_compare). That's no longer needed since you removed the fingerprint addition, so if you still need to support 2.4, then that should be dropped back down to 2.4. If you don't need 2.4, then I hope you'll merge this; using ansible_facts namespace is the best-practice in all of the ansible docs.

cognifloyd commented 3 years ago

Rebased on master

stale[bot] commented 3 years ago

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark pull requests as stale.

stale[bot] commented 3 years ago

This pull request has been closed due to inactivity. If you feel this is in error, please reopen the pull request or file a new PR with the relevant details.

stale[bot] commented 1 year ago

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the PR will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark pull requests as stale.

github-actions[bot] commented 10 months ago

This pr has been marked 'stale' due to lack of recent activity. If there is no further activity, the issue will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark issues as stale.

cognifloyd commented 10 months ago

@geerlingguy Do you have a plan for this PR? The stalebot is vociferously opposed to it.

github-actions[bot] commented 5 months ago

This pr has been marked 'stale' due to lack of recent activity. If there is no further activity, the issue will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark issues as stale.

github-actions[bot] commented 3 months ago

This pr has been closed due to inactivity. If you feel this is in error, please reopen the issue or file a new issue with the relevant details.