Add errors to pkg/podman & wrap errors across codebase#482
Closed
martymichal wants to merge 2 commits intocontainers:mainfrom
Closed
Add errors to pkg/podman & wrap errors across codebase#482martymichal wants to merge 2 commits intocontainers:mainfrom
martymichal wants to merge 2 commits intocontainers:mainfrom
Conversation
|
Build failed.
|
martymichal
added
3. Enhancement
Member
Author
Using Golang errors with dynamic strings is not very good because it's very hard to check them. It is much easier to declare package-wide errors based on which printed error messages are generated. This makes out job to check for expected/unexpected errors much easier (even though there's a bit more code for checking and acting upon the different errors).
When testing for regressions introduced by Podman V2 I encountered an error with getting information about current user. I had to patch the toolbox binary to see the underlying error because we do not wrap errors in most places. Such problems could show again in different places.
martymichal
force-pushed
the
update-error-handling
branch
from
2f20722 to
ce6d113
Compare
|
Build failed.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In a lot of places in the code, we use Go errors in this fashion:
fmt.Errorf("container %s...", container). This is alright if we use the error messages immediately and don't care if a function may return different errors. At the moment we need to check for different errors coming from functions, this style becomes bothersome.This is the most noticeable in the case of removing containers/images. Commands
podman rm/rmimay fail with different errors. Either a container/image does not exist or it is running/used. Without dedicated error variables, this is rather hard to catch. We've been going about it by using the approach briefly shown above.The solution is to add error variables to the package (
podman) that can be checked for using theerrors(errors.Isanderrors.As) library of Go.The second part of this PR is that in a lot of places in the code, we return errors but don't wrap them around the errors that hold the information about the cause. I encountered this recently when Podman V2 introduced a regression about the option
--userns keep-idnot working properly. It caused Toolbox to crash (specifically the container entry-pointinit-container) due to being unable to find info about current user but there was no helpful error message because we dropped it when returning Toolbox's error message.A good side-effect of this is that commands
toolbox rm/rminow detect if a container is running/image has dependent containers and print a helpful message with a hint on how to delete them.