Skip to content
This repository has been archived by the owner on Nov 2, 2022. It is now read-only.

Changed selinux_packages for CentOS 8 #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tvartom
Copy link

@tvartom tvartom commented Jan 7, 2020

The selinux_package libsemanage-python is replaced by python3-libsemanage in CentOS 8.
I had to tweak the including of OS specific variables to handle different versions. Possibly, it is the same for RHEL 8, but I have no chance to test it, and it is a bit strange to make an include which mixes ansible_os_family with the version of the distribution.

@tvartom
Copy link
Author

tvartom commented Jan 8, 2020

A simpler solution would be to just edit vars/os_RedHat.yml
With:
samba_selinux_packages:
- "{{ 'python3-libsemanage' if ansible_distribution_major_version|int >= 8 else 'libsemanage-python' }}"
if someone can confirm that RHEL 8 also need the same change.
(Above code is not tested)

@ericzolf
Copy link

ericzolf commented Jun 3, 2020

I've suggested a very similar approach in the issue #54 but also only tested on CentOS 8, though I don't see why there should be a difference with RHEL 8.

@ericzolf
Copy link

ericzolf commented Jun 3, 2020

And I realise that it is just a different approach to previous PR #46

@yodatak
Copy link

yodatak commented Jun 5, 2020

Needed for Fedora 32 server too

@aairey
Copy link

aairey commented Nov 13, 2020

I think it would be better to detect the python version on the target and use that to determine which python package should be installed.

For example:

- name: Install SELinux python package
  package:
    name: "{{ samba_python2_selinux_packages }}"
    state: present
  when: ansible_selinux is defined and ansible_selinux.status == 'enabled' and ansible_python.version.major < 3
  tags: samba

- name: Install SELinux python3 package
  package:
    name: "{{ samba_selinux_packages }}"
    state: present
  when: ansible_selinux is defined and ansible_selinux.status == 'enabled' and ansible_python.version.major == 3
  tags: samba

where

samba_selinux_packages:
  - python3-libsemanage

samba_python2_selinux_packages:
  - libsemanage-python

@bertvv do you need some help in maintaining this role?
It seems a lot of PRs are open with valid fixes but nothing is getting merged ...

@ericzolf
Copy link

@aairey basing the decision on the python version could be a good idea, but I don't think it works under ArchLinux and your code could be made simpler:

- name: Install SELinux python3 package
  package:
    name: "{{ samba_selinux_packages[ansible_python.version.major] }}"
    state: present
  when: ansible_selinux is defined and ansible_selinux.status == 'enabled'
  tags: samba

where the variable is a dictionary of lists:

# RedHat
samba_selinux_packages:
  2:
  - libsemanage-python
  3:
  - python3-libsemanage
#ArchLinux
samba_selinux_packages
  2: [ ]
  3: [ ]

Once Python 4 is out, you only add a new key to the dictionary.

@aairey
Copy link

aairey commented Nov 15, 2020

... but I don't think it works under ArchLinux ...

Did you run gather_facts for the target node before reaching this point?
Because that is required to get the ansible facts populated.

@ericzolf
Copy link

Nothing to do with facts, I didn't try as I don't have ArchLinux, but I couldn't find an ArchLinux package containing the name libsemanage even though it uses Python 3. And, generally, I'm trying to avoid successive when statements as it means useless "skip" messages and doesn't scale well with the number of cases.

@aairey
Copy link

aairey commented Nov 16, 2020

@ericzolf it wasn't clear why "it did not work" on ArchLinux from your message.
I suspected it was complaining about the ansible_python variable not being set as this role does not issue a gather_facts.

Anyways, I think you need selinux-python from the AUR on ArchLinux (and selinux-python2 for Python2).
https://wiki.archlinux.org/index.php/SELinux

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants