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

🌱 GVKForObject should handle multiple GVKs in Scheme gracefully #2192

Merged
merged 1 commit into from
Feb 16, 2023
Merged

🌱 GVKForObject should handle multiple GVKs in Scheme gracefully #2192

merged 1 commit into from
Feb 16, 2023

Conversation

vincepri
Copy link
Member

  • If the user sets the GVK, make sure that's in the list of the GVK returned from the Scheme.
  • Always print out the multiple GVKs when erroring out
  • Add more comments on where to find more information about this issue

Signed-off-by: Vince Prignano [email protected]

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 13, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2023
@vincepri vincepri mentioned this pull request Feb 13, 2023
Closed
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 13, 2023
@vincepri vincepri force-pushed the gvk-for-object-multiple-hits branch from bc9f14f to 48ec279 Compare February 13, 2023 17:27
justinsb
justinsb reviewed Feb 13, 2023
View reviewed changes
pkg/client/apiutil/apimachinery.go Outdated
return schema.GroupVersionKind{}, fmt.Errorf(
"multiple group-version-kinds associated with type %T, refusing to guess at one", obj)
"multiple GroupVersionKinds associated with type %T within the Scheme: this can happen when a type is registered for multiple GVKs at the same time, callers can either fix their type registration to only register it once, or specify the GroupVersionKind to use for object passed in; refusing to guess at one: %q", obj, gvks)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit/thought: Should we say "go type" instead of "type"? I think it might help people understand what has happened ... I know I sometimes conflate kind vs resource vs type, and it's that association that has gone wrong here.

justinsb
justinsb reviewed Feb 13, 2023
View reviewed changes
pkg/client/apiutil/apimachinery.go
return gvk, nil
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe log if we reject for this reason? It's unlikely, but I would be very confused if I ever set the wrong GVK here which was then silently ignored...

Copy link
Member Author

Choose a reason for hiding this comment

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

ptal, refactored to wrap errors differently depending on what we have

@justinsb
Copy link
Contributor

Thanks for doing this @vincepri ... I like it and I think it's a reasonable approach. Had some suggestions on being even more verbose, but this is already a great improvement :-)

@vincepri vincepri force-pushed the gvk-for-object-multiple-hits branch from 48ec279 to 67d3283 Compare February 13, 2023 19:59
@justinsb
Copy link
Contributor

Looks really good - thanks @vincepri

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 13, 2023
@vincepri
Copy link
Member Author

/hold

for other reviewers @alvaroaleman

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 13, 2023
@vincepri vincepri changed the title WIP: GVKForObject should handle multiple GVKs in Scheme gracefully 🌱 GVKForObject should handle multiple GVKs in Scheme gracefully Feb 13, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2023
@vincepri
Copy link
Member Author

/cherry-pick release-0.14

@k8s-infra-cherrypick-robot
Copy link

@vincepri: once the present PR merges, I will cherry-pick it on top of release-0.14 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sbueringer
sbueringer reviewed Feb 14, 2023
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Just a nit otherwise lgtm

pkg/client/apiutil/apimachinery.go Outdated Show resolved Hide resolved
@vincepri
GVKForObject should handle multiple GVKs in Scheme gracefully
ea10051 
- If the user sets the GVK, make sure that's in the list of the GVK
  returned from the Scheme.
- Always print out the multiple GVKs when erroring out
- Add more comments on where to find more information about this issue

Signed-off-by: Vince Prignano <[email protected]>
@vincepri vincepri force-pushed the gvk-for-object-multiple-hits branch from 67d3283 to ea10051 Compare February 14, 2023 16:49
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2023
@sbueringer
Copy link
Member

/retest

1 similar comment
@sbueringer
Copy link
Member

/retest

@sbueringer
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2023
@vincepri
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2023
@k8s-ci-robot k8s-ci-robot merged commit 25d6edd into kubernetes-sigs:master Feb 16, 2023
@k8s-infra-cherrypick-robot k8s-infra-cherrypick-robot mentioned this pull request Feb 16, 2023
Merged
@k8s-infra-cherrypick-robot
Copy link

@vincepri: new pull request created: #2202

In response to this:

/cherry-pick release-0.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

@sbueringer sbueringer sbueringer left review comments

@FillZpp FillZpp Awaiting requested review from FillZpp

Assignees

@justinsb justinsb

@sbueringer sbueringer

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vincepri @k8s-ci-robot @justinsb @k8s-infra-cherrypick-robot @sbueringer