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 ansible_os_family instead of testing distribution directly #120

Closed
wants to merge 2 commits into from

Conversation

archoversight
Copy link

This will catch the entire Red Hat "family" as it were rather than
limiting to just CentOS, thereby allowing this role to work against RHEL
8.

This will catch the entire Red Hat "family" as it were rather than
limiting to just CentOS, thereby allowing this role to work against RHEL
8.
@dobbymoodge
Copy link
Contributor

@archoversight IIRC I tried using that at first, and for some reason had to replace it with the more complex check in #117 . I can't remember what that reason is though, and haven't time to check it right now.

@dobbymoodge
Copy link
Contributor

dobbymoodge commented Sep 22, 2020

I think I remember now - ansible_os_family is set to RedHat for Fedora, and so the conditional in your PR is True for all versions of Fedora past 8. The conditional I wrote gets the precision the role needs.

@archoversight
Copy link
Author

@dobbymoodge with the downside that it doesn't work for RHEL systems... It sounds like Ansible considering Fedora RHEL family is what's broken here.

@dobbymoodge
Copy link
Contributor

@dobbymoodge with the downside that it doesn't work for RHEL systems... It sounds like Ansible considering Fedora RHEL family is what's broken here.

Um, I think the opposite? My PR is specifically to fix this properly for RHEL systems. Without my PR, the role fails on RHEL 8, which is my team's deployment platform. My change correctly specifies the version check for Fedora versions at or above 27, and CentOS and RHEL systems at or above 8.

@archoversight
Copy link
Author

@dobbymoodge with the downside that it doesn't work for RHEL systems... It sounds like Ansible considering Fedora RHEL family is what's broken here.

Um, I think the opposite? My PR is specifically to fix this properly for RHEL systems. Without my PR, the role fails on RHEL 8, which is my team's deployment platform. My change correctly specifies the version check for Fedora versions at or above 27, and CentOS and RHEL systems at or above 8.

The check specifies CentOS, which is not the name used for ansible_distribution when the system is a RHEL 8 system, or so I assume. I'd have to go add a debug statement to verify.

This is the reason I created the PR in the first place because while deploying against a RHEL 8 system it failed to properly set the facts for setting the appropriate path.

@kyl191
Copy link
Owner

kyl191 commented Dec 5, 2020

I merged #117 since that's closer to the intent of listing specific versions that we know are compatible.

My intent was more along the lines of "Fedora split openvpn configs into server and client, and CentOS followed". RHEL missing from that list was an oversight.

That said, I don't think the answer is to say "all RedHat type releases past 8 should used the systemd logic", since that isn't true.

It might be enough to say "if it's a RHEL-type, and using systemd, then use the systemd logic". I'm continuing that in #129, closing this for now.

@kyl191 kyl191 closed this Dec 5, 2020
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.

None yet

3 participants