-
Notifications
You must be signed in to change notification settings - Fork 78
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
(GH-211) Add support for RHEL 8 #213
Conversation
Hi @ph84172 could you also please add a vagrant box to the Vagrantfile so we can test it works that way. We also need to add a docker image to https://github.com/ghoneycutt/puppet-module-pam/tree/master/spec/acceptance/nodesets and https://github.com/ghoneycutt/puppet-module-pam/blob/master/.travis.yml which I forgot to do for SLES15. |
@ghoneycutt is there a way to specify a different container registry in the nodesets file? Redhat publish their "Universal Base Image" which is based on RHEL8 but it's hosted on registry.access.redhat.com rather than Docker Hub. |
Alternatively we wait until CentOS 8 has been released and there's a centos8 image on Docker Hub :) |
Try specifying the registry through the environment variable DOCKER_REGISTRY="registry.access.redhat.com" \
BEAKER_set="el-8" \
BEAKER_PUPPET_COLLECTION=puppet6 \
bundle exec rake beaker |
If that does not work, suggest using one of the unofficial images from docker hub. |
Hi @ph84172 Seems we just have these two left before we can merge. For the docker image, suggest using an unofficial one and putting a comment in there with a link to this issue that explains why it is being used and that we are waiting on the BKR ticket to use the official one.
|
@ghoneycutt I've tried adding unofficial RHEL 8 Vagrant boxes / Docker images - does this run through Travis correctly? |
Vagrant is done manually and the travis config update looks great. Before a release, I'll run https://github.com/ghoneycutt/puppet-module-pam/blob/master/tests/vagrant_test_all.sh and ensure that you can apply this code on a vanilla platform and still login through ssh. |
It's all yours, thanks Garrett. Might be worth thinking about switching the box/container images over to CentOS 8 when it's released to keep it consistent with the previous use of CentOS 7 as the canonical "EL7" target. |
Docker and Vagrant failing because you need credentials to use yum. Can we work around this?
|
I've removed the "yum -y install wget" in the nodeset file as wget is already installed in the base RHEL 8 environment. I also reworked provision_basic_el.sh into a new provision_basic_rhel.sh for the RHEL 8 target which again, removes the wget reference and removes references to EPEL (doesn't exist yet for RHEL 8) and CentOS repositories. |
I've also renamed the Vagrant machine from "el8-pam" to "rhel8-pam" thinking that it's probably better to start distinguishing explicitly between CentOS (el) and RHEL (rhel). |
Ping @ghoneycutt - how's this PR looking now? Thanks. |
@ph84172 Could you please rebase and I'll get this merged. Great work on this PR! |
@ghoneycutt looks like Beaker tries to do a "yum install" of some openssh components once the container is up, which will fail without a valid RH subscription. Given that we can't be too far away from a CentOS 8 release I'm wondering if we should nix this until then? We could either add support for EL 8 without functioning tests for now and put it on the "unsupported" list or just wait for CentOS 8 and try this PR again then. What do you think? |
Seems like a reasonable approach to put them in unsupported, pending centos 8 release. Besides the README, could you update travis with the an allowed_failures section and put a comment that these should be changed to centos 8 and link back to this pull request. That should hopefully make it apparent to everyone what's going on and when to expect support. here's an example https://github.com/sensu/sensu-puppet/blob/master/.travis.yml#L140-L144 |
@ghoneycutt thanks - now updated. |
@ph84172 Drats! Noticed a typo with centos7 vs 8. Could you please update/rebase and then I'll get this merged for real :) |
@ghoneycutt should be good to go 👍 |
Ping @ghoneycutt 😊 |
HI @ghoneycutt, Is there anything preventing this from being merged? We would love to see RHEL8 support added :) |
Hi again, After some further investigation is appears we might need to do further changes for this pull request. In RHEL8 they've introduced authselect which appears to be a new way of customizing PAM configuration. It allows you to have multiple PAM configuration and select which PAM configuration should be used. Current implemention (old way) still appears to work however maybe we should try to align us to RedHat's way of working? This could make it easier to introduce non-default setups as well such as QAS. |
It's certainly worth revisiting - the current PR had some workarounds for the fact there were no CentOS 8 images around at the time. |
Thanks everyone! |
I've tested this on a fresh install of RHEL 8 and it appears to be working correctly.
Garrett, I'm not sure how you want to handle the vagrant image for this test as there's no CentOS 8 yet. There does appear to be a vagrant box for RHEL 8 but I haven't tested it.