[content-service] show error if failed to download backup file #10491
Conversation
| } | ||
| if err != nil { | ||
| return src, xerrors.Errorf("cannot restore backup: %w", err) |
There was a problem hiding this comment.
If we use xerrors, we get a warning that we have to use fmt instead of it.
There was a problem hiding this comment.
@utam0k I made a similar change and got this comment #10339 (comment)
https://github.com/gitpod-io/go-style-guide/blob/master/style.md#errors
There was a problem hiding this comment.
@aledbf Thanks for your information. I haven't seen it. You are right.
There was a problem hiding this comment.
Hm. I was under the impression that we will be slowly switching to fmt instead of xerrors.
@easyCZ I think it was in one of your PRs that this was discussed, unless I am mistaken.
There was a problem hiding this comment.
Hm. I was under the impression that we will be slowly switching to fmt instead of errors.
Same here. Maybe the guide is not clear about that?
There was a problem hiding this comment.
Same here. Maybe the guide is not clear about that?
The guide is clear IMHO: it promotes xerrors (on purpose).
Thanks @kylos101 for linking the Slack thread. We didn't really land on a change, because
fmt.Errorfneither carries, nor perpetuates a stack tracegithub.com/pkg/errorswould produce a stack trace, but is deprecated and yet another third party library
xerrors is not ideal, but
- it works
- it is reasonably consistent throughout our codebase. Until we decide to do something else we should keep it this way to ease migration "search + replace" style.
- it's not deprecated
- it comes from the Go team themselves.
All that said, xerrors also isn't exactly future proof, and we could look for alternatives in an RFC.
roboquat
added
deployed: workspace
Description
Make sure to show if there was an error attempting to download backup, if any.
Related Issue(s)
Related #10315
How to test
Release Notes
Documentation