🌱 Replace github.com/pkg/errors #1585
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
k8s-ci-robot
added
cncf-cla: yes
|
Hi @pierreprinetti. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
pierreprinetti
force-pushed
the
replace_errors
branch
from
4da89f2 to
cdf8105
Compare
|
Incidentally, with this change the CI is now checking the style of the errors... which is why I had to remove the capital letter from an error message :) |
|
I don't know if we want to move away from |
The package [pkg/errors](/~https://github.com/pkg/errors) has served its purpose and its proposed changes have been incorporated into Go's standard library. This change removes it as a direct dependency, replacing its functionality with the corresponding features of `fmt` and `errors`.
| } | ||
| if len(networkList) == 0 { | ||
| handleUpdateOSCError(openStackCluster, errors.Errorf("failed to find any network: %v", err)) |
There was a problem hiding this comment.
wait. err is nil here, isn't it? Why did we wrap it, here and below?
There was a problem hiding this comment.
I have changed this error (and its message) in a separate commit. PTAL!
pierreprinetti
force-pushed
the
replace_errors
branch
from
cdf8105 to
407baac
Compare
Avoid wrapping `<nil>` in the error message if there is no error to wrap.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve Agree it's small change and we can follow other k/k behavior.. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jichenjc, pierreprinetti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
k8s-ci-robot
removed
the
do-not-merge/hold
What this PR does / why we need it:
The package pkg/errors has served its purpose and its proposed changes have been incorporated into Go's standard library in Go v1.13.
This change removes it as a direct dependency, replacing its functionality with the corresponding features of
fmtanderrors.Special notes for your reviewer:
The old
pkg/errorsdefined the methodCause() error, whereas the stdlib incorporated its functionality with the methodUnwrap() error. As such, if a consumer of CAPO directly callederrors.Cause(err)to match the causing error, then their code would silently break because the underlying sentinel error wouldn't be identified.This problem is susceptible to affect any code that uses CAPO as a library, that uses a version of
pkg/errorsbelowv0.9.0.The last versions of
pkg/errors(v0.9.0+) incorporated a compatibility layer with the newUnwrap()syntax.TODOs:
/hold
EDIT: Added a note pointing out that there is a risk of compatibility issues