-
Notifications
You must be signed in to change notification settings - Fork 96
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
Fixes #111: Build on #113 also address Unable to find file /etc/selinux/config
error
#122
Conversation
Signed-off-by: Maksim Bazhin <[email protected]>
Signed-off-by: Maksim Bazhin <[email protected]>
Signed-off-by: Florian Heiderich <[email protected]>
Signed-off-by: Florian Heiderich <[email protected]>
Hey @heiderich thanks for the contribution, so once this PR is merged, we dont need #113? |
@prudhvigodithi Yes, exactly. The two commits from #113 are included in this PR. |
@@ -8,6 +8,8 @@ | |||
selinux: | |||
state: disabled | |||
when: (ansible_distribution != "Ubuntu") and (ansible_distribution != "Amazon") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heiderich Initially I added when
condition to do this task only for OS distribution other than ubuntu and amazon linux where I faced different errors due to unavailable of this module in those distributions.
If this condition covers errors from Ubuntu
and amazon
distributions then we can remove when
completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@heiderich could you please check the above comments?
Wow! Almost 8 months now and no one had time to review this? |
@ng-bsy I think there is an open comment by @saravanan30erd which is still open for reply from @heiderich and hence the PR is still open. |
@TheAlgo thanks for the quick response. It gives hope, seeing, this project isn't dead :-) Though I'm asking myself, if it's worth, holding out ~8 months on a bugfix, over the discussion if another piece of code should be removed, that may be having the same or partly the same effect, without really compromising performance. |
Thanks @TheAlgo and thanks @ng-bsy for your patience, its true we are holding based on pending conversation, @saravanan30erd are we ok to move forward with this PR based on above comment? |
@prudhvigodithi @peterzhuamazon The concern with this PR is, the way its managed to ignore the errors are limited and not scalable. For example, overall issues are occurring for debian based on this source issue #111 but for different versions of python the errors are also different for same debian distribution. As you can see, here #111 (comment) its different error output and here its another one #111 (comment) thats the reason here c4b2156#diff-26da4e2fc3ae1d719ad41b3e625de34dd3fbc83814e8a4c6875c55f4fb6f0e91R12 new error output is added. In this case, we might get new error output for new versions and we need to keep on adding those new changes in the |
Thank you for the contribution @heiderich. I think the consensus is not to merge this PR because of the reasons above, @prudhvigodithi should we close it? |
Close this as it has been handled in #152. Thanks. |
Description
This is build on top of #113 of @FactorT.
Issues Resolved
#111
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Note that this commit also contains two commits (c5fe1e2, cddafd6) of @FactorT from #113.