Skip to content
This repository has been archived by the owner on Dec 26, 2020. It is now read-only.

RHEL/OL/CentOS 8 support #242

Merged
merged 1 commit into from Oct 12, 2019
Merged

RHEL/OL/CentOS 8 support #242

merged 1 commit into from Oct 12, 2019

Conversation

axkng
Copy link
Contributor

@axkng axkng commented Oct 1, 2019

RHEL 8 only supports python3, therefore the the package for policycoreutils-module changed.
This commit reflects that change and installs a different package depending on the major release.

@axkng axkng force-pushed the master branch 2 times, most recently from 1797e93 to 0e10e6c Compare October 1, 2019 19:27
@rndmh3ro
Copy link
Member

rndmh3ro commented Oct 2, 2019

Thanks for this PR, @Furragen! Since we don't have automated tests for centos/rhel8 yet, I'd like to test it manually first, when I find the time.

@axkng
Copy link
Contributor Author

axkng commented Oct 2, 2019

That sounds fine.
I also noticed that my PR probably breaks support for Fedora. So I am gonna work on that.

@axkng
Copy link
Contributor Author

axkng commented Oct 2, 2019

So, I tested this locally on all machines and only Amazon Linux had problems.
But also, in the tests, SELinux is not available.
I fired up a VM with Fedora to test if this role still works with my recent changes and it does.

I do not have a machine with Amazon Linux around, but it seems to be the only thing breaking here.
Although the tests which failed were not related to SELinux.

@rndmh3ro
Copy link
Member

rndmh3ro commented Oct 4, 2019

Hey @Furragen,

thanks again for your PR. I noticed that when we merge this PR, we will have three tasks that install selinux-dependencies: one for rhel7 and below, one for rhel8 and one for debian systems.

That's a lot of code duplication for basically the same task. So I propose that we create a variable that holds the names of the packages to be installed and put this variable in the vars/Debian.yaml or vars/Redhat_8.yaml. We then only have one package-task that installs the required dependencies.

What do you think about this? Do you want to implement this?

We could also then do this with the install-tasks in tasks/2fa.yml.

@axkng
Copy link
Contributor Author

axkng commented Oct 4, 2019

Hi @rndmh3ro,
happy to help. Also, thank you (and everyone else here) for writing some great roles.

Regarding your proposal:
You are right, there is a lot of duplicat code and including the vars would be cleaner. I can implement it that way, as it will work for me and most other people.
But for me, there is one tradeoff: it gets hard to overwrite the variable holding the packages, because of variable precedence.

On the other hand, I think this role will be fine with that approach.

Before I implement it: do you really need selinux-support on Debian/Ubuntu?
The last time I checked, selinux-support was rather limited on both distros and Debian switched to AppArmor by default with Buster.

@rndmh3ro
Copy link
Member

rndmh3ro commented Oct 5, 2019

You are right, there is a lot of duplicat code and including the vars would be cleaner. I can implement it that way, as it will work for me and most other people.

I'd love that!

But for me, there is one tradeoff: it gets hard to overwrite the variable holding the packages, because of variable precedence.
On the other hand, I think this role will be fine with that approach.

Yeah, making it possible to overwrite variables is something we strive for. However, as you said, this role should be fine, because a) there was no possibility to overwrite the variables before (and no one seemed to care) and b) package-names should rarely be overwritten.

Before I implement it: do you really need selinux-support on Debian/Ubuntu?
The last time I checked, selinux-support was rather limited on both distros and Debian switched to AppArmor by default with Buster.

Well for now I'd keep it. I'll create a new issue though to check, how we can and should support selinux on debian-systems.

Signed-off-by: Furragen <git@axk.io>
@axkng
Copy link
Contributor Author

axkng commented Oct 5, 2019

So,
after some back and forth with git and basically a restart, I think the changes should now be okay.
What do you think?

@axkng axkng reopened this Oct 5, 2019
@rndmh3ro rndmh3ro merged commit b6f947c into dev-sec:master Oct 12, 2019
@rndmh3ro
Copy link
Member

Thanks @Furragen, great work!

@DonEstefan DonEstefan mentioned this pull request Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants