Skip to content

[content-service] show error if failed to download backup file #10491

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 8, 2022
Merged

[content-service] show error if failed to download backup file #10491

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion components/content-service/pkg/initializer/initializer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ func (bi *fromBackupInitializer) Run(ctx context.Context, mappings []archive.IDM

hasBackup, err := bi.RemoteStorage.Download(ctx, bi.Location, storage.DefaultBackup, mappings)
if !hasBackup {
return src, xerrors.Errorf("no backup found")
return src, xerrors.Errorf("no backup found: %w", err)
}
if err != nil {
return src, xerrors.Errorf("cannot restore backup: %w", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the xerrors change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use xerrors, we get a warning that we have to use fmt instead of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aledbf Thanks for your information. I haven't seen it. You are right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csweichel ^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A related Slack thread

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Errorf neither carries, nor perpetuates a stack trace
  • github.com/pkg/errors would 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated PR.

Expand Down