🌱 Replace hashicorp/go-multierror with kerrors #9175
Conversation
k8s-ci-robot
added
area/dependency
k8s-ci-robot
added
the
size/S
| if result != nil { | ||
| klog.Exit(result.Error()) | ||
| if errs != nil { | ||
| klog.Exit(kerrors.NewAggregate(errs).Error()) |
There was a problem hiding this comment.
There is a downgrade in UX here from:
F0814 11:16:57.464635 80425 main.go:214] 2 errors occurred:
* type "cluster.x-k8s.io/MachineDeployment" in package "sigs.k8s.io/cluster-api/api/v1beta1" marked as storage version but it's not convertible, missing Hub() method
* type "cluster.x-k8s.io/MachineSetList" in package "sigs.k8s.io/cluster-api/api/v1beta1" marked as storage version but it's not convertible, missing Hub() method
to:
[type "cluster.x-k8s.io/MachineDeploymentList" in package "sigs.k8s.io/cluster-api/api/v1beta1" marked as storage version but it's not convertible, missing Hub() method, type "cluster.x-k8s.io/MachineHealthCheck" in package "sigs.k8s.io/cluster-api/api/v1beta1" marked as storage version but it's not convertible, missing Hub() method, type "cluster.x-k8s.io/MachineDeployment" in package "sigs.k8s.io/cluster-api/api/v1beta1" marked as storage version but it's not convertible, missing Hub() method]
make: *** [Makefile:628: verify-conversions] Error 1
But I'm fine with that as this tool is pretty low impact and isn't really user facing. We could implement our own string function to compensate thoug if needed.
There was a problem hiding this comment.
Can we add a simple for loop over errors with fmt.Println + os.Exit at the end?
There was a problem hiding this comment.
Also len(errs) > 0 seems more idiomatic vs != nil
There was a problem hiding this comment.
Sure - the current version now looks very similar to the old output using go-multi-error
| @@ -80,7 +79,6 @@ require ( | |||
| github.com/google/uuid v1.3.0 // indirect | |||
| github.com/googleapis/enterprise-certificate-proxy v0.2.5 // indirect | |||
| github.com/googleapis/gax-go/v2 v2.12.0 // indirect | |||
| github.com/hashicorp/errwrap v1.1.0 // indirect | |||
| github.com/hashicorp/hcl v1.0.0 // indirect | |||
There was a problem hiding this comment.
This indirect dep is brought in by viper
> go mod why -m github.com/hashicorp/hcl
# github.com/hashicorp/hcl
sigs.k8s.io/cluster-api/cmd/clusterctl/client/config
github.com/spf13/viper
github.com/spf13/viper/internal/encoding/hcl
github.com/hashicorp/hcl
We have a number of other indirect hashicorp dependencies in our go.sum - but I think all of them are going to remain at MPL for now.
killianmuldoon
force-pushed
the
pr-replace-go-multierror
branch
2 times, most recently
from
4176cc2
to
93d11ed
Compare
Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
killianmuldoon
force-pushed
the
pr-replace-go-multierror
branch
from
93d11ed
to
cd595fb
Compare
|
Thank you very much! /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 8506d74605880914d166ffc72405711c4179a140
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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
Replace hashicorp/go-multierror with kerrors. This bring the conversion-verifier in line with what's being done across the rest of the codebase.
Related to cncf/foundation#617 which investigates license changes in hashicorp repos. This is the only direct dependency on a hashicorp repo in the CAPI codebase. AFAICT this repo won't be impacted by the licensing changes, but it's a low impact change which drops a dependency and makes the code more consistent.
/area dependency