-
Notifications
You must be signed in to change notification settings - Fork 8
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 #74
Conversation
Also includes some improvements for updating existing PRs - update commit msg, PR title and body |
I believe we remove symlinks when building RPM. We should ensure to keep symlinks that we add in this PR, and downstream I guess it doesn't matter to remove or keep much. |
ok - but I think it already works - some roles have been using symlinks like this for years - which is what gave me the idea to just do this for all roles |
@spetrosi this is ready for review - you can also check one of the role PRs like linux-system-roles/nbde_client#181 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
other than 1 nitpick and one question lgtm
echo git push $force_flag -u origin {{ update_files_branch | quote }} | ||
fi | ||
# commit orig head, then amend, and ensure commit is signed | ||
git commit -C ORIG_HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed so that you can edit existing PRs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now I see, that's smart
…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: 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 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.
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: 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
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/redhat_clone_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.