Handle commit error gracefully #385
Conversation
cmd/buildah/commit.go
Outdated
| oErr := nErr[0] | ||
| for _, e := range nErr[1:] { | ||
| oErr = errors.Wrapf(e, oErr.Error()) | ||
| } |
There was a problem hiding this comment.
Iteratively wrapping each error with a separate call to errors.Wrapf() is going to produce duplication in the call stack. Perhaps converting the errcode.Errors into a github.com/urfave/cli/MultiError with its NewMultiError() function and then wrapping that result by calling errors.Wrapf() once would produce something more readable?
There was a problem hiding this comment.
I found out that errors.Wrapfing MultiError makes the error message a little bit confusing so I went with returning just MultiError instead.
There was a problem hiding this comment.
Ah, I was wondering about that. Makes sense.
|
@ripcurld0 You have to sign your PR. |
This change gives a better error message when the commit fails because of bad authentication. Signed-off-by: Boaz Shuster <[email protected]>
|
LGTM |
|
@nalind @TomSweeneyRedHat PTAL |
|
📌 Commit 8ca2bdb has been approved by |
|
⚡ Test exempted: merge already tested. |
|
This should not have been merged yet. |
|
Until containers/image#390 gets merged (and vendored here), this should just produce the same behavior we have now, so no harm done, right? |
|
@nalind correct. |
|
Thanks! |
|
@nalind no problem 👍 |
When trying to tag an alias (tag) of an image using only the shortname and no tag, we were unable to find the image in storage. This corrects that issue and adds an integration test to protect against regression. I also updated the man page per the filed issue. While writing the integration test, I discovered that inspect could also not find a tagged image without its :tag. Resolves Issue #385 Resolves Issue #384 Signed-off-by: baude <[email protected]> Closes: #398 Approved by: mheon
5 participants
This change gives a better error message when the commit fails because of bad authentication.
This fixes #254 and should be merged only after containers/image#390 is merged.