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

Avoid python libselinux bindings being hidden by tox #1823

Merged
merged 2 commits into from
Mar 14, 2019
Merged

Conversation

ssbarnea
Copy link
Member

Adds libselinux python extensions to the default Dockerfile template
used by molecule in order to avoid failure to build it on selinux
systems.

Fixes: #1724
Signed-off-by: Sorin Sbarnea [email protected]

@ssbarnea ssbarnea self-assigned this Mar 11, 2019
@ssbarnea ssbarnea added the bug label Mar 11, 2019
@ssbarnea ssbarnea changed the title Assure python libselinux extensions are installed [WIP] Assure python libselinux extensions are installed Mar 11, 2019
@ssbarnea ssbarnea force-pushed the fix/libselinux branch 2 times, most recently from 713f314 to 710b9c4 Compare March 11, 2019 21:31
@ssbarnea ssbarnea changed the title [WIP] Assure python libselinux extensions are installed Avoid python libselinux bindings being hidded by tox Mar 11, 2019
@decentral1se
Copy link
Contributor

decentral1se commented Mar 12, 2019

Adds libselinux python extensions to the default Dockerfile template

I don't see any addition to the Dockerfile template!?

Also, it is unclear to me why you're making a change to our Tox setup here when the original report was about failing to build a docker image.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

This comment points out that using system site-packages may cause conflicts: #1724 (comment)

tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated
# sitepackages=true <- template module <- libselinux bindings which cannot be
# be installed with pip. Also this lowers virtualenvs footprints and install
# time
sitepackages=true
Copy link
Member

Choose a reason for hiding this comment

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

wrap it with whitespaces to match the common style

Suggested change
sitepackages=true
sitepackages = true

tox.ini Outdated
@@ -41,6 +41,10 @@ commands =
lint: yamllint -s test/ molecule/
whitelist_externals =
find
# sitepackages=true <- template module <- libselinux bindings which cannot be
Copy link
Member

Choose a reason for hiding this comment

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

We need to have way more comprehensive explanation then

@webknjaz
Copy link
Member

cc #1724 (comment)

@gundalow
Copy link
Contributor

2019-03-13 IRC Meeting. Agreed this isn't 2.20 blocking

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Please add a detailed explanation as a comment as per https://github.com/ansible/molecule/pull/1823/files#r264877505

@fabianvf
Copy link
Contributor

What information do you feel is missing? The current comment seems quite clear to me.

tox.ini Outdated Show resolved Hide resolved
@webknjaz
Copy link
Member

@fabianvf things tend to seem "obvious" once you already know a lot of details about them. I just want it to become more obvious to a person who has no context, sees it for the first time and tries to acquire such context. "Look at these 100+ comments" is not very helpful here. I think, this kind of comments should follow the same rules/considerations as good commit messages and docstings.

Avoid akward error during templating of Dockerfile which happens
on selinux enabled machines when ansible template module fails
due to missing selinux bindings.

By using sitepackages=true in tox.ini we likely inherit the libselinux
bindings from the system when we create the virtualenvs.

Workaround for ansible/ansible#34340 which
happens even if selinux is set to permissive.

Documents selinux issues in install documentation in order to inform
users about what they could encounter.

Fixes: #1724
Signed-off-by: Sorin Sbarnea <[email protected]>
@fabianvf
Copy link
Contributor

I'm not sure how many people would be building context from the tox.ini. I think it would be much more helpful to add a section to the docs explaining why Molecule doesn't work well in a normal virtualenv on Fedora-based distributions. If we wanted we could then link to it from here as well.

@ssbarnea ssbarnea changed the title Avoid python libselinux bindings being hidded by tox Avoid python libselinux bindings being hidden by tox Mar 14, 2019
@webknjaz webknjaz merged commit 12f925e into master Mar 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants