-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Makefile: Use 'command -v selinuxenabled' instead of hard-coded path #1363
Makefile: Use 'command -v selinuxenabled' instead of hard-coded path #1363
Conversation
The hard-coded path landed in 488216f (Make sure selinuxenabled exists before executing it, 2016-10-17, cri-o#154), but there's no need to require that path. Using 'command -v' (in POSIX [1]) supports anyone who has selinuxenabled in their PATH. [1]: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/command.html Signed-off-by: W. Trevor King <wking@tremily.us>
Hi @wking. Thanks for your PR. I'm waiting for a openshift or kubernetes-incubator member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
LGTM
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.
LGTM
I think this can be merged without running E2E test. |
This patch fixes selinuxopt generation as found in: ``` install /usr/sbin/selinuxenabled -D -m 644 crio.conf /etc/crio/crio.conf ``` The above is clearly wrong when installing the configuration because `commmand -v` outputs the path of selinuxenabled as well, resulting in /usr/bin/selinuxenabled -Z This patch fixes that by just echoing the -Z as needed. Issue introduced in cri-o#1363 Signed-off-by: Antonio Murdaca <runcom@redhat.com>
Nack, this has broken docker hub CD |
The hard-coded path landed in 488216f (#154), but there's no need to require that path. Using
command -v
(in POSIX) supports anyone who hasselinuxenabled
in theirPATH
.