-
Notifications
You must be signed in to change notification settings - Fork 613
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
Add Red Hat Enterprise Linux 9 support #1303
Conversation
postgresql::server::config is a classthat may have no external impact to Forge modules. postgresql::server::config_entry is a typethat may have no external impact to Forge modules. This module is declared in 70 of 578 indexed public
|
|
is that all to get EL 9 support? Because that's currently not listed in the metadata.json. |
|
It probably needs more work for real EL9 support. I was going over the places where the OS major version was used and this jumped out. It also needs to set a default PG version in globals.pp. |
|
I fixed a versioncmp check and added the default version in globals.pp. |
|
I must admit I didn't have an EL9 box at hand to actually test this out. That's why I've labeled it as initial support and didn't add it to metadata.json. |
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.
Looks good!
Can't we count on acceptance tests as a base for supporting this OS?
|
Not easily. I started in Beaker but the problem is that you don't have RPMs for EL9 and the EL8 ones don't install. See voxpupuli/beaker-hostgenerator#225 (comment) as well. |
|
Looks like CI here doesn't automatically run tests on EL9 if you add it to metadata. |
|
I think you should also update manifests/params.pp. Please find the below change I made in another PR. |
296915a
to
71b5329
Compare
|
I've chosen to rely on the service_provider param/fact instead. I'm still not sure how PDK determines how to run tests acceptance tests. It states:
So I guess we need Puppetlabs to create a CentOS 9 Stream image on their CI. |
|
This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days. |
|
I'm still blocked on a question to the Puppet folks for actual testing. |
|
I've removed the label for now. This one will have remain on hold until we support the required OS. |
Rather than using static OS definitions, this uses the service_provider parameter or fact (provided by puppetlabs/stdlib).
|
Rebased to resolve conflicts and added RHEL 9 to metadata.json. |
|
@david22swan since you opened #1343, could you take a look a this instead? I'm not sure why EL8 tests timed out. |
|
@ekohl Yeh, this look's good, have re-kicked those tests that failed just to be sure but would be happy to merge. |
|
Like this? Should I also update the (last) commit message or is the current one sufficient? |
|
@ekohl Yeh like that, but could you also remove the CentOS Stream 9 from the title and metadata. edit: Ah, the commit messages is fine as it is |
The comment indicates the checks in server config need to be done on EL6 and older only. The current check would also trigger on EL9. This uses a version comparison on the major version. We can leave out the Fedora check since we don't need to care about Fedora < 7. It also sets the default version on EL9 to version 13. This should also affect CentOS Stream 9, which is supposed to work but not officially verified to work.
|
Not sure what is causing the issue's with the AlmaLinux/Rocky/CentOS 8 tests here but it seems to be consistent. |
|
Yes, but they have existed for a long time I think. Perhaps they commonly failed in puppetlabs-apache, but I recall them timing out very often in some module(s). |
|
Have rekicked the tests with extra debugging |
|
@ekohl |
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
The comment indicates this is something that needs to be done on EL6 and older. The current check would also trigger on EL9. This uses a version comparison on the major version. We can leave out the Fedora check since we don't need to care about Fedora < 7.