Skip to content

vendor: tags.cncf.io/container-device-interface v1.0.1 #5856

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

Merged
merged 1 commit into from
Mar 24, 2025

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Mar 19, 2025

@github-actions github-actions bot added the area/dependencies Pull requests that update a dependency file label Mar 19, 2025
go.mod Outdated
@@ -180,10 +180,11 @@ require (
go.opentelemetry.io/otel/metric v1.31.0 // indirect
golang.org/x/text v0.21.0 // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20241021214115-324edc3d5d38 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
Copy link
Member Author

Choose a reason for hiding this comment

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

+10k LOC mainly because of this dep 😰

Relates to cncf-tags/container-device-interface#236

@elezar Any reason why gopkg.in/yaml.v3 is not used instead?

Copy link

Choose a reason for hiding this comment

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

See containerd/containerd#11522 (comment)

We also have also created cncf-tags/container-device-interface#262 to move to gopkg.in/yaml.v3 instead. Would that be more reasonable?

Copy link
Member Author

@crazy-max crazy-max Mar 19, 2025

Choose a reason for hiding this comment

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

We also have also created cncf-tags/container-device-interface#262 to move to gopkg.in/yaml.v3 instead. Would that be more reasonable?

That would be perfect because we already vendor it:

buildkit/go.mod

Line 183 in 5efd380

gopkg.in/yaml.v3 v3.0.1 // indirect

Copy link
Member

Choose a reason for hiding this comment

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

That would be perfect because we already vendor it:

Afaics it is only pulled in by the testify tests so would still increase the binary side.

https://github.com/cncf-tags/container-device-interface/pull/262/files# Why does it need to directly pull in 2 different yaml libraries?

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaics it is only pulled in by the testify tests so would still increase the binary side.

Ah yes indeed 🤔

# gopkg.in/yaml.v3
github.com/moby/buildkit/client/llb
github.com/moby/buildkit/client/llb.test
github.com/stretchr/testify/assert
github.com/stretchr/testify/assert/yaml
gopkg.in/yaml.v3

@elezar
Copy link

elezar commented Mar 22, 2025

We have released v1.0.1 including the switch to yaml v3

@crazy-max crazy-max changed the title vendor: tags.cncf.io/container-device-interface v1.0.0 vendor: tags.cncf.io/container-device-interface v1.0.1 Mar 22, 2025
@crazy-max crazy-max marked this pull request as ready for review March 24, 2025 17:38

import "syscall"

func osSync() {
Copy link
Member

Choose a reason for hiding this comment

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

@elezar This file should be called cache_darwin_test.go

Copy link

Choose a reason for hiding this comment

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

Noted. I will change this upstream.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Thanks. I haven't gotten around to it yet.

Copy link
Member

Choose a reason for hiding this comment

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

No worries! I recalled this comment, and went looking if there was an updated tag. It was an easy change, so thought I'd just open a PR 😄

@tonistiigi tonistiigi merged commit d42d8a3 into moby:master Mar 24, 2025
108 checks passed
@crazy-max crazy-max deleted the update-cdi branch April 8, 2025 12:20
@crazy-max crazy-max added this to the v0.21.0 milestone Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants