conversion-gen: support recursive types#49747
conversion-gen: support recursive types#49747k8s-github-robot merged 1 commit intokubernetes:masterfrom
Conversation
|
Hi @nikhita. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. DetailsInstructions 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. I understand the commands that are listed here. |
|
/ok-to-test |
|
@lavalamp I think you reviewed basically the same thing in gengo already. This time it's in conversion-gen. ptal. |
There was a problem hiding this comment.
In Go passing map means effectively passing a pointer to map. Since noone is deleting anything from that map, you end up with map with a bunch of things at the end.
This probably isn't a big deal, but I think calling it "currentlyBuildingTypes" is a bit misleading then.
There was a problem hiding this comment.
I agree. I was a bit unsure about the name too. How about "alreadyVisitedTypes"?
There was a problem hiding this comment.
It's definitely better in my opinion.
ad46f2a to
23d0445
Compare
|
/retest |
There was a problem hiding this comment.
Wondering, do we need the parameter inEqual at all? Can't we create the empty map here?
There was a problem hiding this comment.
We can't do that because we need the populated map in line 366, 372, 374 and 376 below. If we create an empty map here and those lines are executed, they will see the type as "not visited" - which will defeat our purpose. :)
Also, tried it out. Gives the goroutine stack exceeds 1000000000-byte limit error.
There was a problem hiding this comment.
oh, didn't realize the recursion goes through Equal and not equal.
There was a problem hiding this comment.
But, everything here in Equal can be moved to equal and then we can recursively call equal instead of Equal, can't it? Then my suggestion should work.
There was a problem hiding this comment.
Doesn't make the code easier to read though. Let's leave it as it is.
23d0445 to
f98b74b
Compare
|
/retest |
There was a problem hiding this comment.
here it's not about "been converted", but about comparing that two structs have the same memory structure. If they have, we can use unsafe.Pointer to replace conversion by a type cast.
Better:
// alreadyVisitedTypes holds all the types that have already been checked in the structural type recursion.
Currently, the conversion-gen goes into an infinite recursion for recursive types. rename: currentlyBuildingTypes -> alreadyVisitedTypes use a cachingEqual func update comment
f98b74b to
c4656c3
Compare
|
/lgtm |
|
@wojtek-t approved? |
|
/retest |
|
/approve no-issue |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nikhita, sttts, wojtek-t Associated issue: 47263 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
/test pull-kubernetes-unit |
|
/test pull-kubernetes-e2e-kops-aws |
|
/test pull-kubernetes-bazel |
|
/retest |
|
/retest Review the full test history for this PR. |
|
Submit Queue says |
|
/retest |
|
/test pull-kubernetes-e2e-kops-aws |
|
/retest |
1 similar comment
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 49651, 49707, 49662, 47019, 49747) |
Automatic merge from submit-queue apiextensions: validation for customresources - [x] Add types for validation of CustomResources - [x] Fix conversion-gen: #49747 - [x] Fix defaulter-gen: kubernetes/gengo#61 - [x] Convert to OpenAPI types - [x] Validate CR using go-openapi - [x] Validate CRD Schema - [x] Add integration tests - [x] Fix round trip tests: #51204 - [x] Add custom fuzzer functions - [x] Add custom conversion functions - [x] Fix data race while updating CRD: #50098 - [x] Add feature gate for CustomResourceValidation - [x] Fix protobuf generation Proposal: kubernetes/community#708 Additional discussion: #49879, #50625 **Release note**: ```release-note Add validation for CustomResources via JSON Schema. ``` /cc @sttts @deads2k
Currently, conversion-gen goes into an infinite recursion for recursive types. This fixes it to support recursive types.
Needed for #47263.
Release note:
/cc @sttts