Skip to content
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

Use major version if python installed in custom path #65

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

Conversation

dmasteller4
Copy link
Contributor

Switch to using python major version if python is installed on a custom path. Fix recent issues that seem to have risen due to #56 for @sophos-rickc and @sophos-daniels.

@dmasteller4 dmasteller4 requested a review from ssbarnea as a code owner January 6, 2023 22:11
@ssbarnea ssbarnea added the bug Something isn't working label Jan 7, 2023
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I understand the need to address your current issue, I also see that this change contains no testing. Accepting it as such could easily translate into breaking the module for others.

We usually use containers for testing stuff like this. Can you please describe the distro/version/setup on which you encounter the issue, maybe we can see how to add a test for it.

If we can add a test that would fail before your change and pass with your change, we would clearly be more confident to merge it.

@conn
Copy link

conn commented Jan 11, 2023

Why would the fix for issues related to #56 require testing when the original change did not require testing? Falling back to the original behavior seems like a safe and critical change for at least the mentioned Sophos folks.

I've been digging through issues on this projects due to issues with pyenv and Python 3.9+ (though 3.8 seems to work just fine). I'm aware of #35 and #42, and how pyenv isn't a supported use case, but I have to point out that patching in system_python = sys.executable seems to work perfectly fine for my use case (Molecule creating/destroying Docker containers with SELinux set to targeted and enforcing).

The reason why I'm mentioning this completely separate use case is that I think a fallback pattern for selecting system_python has been badly needed for years. I'd love to see this package test for multiple suitable Pythons until it finds one instead of stubbornly sticking with one hard-coded path that's bound to exclude systems.

I'd love to see this change be expanded with an additional fallback so that sys.executable is selected when no other suitable Pythons are found. That way, more use cases can be silently served while also still telling people to install python3-libselinux or that pyenv isn't supported when they have issues.

@ssbarnea
Copy link
Member

@conn This package needs help, especially in testing area, so we would avoid future regressions. My time is very limited but I would be able to help if someone steps in and starts adding tests.

As some regressions were reported, the first measure is to prevent merging new code that is not well tested, even if the change would mainly be a revert.

I will have more work to do with molecule in the following months, so there is a small change I might need to add fixes myself here. Still, nobody should rely on me having the time.

Shortly, I would LOVE to get others to help with project maintenance and this project is extremely small, it should not require too much of anyone's time to help with testing.

@conn
Copy link

conn commented Jan 11, 2023

Totally understandable.

Do you know where this package is being used outside of Ansible and Molecule? My understanding is that this package isn't being used by Molecule anymore and is only being pulled in by Molecule-Plugins because it's still listed as a dependency. If I can muster some spare cycles, I'd be happy to add some tests but I'm admittedly ignorant about where this package is being used.

@ssbarnea
Copy link
Member

ssbarnea commented Jan 11, 2023

I think that it could make sense to remove selinux from molecule-plugins and require the users to install it when needed. It can cause some headaches for them but at the same time it can same some for those that do not really need it.

See ansible-community/molecule-plugins#89

@conn
Copy link

conn commented Jan 18, 2023

Thank you for that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants