cmd/create, pkg/utils: Add spinner helpers and distro image matching#1781
cmd/create, pkg/utils: Add spinner helpers and distro image matching#1781DaliborKr wants to merge 3 commits into
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>
![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 refactors the spinner logic into reusable helper functions, introduces new shell execution utilities with enhanced logging, and adds a helper to identify supported distro images. A potential panic was identified in the spinner implementation where a nil pointer could be dereferenced if the stop helper is not used consistently; a refactor to ensure a valid spinner instance is always returned was suggested.
| func startSpinner(message string) *spinner.Spinner { | ||
| if logLevel := logrus.GetLevel(); logLevel < logrus.DebugLevel { | ||
| s := spinner.New(spinner.CharSets[9], 500*time.Millisecond, spinner.WithWriterFile(os.Stdout)) | ||
| s.Prefix = message | ||
| s.Start() | ||
| return s | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The current implementation of startSpinner returns nil when the log level is Debug or higher. However, there is a direct call to s.Stop() at line 497 (visible in the full file but outside this diff hunk) that does not check for nil. This will cause a panic when running with verbose logging (e.g., toolbox create --verbose).
To fix this, you should either use the stopSpinner(s) helper at line 497 or refactor startSpinner to always return a spinner instance that is only started conditionally, as suggested below.
func startSpinner(message string) *spinner.Spinner {
s := spinner.New(spinner.CharSets[9], 500*time.Millisecond, spinner.WithWriterFile(os.Stdout))
if logLevel := logrus.GetLevel(); logLevel < logrus.DebugLevel {
s.Prefix = message
s.Start()
}
return s
}In /src/cmd/create.go, the same pattern of spinner creation and nil-safe stopping is repeated. Extract this into startSpinner() and stopSpinner() helper functions so that future callers can use spinners without duplicating the code. Replace the existing inline spinner code in createContainer() and pullImage() with calls to these new helpers. containers#1781 Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
…atching Add IsSupportedDistroImage(), which iterates over all supported distros and checks if the image basename matches any of them. This will be used by the architecture resolution code to decide whether to parse architecture suffixes from image tags, as this should be done only for natively supported images [1]. [1] Toolbx supported distributions: https://containertoolbx.org/distros/ containers#1781 Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
DaliborKr
force-pushed
the
pr02-spinner-utils
branch
from
051b8d7 to
4afa672
Compare
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 17s |
In /src/cmd/create.go, the same pattern of spinner creation and nil-safe stopping is repeated. Extract this into startSpinner() and stopSpinner() helper functions so that future callers can use spinners without duplicating the code. Replace the existing inline spinner code in createContainer() and pullImage() with calls to these new helpers. containers#1781 Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
…atching Add IsSupportedDistroImage(), which iterates over all supported distros and checks if the image basename matches any of them. This will be used by the architecture resolution code to decide whether to parse architecture suffixes from image tags, as this should be done only for natively supported images [1]. [1] Toolbx supported distributions: https://containertoolbx.org/distros/ containers#1781 Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
DaliborKr
force-pushed
the
pr02-spinner-utils
branch
from
4afa672 to
80c972b
Compare
|
Build succeeded. ✔️ unit-test SUCCESS in 3m 08s |
1 participant
This is PR 2/10 in a series adding cross-architecture container support using QEMU and binfmt_misc.
Depends on: #1780 (pkg/shell: Preserve error types in shell command execution)
Please review #1780 first. The new commits in this PR are:
Summary
startSpinner()andstopSpinner()helper functions covering the inline spinner code increateContainer()andpullImage(), reducing repetition and making future spinner usage simplerIsSupportedDistroImage()to the utils package for checking whether an image reference matches any natively supported distribution's base image name (see Toolbx supported distros)