Skip to content
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 an errors package with a similar API to github.com/pkg/errors #291

Merged
merged 3 commits into from
Sep 13, 2021

Conversation

negz
Copy link
Member

@negz negz commented Sep 9, 2021

Description of your changes

Go introduced a 'native' way to wrap errors back in v1.13. At that point we were already using github.com/pkg/errors to 'wrap' errors with context, and we never got around to migrating. In addition to pure inertia, I've personally avoided making the switch because I prefer the github.com/pkg/errors API. Specifically I like that errors.Wrap handles the "outer context: inner context" error format that Go uses by convention, and that errors.Wrap will return nil when passed a nil error.

Given that github.com/pkg/errors has long been in maintenance mode, and is (per pkg/errors#245) no longer used by its original author now seems as good a time as any to migrate. This commit attempts to ease that migration for the Crossplane project - and to retain the nice API - by adding a package that acts as a small github.com/pkg/errors style shim layer around the stdlib pkg/errors (and friends, like fmt.Errorf).

It's worth noting that this PR would result in a behaviour change; we'd no longer include stack traces when errors were logged. In practice we only log errors at debug level most (all?) of the time, and I'm fairly confident no one will miss this behaviour. Otherwise, I'm fairly confident this subset of the github.com/pkg/errors API will serve as a drop-in replacement for all the parts we use.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

I've confirmed that this package is a drop-in replacement for all our uses within crossplane/crossplane-runtime and crossplane/crossplane.

@negz negz changed the title Add an errors package with a similar API to github.com/pkg/errorsError Add an errors package with a similar API to github.com/pkg/errors Sep 9, 2021
Go introduced a 'native' way to wrap errors back in v1.13. At that point we were
already using github.com/pkg/errors to 'wrap' errors with context, and we never
got around to migrating. In addition to pure inertia, I've personally avoided
making the switch because I prefer the github.com/pkg/errors API. Specifically I
like that errors.Wrap handles the "outer context: inner context" error format
that Go uses by convention, and that errors.Wrap will return nil when passed a
nil error.

Given that github.com/pkg/errors has long been in maintenance mode, and is (per
pkg/errors#245) no longer used by its original author
now seems as good a time as any to migrate. This commit attempts to ease that
migration for the Crossplane project - and to retain the nice API - by adding a
package that acts as a small github.com/pkg/errors style shim layer around the
stdlib pkg/errors (and friends, like fmt.Errorf).

Signed-off-by: Nic Cope <[email protected]>
I tried to address this TODO today, but found that I kind of prefer how our
implementation works. I've found from time to time while writing tests that
I was accidentally wrapping my errors with the wrong context (i.e. message),
which is not something the cmpopts variant tests for.

Signed-off-by: Nic Cope <[email protected]>
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Do you expect github.com/pkg/errors going into maintenance expect us in any way? I think our usage of that library has not changed since the inception of Crossplane so I feel like if we keep using that nothing would really change for another long period, plus we'd not have to migrate all and enforce our own errors package everywhere. So, I'm not sure if we should have this unless there is a feature that we'd like to have that doesn't exist in github.com/pkg/errors.

pkg/errors/errors.go Show resolved Hide resolved
return fmt.Errorf("%s: %w", fmt.Sprintf(format, args...), err)
}

// Wrap is an alias for WithMessage.
Copy link
Member

Choose a reason for hiding this comment

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

We could possibly have an error type that has the initial error in one property and then the wrap strings in a slice. When printed, it'd look just like today but we could use that initial error property when we'd like to know the actual type of the error. Right now, once you wrap, the actual error becomes just a string and usual error checking functions don't work as easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the idiomatic stdlib way to do this these days is to use errors.As to determine whether an error wraps a matching error (i.e. somewhere in the stack of errors there's an error that == the one you're checking against).

@negz
Copy link
Member Author

negz commented Sep 10, 2021

Do you expect github.com/pkg/errors going into maintenance expect us in any way? I think our usage of that library has not changed since the inception of Crossplane so I feel like if we keep using that nothing would really change for another long period.

I doubt we'll be affected anytime soon, but I'd still prefer to move us closer to the stdlib error tooling. I expect doing so will increase compatibility with other libraries, and perhaps (as github.com/pkg/errors becomes less prominent over time) make our code a little easier to follow, since folks will be able to see that this package is really just a tiny opinionated wrapper that changes errors.Wrap(err, "foo") into the fmt.Errorf("foo: %w") pattern they'll likely be most accustomed to.

plus we'd not have to migrate all and enforce our own errors package everywhere.

A goal of this package is to make migration really straightforward. I expect it to be nothing more than changing the import path (as demonstrated in this PR, and tested against crossplane/crossplane).

So, I'm not sure if we should have this unless there is a feature that we'd like to have that doesn't exist in github.com/pkg/errors.

There's no new features in this library - in fact it has fewer features than github.com/pkg/errors (on purpose). Given how tiny it is I'm not concerned about it being a maintenance burden.

Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

LGTM!

@negz negz merged commit 6a7a44a into crossplane:master Sep 13, 2021
@negz negz deleted the error branch September 13, 2021 01:54
negz added a commit to negz/crossplane that referenced this pull request Sep 13, 2021
negz added a commit to negz/provider-gcp that referenced this pull request Sep 15, 2021
negz added a commit to negz/provider-gcp that referenced this pull request Sep 15, 2021
turkenh pushed a commit to turkenh/provider-gcp that referenced this pull request Sep 20, 2021
See crossplane/crossplane-runtime#291 for context.

Signed-off-by: Nic Cope <[email protected]>
(cherry picked from commit 0cd8367)
luebken pushed a commit to luebken/crossplane that referenced this pull request Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants