Skip to content

Fix deepcopy for generics #274

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 5 commits into from
Aug 26, 2024

Conversation

Jefftree
Copy link
Member

@Jefftree Jefftree commented Aug 23, 2024

We had a regression with the deepcopy gen not being able to handle generic fields.

type Foo struct {
   field set.Set[string]
}

This fixes the name to not be trimmed for generics in goNameToName and also fixes a bug with named interfaces capturing generics not capturing generics properly.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 23, 2024
@k8s-ci-robot k8s-ci-robot requested review from sttts and thockin August 23, 2024 20:29
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 23, 2024
@thockin
Copy link
Member

thockin commented Aug 23, 2024

verified against k/k - this function seems like a great target for a unit test

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 23, 2024
@Jefftree Jefftree force-pushed the fix-deepcopy-generic branch from d1f1f55 to 9a9c4ca Compare August 26, 2024 16:14
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 26, 2024
@Jefftree Jefftree changed the title [WIP] Fix deepcopy generic Fix deepcopy for generics Aug 26, 2024
@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 Aug 26, 2024
@Jefftree Jefftree force-pushed the fix-deepcopy-generic branch from 9a9c4ca to 51ad459 Compare August 26, 2024 16:29
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 26, 2024
@Jefftree Jefftree force-pushed the fix-deepcopy-generic branch from 51ad459 to 0dc45e1 Compare August 26, 2024 18:40
@Jefftree
Copy link
Member Author

/assign @thockin
/cc @TheSpiritXIII

Updated with fixes.

@k8s-ci-robot
Copy link
Contributor

@Jefftree: GitHub didn't allow me to request PR reviews from the following users: TheSpiritXIII.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/assign @thockin
/cc @TheSpiritXIII

Updated with fixes.

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-sigs/prow repository.

@@ -10,5 +10,6 @@ require (

require (
github.com/go-logr/logr v0.2.0 // indirect
github.com/google/go-cmp v0.6.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.

This change is optional but I figured it'd be more useful to use cmp.Diff instead of a two complex structs that are quite hard to read in the tests. k/k already has this dependency https://github.com/kubernetes/kubernetes/blob/master/go.mod#L38 so we are not adding anything new for them.

@thockin
Copy link
Member

thockin commented Aug 26, 2024

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 Aug 26, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Aug 26, 2024
@k8s-ci-robot k8s-ci-robot merged commit a7b603a into kubernetes:master Aug 26, 2024
4 checks passed
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.

3 participants