-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add context to registry push errors #2981
Conversation
exporter/containerimage/export.go
Outdated
@@ -301,7 +301,7 @@ func (e *imageExporterInstance) Export(ctx context.Context, src exporter.Source, | |||
if e.push { | |||
err := e.pushImage(ctx, src, sessionID, targetName, desc.Digest) | |||
if err != nil { | |||
return nil, err | |||
return nil, fmt.Errorf("failed to push %v: %w", targetName, err) |
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.
We don't allow fmt.Errorf
in build repos because they lose stacktraces. Use pkg/errors.Wrap
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.
Good to know, thanks!
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.
Does it lose stack traces even with %w
?
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.
Yes. Stdlib errors
does not capture a stacktrace.
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.
Ah, makes sense. I misunderstood your comment and thought you meant it would remove existing stack traces from the error chain.
@@ -42,6 +42,12 @@ func ToGRPC(err error) error { | |||
st = status.FromProto(pb) | |||
} | |||
|
|||
if err.Error() != st.Message() { |
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.
Could you add a comment in here that "If grpc error was wrapped with additional message then set full message to grpc error".
Otherwise it is quite tricky to understand why this code is in here.
Signed-off-by: Jonny Stoten <jonny.stoten@docker.com>
Signed-off-by: Jonny Stoten <jonny.stoten@docker.com>
Signed-off-by: Jonny Stoten <jonny.stoten@docker.com>
67e52c2
to
df0c9b6
Compare
Fixes #2019.
Registry push error from buildx before this change:
and after: