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

remove usage of github.com/hashicorp/go-multierror #89

Merged
merged 1 commit into from
Nov 3, 2022

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Nov 1, 2022

The internal replacement is a simpler implementation which only supports enough functionality for the places where github.com/hashicorp/go-multierror was used before.

Not depending on github.com/hashicorp/go-multierror and thus github.com/hashicorp/errwrap makes it easier to use CDI in Kubernetes because we don't need to add those other dependencies.

Fixes: #88

@pohly pohly force-pushed the error-dependencies branch 3 times, most recently from 81a5a0f to b9a62f2 Compare November 2, 2022 07:46
@kad
Copy link
Contributor

kad commented Nov 2, 2022

@pohly the dependency seems to be still present as indirect, is that expected?

Second question, is multierror.test binary really needed in the tree?

The internal replacement is a simpler implementation which only supports enough
functionality for the places where github.com/hashicorp/go-multierror was used
before.

Not depending on github.com/hashicorp/go-multierror and thus
github.com/hashicorp/errwrap makes it easier to use CDI in Kubernetes because
we don't need to add those other dependencies.

Signed-off-by: Patrick Ohly <[email protected]>
@pohly
Copy link
Contributor Author

pohly commented Nov 2, 2022

@pohly the dependency seems to be still present as indirect, is that expected?

Yes:

# github.com/hashicorp/go-multierror
github.com/container-orchestrated-devices/container-device-interface/cmd/cdi/cmd
github.com/opencontainers/runtime-tools/generate
github.com/opencontainers/runtime-tools/validate
github.com/hashicorp/go-multierror

github.com/opencontainers/runtime-tools would have to be modified to get rid of it completely. May be useful, but doesn't affect us that much because it doesn't get pulled into Kubernetes (I checked).

Second question, is multierror.test binary really needed in the tree?

Oops, no. Looks like I was relying on Go projects filtering out "*.test" during git add and this project doesn't do that. Fixed.

@pohly pohly force-pushed the error-dependencies branch from b9a62f2 to ec52f5a Compare November 2, 2022 11:05
@kad
Copy link
Contributor

kad commented Nov 2, 2022

ok. if that is not propagating to k8s as import, then it is lgtm from me.

@elezar elezar self-requested a review November 3, 2022 16:31
Copy link
Contributor

@elezar elezar left a comment

Choose a reason for hiding this comment

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

Thanks @pohly

@elezar elezar merged commit d4004b3 into cncf-tags:main Nov 3, 2022
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.

dependencies: remove github.com/hashicorp/go-multierror and github.com/hashicorp/errwrap
3 participants