-
Notifications
You must be signed in to change notification settings - Fork 149
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
Common exec error handling #2501
Conversation
tty) | ||
|
||
if err != nil { |
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.
Is that necessary here?
It seems like this will change the behaviour for ExecWithOptions
so it always returns error, should it be left in pod_command_executor?
Am I missing something here?
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.
Old code was returning error (or nil) as is, new code returns nil as is, and wraps an error with ExecError (which contains stderr/stdout tails), what is actually was the purpose of this PR.
Because without that change, only PodCommandExecutor
users were able to find/parse an error in logs, and we want to give this possibility for users of kube.Exec
/kube.ExecWithOptions
/kube.ExecOutput
too.
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
LGTM, we can merge after Prasad looks into the reply of his comment. |
Change Overview
This PR contains fix for #2066
From now, regardless the way of invocation, execution error will contain tail of stdout/stderr.
Which then could be used by the invoker to construct more precise error.
Pull request type
Please check the type of change your PR introduces:
Issues
kube.Exec
andpodCommandExecutor.Exec
should create theExecOptions
similarly #2066Test Plan