-
Notifications
You must be signed in to change notification settings - Fork 250
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
Verify if binary exists before executing #4635
base: main
Are you sure you want to change the base?
Conversation
Hi @hector-vido. Thanks for your PR. I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
b8f9bb4
to
abf149a
Compare
/ok-to-test |
@@ -49,6 +49,10 @@ func RunPrivileged(reason string, cmdAndArgs ...string) (string, string, error) | |||
return "", "", errors.New("sudo executable not found") | |||
} | |||
logging.Infof("Using root access: %s", reason) | |||
_, err = exec.LookPath(cmdAndArgs[0]) | |||
if err != nil { | |||
return "", "", errors.New(cmdAndArgs[0] + " executable not found") |
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.
👍🏼
@hector-vido : Thanks for your PR! Is it possible to add a unit test to verify that your changes work as expected? |
abf149a
to
28a63b6
Compare
28a63b6
to
3983c64
Compare
This is interesting, pull-ci-crc-org-crc-main-images and some other tests are failing because
Fixed with an unexported package-level variable |
3983c64
to
5654a32
Compare
/retest |
2 similar comments
/retest |
/retest |
5654a32
to
58fa905
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: praveenkumar, rohanKanojia The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@hector-vido: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Description
Verify if the binary exists before running it with sudo.
Actually the exit code is not useful, only show that a result different than 0 was returned.
With this tiny modification, is easy for the user to understand what is missing:
I was testing
crc
inside an openSUSE Tumbleweed version 20250219.Type of change
test, version modification, documentation, etc.)
Proposed changes
Just an extra verification for the existence of binaries used and a small unit test.
Unfortunately with this unit test the
sudo
command will be invoked in each test call and most of the container images doesn't have it. The solution for this was to use an unexported package-level variable, this variable can be overwritten inside the test, avoiding the use of an interface.Testing
Added a unit test that should fail trying to find a binary named
i-dont-exist
.Thanks to @rohanKanojia who gives me a north on that.
Contribution Checklist