-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fixes #29190 - Support EL8 #77
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.
In general you only do this if it's really supported. We have acceptance tests so please make sure they also run on EL8.
Currently the acceptance tests are blocked because With that said, I obtained the package from http://koji.katello.org/koji/taskinfo?taskID=295341 and manually installed it on my test el8 system. Once the package is installed, all acceptance tests are successful. |
theforeman/foreman-infra#1290 is open for that. IMHO we shouldn't merge partial support. |
.travis.yml would need EL8 added to it so that the EL8 versions of the acceptance run, the pulpcore-selinux issue has been fixed |
bc50204
to
eb08b03
Compare
|
quick question for @ekohl -- do we need to support Puppet 5 with el8 or is it safe to assume el8 will be at least Puppet 6? |
We should support Puppet 5 as well and there's no plan to drop that. Satellite also ships with Puppet 5 and there are some minor differences so I'd still treat it as first class supported. |
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.
Please add it to https://github.com/theforeman/puppet-pulpcore/blob/master/.sync.yml so it won't be overwritten in the next modulesync.
It was fine to change |
|
The acceptance tests are doing their job :) Looks like a packaging issue. |
When I run the tests locally as EL7 or EL8 I am seeing an selinux relabeling happening that breaks idempotency, e.g.
|
I dug into the EL8 gridfs issue. The problem lies in what version of pymongo is being installed. In this case, it is installing the version from the python36 because modules will exclude non-modular repositories if those repositories contain updates to a package from a module. To get around this, we need to enable https://dnf.readthedocs.io/en/latest/modularity.html#hotfix-repositories This diff worked for me (the SELinux idempotency is still an issue):
And if you are wondering, the |
This is indeed a limitation we run into with the installer as well. I've asked around and the issue we thought was tracking it, isn't actually that. I need to file a proper bugreport. |
Is there a work around for the bug in our testing we can implement? Installing the packages before the puppet run? |
It's what we do for puppet-foreman: In this case it's too bad that it includes the We don't see it in CI because that's running on Docker which doesn't support SELinux. I guess we could only install the packages in the spec helper |
I have a working fix for the relabeling of |
0a9892e
to
e0ee399
Compare
I opened #78 to ensure relabeling of the cache dir is performed after it is created and therefore will not break idempotence. The other idempotence breaking relabeling issue is related to the redis scl (and therefore does not affect el8) and I will resolve it by pre-installing the RPM in the el7 test environment per @ekohl 's suggestion. |
I've changed the PR title to better cover the intent. |
This is failing on installing the migration plugin on EL8. AFAIK there will be no pulp2 on EL8 ever, so I should change the tests to not even attempt installing the plugin on EL8. |
AFAIK you do install the plugin to import content from Pulp 2 servers. |
Thanks. Latest changes should resolve the issue that was blocking it. I've rebased and I'm testing it out. |
Migration plugin should be installable on el8 now |
Tests are 🍏 |
spec/setup_acceptance_node.pp
Outdated
gpgcheck => 0, | ||
include => 'file:///etc/yum/pulpcore.conf', | ||
file { '/etc/yum.repos.d/pulpcore.repo': | ||
ensure => 'present', |
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.
In this case it doesn't matter that much, but I've learned to be explicit with the file
type and state whether it's a file or directory.
ensure => 'present', | |
ensure => 'file', |
spec/setup_acceptance_node.pp
Outdated
} | ||
|
||
# Needed as a workaround for idempotency | ||
if $facts['os']['selinux']['enabled'] { | ||
package { 'pulpcore-selinux': | ||
ensure => installed, | ||
require => Yumrepo['pulpcore'], | ||
ensure => file, |
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 think you meant to change the file resource, not the package one.
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 shouldn't push anything before I've had my coffee
After marking el8 as a supported OS in metadata.json, I can run spec tests using el8 facts: