-
Notifications
You must be signed in to change notification settings - Fork 16
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: Use vars/RedHat_N.yml symlink for CentOS, Rocky, Alma wherever possible #203
Conversation
ba9dfcf
to
69313e0
Compare
Hm is this code actually needed in the Looking at the code, I see only a couple of places of "potential interest":
Summary:
What I'm thinking about is something simpler:
Rich, what do you think? |
Maybe not? But
Right.
Which has also been pointed out by @jjelen linux-system-roles/ssh#163 (review)
Adding OracleLinux to this list is problematic because it does not appear to be a strict clone, which is why I did not add it here - see linux-system-roles/cockpit#130
I guess it doesn't matter for now because we only run the tests on
agreed - but I would prefer to merge this PR anyway for the reasons given above (and removing the Alma and Rocky links for versions < 8)
Probably yes, but I would prefer to make that explicit, if/when someone wants to run tests on Alma et. al.
agreed - but I would prefer to merge this PR anyway for the reasons given above
I would prefer not to modify the tests unless someone wants to run the tests on Alma et. al., and then I would prefer that person would submit a PR.
I would prefer to merge this PR anyway for the reasons given above (and removing the Alma and Rocky links for versions < 8) |
69313e0
to
8e7a8f0
Compare
8e7a8f0
to
234ccd0
Compare
See the Q&A section in the updated description |
…ver possible We have a lot of requests to support Rocky and Alma in various system roles. The first part of adding support is adding `vars/` files for these platforms. In almost every case, for a given major version N, the vars file RedHat_N.yml can be used for CentOS, Rocky, and Alma. Rather than making a copy of the RedHat_N.yml file, just use a symlink to reduce size and maintenance burden, and standardize this across all system roles for consistency. NOTE: There is no Alma or Rocky version 7 or less. NOTE: OracleLinux is not a strict clone, so we are not going to do this for OracleLinux at this time. Support for OracleLinux will need to be done in separate PRs. For more information, see linux-system-roles/cockpit#130 **Question**: Why not just use `ansible_facts["os_family"] == "RedHat"`? **Answer**: This is what Ansible uses as the RedHat os_family: https://github.com/ansible/ansible/blob/1e6ffc1d02559a26def6c9c3b07baf27032865a2/lib/ansible/module_utils/facts/system/distribution.py#L511 There are a lot of distributions in there. I know that Fedora is not a clone of RHEL, but it is very closely related. Most of the others are not clones, and it would generally not work to replace ansible_distribution in ['CentOS', 'Fedora', 'RedHat'] with ansible_facts['os_family'] == 'RedHat' (but it would probably work in specific cases with specific distributions). For example, OracleLinux is in there, and we know that doesn't generally work. The only ones we can be pretty sure about are `RedHat`, `CentOS`, `Fedora`, `AlmaLinux`, and `Rocky`. **Question**: Does my role really need this because it should already work on RHEL clones? **Answer**: Maybe not - but: * it doesn't hurt anything * it's there if we need it in the future * the role will be inconsistent with the other system roles if we don't have this **Question**: Why do I need the `tests/vars/rh_distros_vars.yml` file? Doesn't the test load the vars from the role? **Answer**: No, the test does not load the vars from the role until the role is included, and many tests use version and distribution before including the role. **Question**: Do we need to change the code now to use the new variables? **Answer**: No, not now, in subsequent PRs, hopefully by Alma and Rocky users. Note that there may be more work to be done to the role to fully support Rocky and Alma. Many roles have conditionals like this: ```yaml some_var: "{{ 'some value' if ansible_distribution in ['CentOS', 'RedHat'] else 'other value' }}" another_var: "{{ 'some value' if ansible_distribution in ['CentOS', 'Fedora', 'RedHat'] else 'other value' }}" ... - name: Do something when: ansible_distribution in ['CentOS', 'RedHat'] ... - name: Do something else when: ansible_distribution in ['CentOS', 'Fedora', 'RedHat'] ... ``` Adding Rocky and AlmaLinux to these conditionals will have to be done separately. In order to simplify the task, some new variables are being introduced: ```yaml __$rolename_rh_distros: - AlmaLinux - CentOS - RedHat - Rocky __$rolename_rh_distros_fedora: "{{ __$rolename_rh_distros + ['Fedora'] }}" __$rolename_is_rh_distro: "{{ ansible_distribution in __$rolename_rh_distros }}" __$rolename_is_rh_distro_fedora: "{{ ansible_distribution in __$rolename_rh_distros_fedora }}" ``` Then the conditionals can be rewritten as: ```yaml some_var: "{{ 'some value' if __$rolename_is_rh_distro else 'other value' }}" another_var: "{{ 'some value' if __$rolename_is_rh_distro_fedora else 'other value' }}" ... - name: Do something when: __$rolename_is_rh_distro | bool ... - name: Do something else when: __$rolename_is_rh_distro_fedora | bool ... ``` For tests - tests that use such conditionals will need to use `vars_files` or `include_vars` to load the variables that are defined in `tests/vars/rh_distros_vars.yml`: ```yaml vars_files: - vars/rh_distros_vars.yml ``` We don't currently have CI testing for Rocky or Alma, so someone wanting to run tests on those platforms would need to change the test code to use these. Signed-off-by: Rich Megginson <[email protected]>
234ccd0
to
b469170
Compare
We have a lot of requests to support Rocky and Alma in various system roles. The
first part of adding support is adding
vars/
files for these platforms. Inalmost every case, for a given major version N, the vars file RedHat_N.yml can
be used for CentOS, Rocky, and Alma. Rather than making a copy of the
RedHat_N.yml file, just use a symlink to reduce size and maintenance burden, and
standardize this across all system roles for consistency.
NOTE: There is no Alma or Rocky version 7 or less.
NOTE: OracleLinux is not a strict clone, so we are not going to do this for
OracleLinux at this time. Support for OracleLinux will need to be done in
separate PRs. For more information, see
linux-system-roles/cockpit#130
Question: Why not just use
ansible_facts["os_family"] == "RedHat"
?Answer: This is what Ansible uses as the RedHat os_family:
https://github.com/ansible/ansible/blob/1e6ffc1d02559a26def6c9c3b07baf27032865a2/lib/ansible/module_utils/facts/system/distribution.py#L511
There are a lot of distributions in there. I know that Fedora is not a clone of
RHEL, but it is very closely related. Most of the others are not clones, and it
would generally not work to replace ansible_distribution in ['CentOS', 'Fedora',
'RedHat'] with ansible_facts['os_family'] == 'RedHat' (but it would probably
work in specific cases with specific distributions). For example, OracleLinux
is in there, and we know that doesn't generally work. The only ones we can be
pretty sure about are
RedHat
,CentOS
,Fedora
,AlmaLinux
, andRocky
.Question: Does my role really need this because it should already work on
RHEL clones?
Answer: Maybe not - but:
Question: Why do I need the
tests/vars/rh_distros_vars.yml
file? Doesn'tthe test load the vars from the role?
Answer: No, the test does not load the vars from the role until the role is
included, and many tests use version and distribution before including the role.
Question: Do we need to change the code now to use the new variables?
Answer: No, not now, in subsequent PRs, hopefully by Alma and Rocky users.
Note that there may be more work to be done to the role to fully support Rocky
and Alma. Many roles have conditionals like this:
Adding Rocky and AlmaLinux to these conditionals will have to be done
separately. In order to simplify the task, some new variables are being
introduced:
Then the conditionals can be rewritten as:
For tests - tests that use such conditionals will need to use
vars_files
orinclude_vars
to load the variables that are defined intests/vars/rh_distros_vars.yml
:We don't currently have CI testing for Rocky or Alma, so someone wanting to run
tests on those platforms would need to change the test code to use these.