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

Change references of googleapis/gnostic -> google/gnostic #285

Merged
merged 1 commit into from Mar 14, 2022

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Mar 10, 2022

See kubernetes/kubernetes#108644

k/k and k/kube-openapi need to change googleapis/gnostic -> google/gnostic at the same time to avoid type check failure.

Test failure: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/108644/pull-kubernetes-integration/1502033895208521728

Change references of googleapis/gnostic -> google/gnostic

Also upgrade gnostic from 0.5.5 to 0.6.6. No significant changes for openapi_v2 and openapi_v3,

since April 26, 2021 @ version v0.5.5

  • PR 250 updates the .md file
  • PR 259 Renames googleapis -> google
  • PR 270 updates the protoc version

/assign @apelisse

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 10, 2022
@apelisse
Copy link
Member

OK Thanks,
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, Jefftree

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 14, 2022
@k8s-ci-robot k8s-ci-robot merged commit c7e0de3 into kubernetes:master Mar 14, 2022
@mattrobenolt
Copy link

I think this breaks dependencies like k8s.io/client-go too.

I'm not entirely sure, since this is a bit over my head in how Go resolves dependencies.

But it seems that because the old version googleapis/gnostic is just a redirect to the newer version google/gnostic, I think explicitly in the dependency resolver, it doesn't treat them the same.

So what's happening is I'm getting an indirect dependency for both google/gnostic v0.6.6 and googleapis/gnostic v0.5.5 from client-go. I think this confuses the compiler or somewhere in that it tries to alias the googleapis/gnostic version as google/gnostic and they are incompatible because different versions? I'm not exactly sure.

But the exact compilation error when updating deps now is:

/go/pkg/mod/k8s.io/client-go@v0.23.4/applyconfigurations/meta/v1/unstructured.go:64:38: cannot use doc (variable of type *"github.com/googleapis/gnostic/openapiv2".Document) as *"github.com/google/gnostic/openapiv2".Document value in argument to proto.NewOpenAPIData (compile)

I'm not exactly sure how to work around this or what the resolution is.

@mattrobenolt
Copy link

I guess I worked around it by pinning my indirect dependency to a sha before this one, and it resolved it.

I wasn't able to reproduce it on a new project either, so I'm not sure the perfect storm that leads to this being a problem.

@Jefftree
Copy link
Member Author

@mattrobenolt Yes, we are aware there will be a version mismatch with kubernetes/kubernetes (and subsequently the client-go applyconfigurations) until kubernetes/kubernetes#108644 is merged.

@liggitt
Copy link
Member

liggitt commented Mar 15, 2022

I think this breaks dependencies like k8s.io/client-go too.

it shouldn't, as long as you use the level of kube-openapi referenced by client-go. did you run go get -u to bump all dependencies to their latest versions? client-go has many v0.x.y dependencies that will not work with

mengqiy pushed a commit to mengqiy/kpt-functions-catalog that referenced this pull request Mar 23, 2022
kubernetes/kube-openapi#285 will break
things if we are using inconsitent version of kube-openapi
mengqiy pushed a commit to GoogleContainerTools/kpt-functions-catalog that referenced this pull request Mar 24, 2022
* Use the same version as in the sdk

kubernetes/kube-openapi#285 will break
things if we are using inconsitent version of kube-openapi

* use kube-openapi e816edb12b65 for apply-replacements
yuwenma pushed a commit to GoogleContainerTools/kpt-functions-catalog that referenced this pull request May 4, 2022
* Use the same version as in the sdk

kubernetes/kube-openapi#285 will break
things if we are using inconsitent version of kube-openapi

* use kube-openapi e816edb12b65 for apply-replacements
@Jefftree Jefftree deleted the proto-upgrade branch March 21, 2023 15:40
reneleonhardt added a commit to reneleonhardt/mesh that referenced this pull request Oct 14, 2023
kube-openapi pinned before the migration to google/gnostic
kubernetes/kube-openapi#285
google/gnostic#397
reneleonhardt added a commit to reneleonhardt/mesh that referenced this pull request Oct 14, 2023
kube-openapi pinned before the migration to google/gnostic
kubernetes/kube-openapi#285
google/gnostic#397
reneleonhardt added a commit to reneleonhardt/mesh that referenced this pull request Oct 14, 2023
kube-openapi pinned before the migration to google/gnostic
kubernetes/kube-openapi#285
google/gnostic#397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants