ansible-playbook icon indicating copy to clipboard operation
ansible-playbook copied to clipboard

Fixes #111: Build on #113 also address `Unable to find file /etc/selinux/config` error

Open heiderich opened this issue 1 year ago • 7 comments

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 (c5fe1e2ea87b1642585752abea4aa1e3499ce879, cddafd6d00da23225d7f3e68a33e822647cb183c) of @FactorT from #113.

heiderich avatar Mar 18 '23 20:03 heiderich

Hey @heiderich thanks for the contribution, so once this PR is merged, we dont need https://github.com/opensearch-project/ansible-playbook/pull/113?

prudhvigodithi avatar Mar 19 '23 00:03 prudhvigodithi

@prudhvigodithi Yes, exactly. The two commits from #113 are included in this PR.

heiderich avatar Mar 19 '23 17:03 heiderich

Wow! Almost 8 months now and no one had time to review this? Is this project dead? Is it too much work, reviewing 4 lines of code? If it is, maybe someone else should take over this project? @TheAlgo @prudhvigodithi @gaiksaya @peterzhuamazon @bbarani

ng-bsy avatar Nov 16 '23 14:11 ng-bsy

@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 avatar Nov 16 '23 15:11 TheAlgo

@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. In my opinion, that discussion is another issue (code optimization) and this PR (bugfix) should be merged asap.

ng-bsy avatar Nov 16 '23 15:11 ng-bsy

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 avatar Nov 16 '23 16:11 prudhvigodithi

@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 https://github.com/opensearch-project/ansible-playbook/issues/111 but for different versions of python the errors are also different for same debian distribution. As you can see, here https://github.com/opensearch-project/ansible-playbook/issues/111#issue-1575639716 its different error output and here its another one https://github.com/opensearch-project/ansible-playbook/issues/111#issuecomment-1474984451 thats the reason here https://github.com/opensearch-project/ansible-playbook/pull/122/commits/c4b21560b69a57af97261468e36857e7906afe4c#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 search thats why I am pointing out its not scalable. I can see both issues were for debian, so we can ignore completely that distribution for this step so we don't have to cover different error outputs to ignore. Raised the pr for this https://github.com/opensearch-project/ansible-playbook/pull/152 please review it and provide your thoughts.

saravanan30erd avatar Feb 03 '24 05:02 saravanan30erd

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?

dblock avatar Jul 22 '24 16:07 dblock

Close this as it has been handled in #152.

Thanks.

peterzhuamazon avatar Jul 22 '24 18:07 peterzhuamazon