-
Notifications
You must be signed in to change notification settings - Fork 222
chore: kbagent log stdout on error #9454
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
base: main
Are you sure you want to change the base?
Conversation
Auto Cherry-pick Instructions
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9454 +/- ##
==========================================
+ Coverage 59.63% 59.93% +0.30%
==========================================
Files 500 517 +17
Lines 54384 55678 +1294
==========================================
+ Hits 32431 33370 +939
- Misses 19033 19336 +303
- Partials 2920 2972 +52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8d3c46e
to
38c9185
Compare
return nil, (*result).err | ||
} | ||
return (*result).stdout.Bytes(), nil | ||
return (*result).stdout.Bytes(), wrapExecError((*result).err, (*result).stderr) |
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.
It should not operate on stdout when a failure occurs. This is the behavior defined by the API.
return nil, errors.Wrapf(proto.ErrFailed, errMsg) | ||
} | ||
return nil, err | ||
return result.stdout.Bytes(), wrapExecError(result.err, result.stderr) |
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.
Ditto.
Log kbagent command's stdout on error so that users would have more context to debug.
One more thing. Action command is executed by os/exec. If the command A has created a subprocess B, and A exits before B exits, the command's timeout, which is included in the context, is not respected. (the exact same problem described in golang/go#23019) That is because B shares stdout/stderr fd with A, and go will wait for the fd to close before
cmd.Wait()
returns. This PR addscmd.WaitDelay
to handle this.The timeout parameter seems like not used currently, but it makes the test slow.