-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
(MODULES-11075) Improve future version handling for RHEL #2174
Conversation
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.
All changes to params.pp look correct. Perhaps we can add CentOS Stream 9 facts to https://github.com/voxpupuli/facterdb and get some early testing set up.
e73dcea
to
0cab5bb
Compare
|
@mwhahaha Apologies, but this isn't something that can be merged right now. Support/compatibility for RHEL and CentOS 9 in our modules can only be added after they are supported in the agent. Unfortunately this means I will have to close this PR, but I would first like to say thank you for putting in the thought and the work that you have and I hope to see more from you in the future. |
|
I think disagree with not merging it altogether. IMHO we can change params.pp to be more future compatible. We wouldn't claim it in metadata.json, but there's no harm in the params.pp changes. |
apache::params is a classBreaking changes to this file WILL impact these 16 modules (exact match):Breaking changes to this file MAY impact these 11 modules (near match):This module is declared in 175 of 578 indexed public
|
| '7' => "${httpd_dir}/conf.modules.d", | ||
| '8' => "${httpd_dir}/conf.modules.d", | ||
| default => "${httpd_dir}/conf.d", | ||
| '6' => "${httpd_dir}/conf.d", |
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.
Where are we today with EL5 support? Is it really dead or should we keep some code for it?
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.
It's no longer in the metadata.json and 5 has been EOL since November 30, 2020 per https://access.redhat.com/support/policy/updates/errata#Life_Cycle_Dates. I can add it back in but I think 6+ is the current support list?
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.
I agree and I'd love to see it die, but the support policy of Puppetlabs' modules is not 100% matching metadata.json (which always confuses me). For example, there's still conditionals left in the codebase. That always makes me cautious with removing it.
|
We need this landed so we can get EL9 started using the community version. I would appreciate not holding on to this because it blocks our EL9 work. |
|
And for the record I'm core on the openstack-* modules which is where we want to consume these changes as we work on getting openstack spun up on CentOS9 |
|
ok i can go back and remove this optimization |
|
I'd like someone from the modules team to weigh in, but sadly I can't ping them. |
|
@mwhahaha @ekohl If you wish to remove these parts of the pr however and push the changes up, either here or in a fresh pr, they look good to merge in my eyes. Once again apologies that we are not able to merge your full changes at this time. |
|
K i'll update this one and just frame it as a future proofing/optimization |
This change updates the version handling to better future proof the logic when newer versions are shipped. This should prevent having to add things like puppetlabs#2063, puppetlabs#2021, puppetlabs#2038, etc.
|
@david22swan Will this suffice? |
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.
LGTM
Thank you for getting back so fast on the changes, and once again I am sorry we couldn't get in all that you wanted.
Will merge once the test's have finished their run.
Hope to see more from you in the future :)
This change updates the version handling to better future proof the
logic when newer versions are shipped. This should prevent having to add
things like #2063, #2021, #2038, etc.