pkg/shell: Preserve error types in shell command execution#1780
pkg/shell: Preserve error types in shell command execution#1780DaliborKr wants to merge 1 commit intocontainers:mainfrom
Conversation
The existing RunContextWithExitCode() wraps all errors from exec.Command in generic "failed to invoke" messages, which prevents callers from distinguishing between actual error types. Add RunContextWithExitCode2() and RunWithExitCode2() that return errors with their original types intact, including for ExitError. This allows callers to use errors.Is() and errors.As() to handle specific failure modes. For example, detecting a missing skopeo binary (exec.ErrNotFound) or an ENOEXEC error from inside non native containers, when an emulator is not set correctly. These new functions are meant to replace its original versions in the future. containers#1780 Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
DaliborKr
force-pushed
the
pr01-shell-error-types
branch
from
588461c to
d28cb8c
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces RunContextWithExitCode2 and RunWithExitCode2 to the shell package. The feedback suggests refactoring the code to reduce duplication between the new and existing functions, as well as simplifying the error handling logic to be more idiomatic by removing redundant variables.
| func RunContextWithExitCode2(ctx context.Context, | ||
| name string, | ||
| stdin io.Reader, | ||
| stdout, stderr io.Writer, | ||
| arg ...string) (int, error) { | ||
|
|
||
| logLevel := logrus.GetLevel() | ||
| if stderr == nil && logLevel >= logrus.DebugLevel { | ||
| stderr = os.Stderr | ||
| } | ||
|
|
||
| cmd := exec.CommandContext(ctx, name, arg...) | ||
| cmd.Stdin = stdin | ||
| cmd.Stdout = stdout | ||
| cmd.Stderr = stderr | ||
|
|
||
| if err := cmd.Run(); err != nil { | ||
| exitCode := 1 | ||
|
|
||
| if ctxErr := ctx.Err(); ctxErr != nil { | ||
| return 1, ctxErr | ||
| } | ||
|
|
||
| var exitErr *exec.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| exitCode = exitErr.ExitCode() | ||
| return exitCode, err | ||
| } | ||
|
|
||
| return exitCode, err | ||
| } | ||
|
|
||
| return 0, nil | ||
| } |
There was a problem hiding this comment.
The implementation of RunContextWithExitCode2 duplicates much of the logic found in RunContextWithExitCode, specifically the command initialization and logging setup. Consider refactoring these common steps into a private helper function to improve maintainability and reduce code duplication.
| exitCode := 1 | ||
|
|
||
| if ctxErr := ctx.Err(); ctxErr != nil { | ||
| return 1, ctxErr | ||
| } | ||
|
|
||
| var exitErr *exec.ExitError | ||
| if errors.As(err, &exitErr) { | ||
| exitCode = exitErr.ExitCode() | ||
| return exitCode, err | ||
| } | ||
|
|
||
| return exitCode, err |
There was a problem hiding this comment.
The error handling logic can be simplified by removing the redundant exitCode variable and returning values directly. This makes the function more idiomatic and easier to read.
if ctxErr := ctx.Err(); ctxErr != nil {
return 1, ctxErr
}
var exitErr *exec.ExitError
if errors.As(err, &exitErr) {
return exitErr.ExitCode(), err
}
return 1, err
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 13s |
1 participant
This is PR 1/10 in a series adding cross-architecture container support using QEMU and binfmt_misc.
Summary
RunContextWithExitCode2()andRunWithExitCode2()to the shell packageexec.ErrNotFound,exec.ExitError,syscall.ENOEXEC, etc.) instead of wrapping them in generic messageserrors.Is()anderrors.As()to handle specific failure modesThese new functions are meant to replace its original versions in the future.