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.

Sign up for GitHub

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

Jump to bottom

pkg,schema,internal: eliminate multierror. #196

Merged
merged 2 commits into from
Mar 5, 2024
Merged

pkg,schema,internal: eliminate multierror. #196

merged 2 commits into from
Mar 5, 2024

Conversation

klihub
Copy link
Contributor

@klihub klihub commented Mar 1, 2024

Migrate from our internal multierror implementation to using error slices and stock errors.Join().

@klihub klihub requested a review from elezar March 1, 2024 16:21
elezar
elezar reviewed Mar 1, 2024
View reviewed changes
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.

I think we need to bump our go version to at least 1.19.

@klihub klihub force-pushed the eliminate-multierror branch from d6bfc31 to 72d9aa7 Compare March 1, 2024 16:56
@klihub
Copy link
Contributor Author

klihub commented Mar 1, 2024

I think we need to bump our go version to at least 1.19.

Yeah, I figured too.

@elezar
Copy link
Contributor

elezar commented Mar 1, 2024

Are our clients using go >= 1.21 to build their applications?

For completeness, I recently did something similar in one of our projects to enable errors.Join in go1.17: NVIDIA/nvidia-container-toolkit#337

elezar
elezar reviewed Mar 1, 2024
View reviewed changes
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.

The use of errors.Join can clean this up even further since it's never required to store a slice of errors.

internal/validation/k8s/objectmeta.go Show resolved Hide resolved
pkg/cdi/cache.go Outdated Show resolved Hide resolved
pkg/cdi/cache.go Outdated Show resolved Hide resolved
pkg/cdi/cache.go Outdated Show resolved Hide resolved
pkg/cdi/cache.go Show resolved Hide resolved
schema/schema.go Show resolved Hide resolved
schema/schema.go Show resolved Hide resolved
internal/validation/k8s/objectmeta.go Show resolved Hide resolved
internal/validation/k8s/objectmeta.go Show resolved Hide resolved
internal/validation/k8s/objectmeta.go Show resolved Hide resolved
@klihub klihub force-pushed the eliminate-multierror branch 2 times, most recently from 1d38004 to 7a20465 Compare March 3, 2024 15:47
@klihub
Copy link
Contributor Author

klihub commented Mar 3, 2024 •
edited
Loading

The use of errors.Join can clean this up even further since it's never required to store a slice of errors.

If you don't care about how the topology of the final set of stored errors look like, but (for some reason) I prefer to keep errors 'at the same level' in the same slice (of joinErrors) and only 'cascading result of errors' (errors caused by other errors) nested the way it would be if we used the code you suggested. See my other related comment...

@klihub
pkg,schema,internal: eliminate multierror.
5e94eef 
Migrate from our internal multierror implementation to using
error slices and stock errors.Join().

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the eliminate-multierror branch from 7a20465 to 62477a6 Compare March 3, 2024 16:03
@elezar
Copy link
Contributor

elezar commented Mar 5, 2024

The use of errors.Join can clean this up even further since it's never required to store a slice of errors.

If you don't care about how the topology of the final set of stored errors look like, but (for some reason) I prefer to keep errors 'at the same level' in the same slice (of joinErrors) and only 'cascading result of errors' (errors caused by other errors) nested the way it would be if we used the code you suggested. See my other related comment...

OK. That's a fair point. I was not considering this aspect.

elezar
elezar reviewed Mar 5, 2024
View reviewed changes
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.

I think I'm good with the changes now. Thanks for motivating your choices so clearly.

My final concern is the go version bump. Do we foresee issues with clients consuming this module?

cmd/cdi/go.mod Outdated Show resolved Hide resolved
@klihub klihub force-pushed the eliminate-multierror branch from 62477a6 to 788ec0e Compare March 5, 2024 11:19
@klihub
go.mod,cmd,.github: bump go version to 1.20.
7ce7168 
Bump go version to 1.20. That'll get us stock errors.Join()
support and remove reliance on an EOL'd 1.19 go version.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the eliminate-multierror branch from 788ec0e to 7ce7168 Compare March 5, 2024 11:23
@klihub klihub requested a review from elezar March 5, 2024 11:25
elezar
elezar approved these changes Mar 5, 2024
View reviewed changes
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 @klihub. I think I'm fine with the 1.20 bump here.

Once Podman bumps their version we could look at doing that here too.

@elezar elezar merged commit ac91358 into cncf-tags:main Mar 5, 2024
7 checks passed
@klihub klihub deleted the eliminate-multierror branch March 5, 2024 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elezar elezar elezar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@klihub @elezar