-
Notifications
You must be signed in to change notification settings - Fork 209
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
use our own fork of go-yaml #511
Conversation
/assign @liggitt |
go.mod
Outdated
@@ -48,4 +46,5 @@ require ( | |||
golang.org/x/sync v0.8.0 // indirect | |||
golang.org/x/sys v0.23.0 // indirect | |||
golang.org/x/text v0.17.0 // indirect | |||
gopkg.in/yaml.v3 v3.0.1 // indirect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is coming from
go mod why gopkg.in/yaml.v3
# gopkg.in/yaml.v3
k8s.io/kube-openapi/cmd/openapi2smd
github.com/google/gnostic-models/openapiv2
gopkg.in/yaml.v3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I See, and the type is required, breaking this PR 😢
Error: pkg/util/proto/document_v3.go:291:31: cannot use s.GetDefault().ToRawInfo() (value of type *"gopkg.in/yaml.v3".Node) as *"sigs.k8s.io/yaml/goyaml.v3".Node value in argument to parseV3Interface
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jpbetz should we move github.com/google/gnostic-models/openapiv2
to our fork too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing gnostic models into another repo will cause import breakage (kubernetes/kubernetes#118340 and https://kubernetes.slack.com/archives/C0EG7JC6T/p1687271483899979) because client-go depends on it, and I would prefer not doing unless absolutely necessary. seeking some opinions from others, cc: @liggitt @sttts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Jefftree tried to mean, move the yaml imported version, not the package :)
/assign @jpbetz |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, sttts 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 |
/retest What do we do with the 1.20 CI failure? |
it is legit, I will investigate more |
Change-Id: Ic7dee4c0a08704a899194247dae9a1e97b5ffdae
@sttts it should be fixed now, I somehow added unnecessarely the dependency to |
/lgtm |
We have our own fork of go-yaml kubernetes-sigs/yaml#76 so we can simplify the dependency graph in kubernetes if we end the path in kubernetes-sigs/yaml
Ref kubernetes-sigs/structured-merge-diff#264