-
Notifications
You must be signed in to change notification settings - Fork 105
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
Conversation
errors
package with a similar API to github.com/pkg/errors
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]>
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]>
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.
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
.
return fmt.Errorf("%s: %w", fmt.Sprintf(format, args...), err) | ||
} | ||
|
||
// Wrap is an alias for WithMessage. |
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 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.
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.
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).
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
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).
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. |
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.
LGTM!
See crossplane/crossplane-runtime#291 for context. Signed-off-by: Nic Cope <[email protected]>
See crossplane/crossplane-runtime#291 for context. Signed-off-by: Nic Cope <[email protected]>
See crossplane/crossplane-runtime#291 for context. Signed-off-by: Nic Cope <[email protected]>
See crossplane/crossplane-runtime#291 for context. Signed-off-by: Nic Cope <[email protected]> (cherry picked from commit 0cd8367)
See crossplane/crossplane-runtime#291 for context. Signed-off-by: Nic Cope <[email protected]>
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 thegithub.com/pkg/errors
API. Specifically I like thaterrors.Wrap
handles the"outer context: inner context"
error format that Go uses by convention, and thaterrors.Wrap
will returnnil
when passed anil
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 smallgithub.com/pkg/errors
style shim layer around the stdlibpkg/errors
(and friends, likefmt.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:
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.