open-cluster-management-io / api

Core APIs for open cluster management
https://open-cluster-management.io
Apache License 2.0
251 stars 77 forks source link

:bug: run `make update` fail because deepcopy doesn't support generic … #288

Closed xuezhaojun closed 1 year ago

xuezhaojun commented 1 year ago

…type.

Summary

Run make update fail because of deepcopy-gen doesn't support generic type yet. https://github.com/kubernetes/gengo/issues/225

It will log errors:

W1017 14:10:32.195161   62703 parse.go:863] Making unsupported type entry "T" for: &types.TypeParam{check:(*types.Checker)(nil), id:0x1, obj:(*types.TypeName)(0xc009651900), index:0, bound:(*types.Interface)(0xc00009a960)}
F1017 14:10:32.221298   62703 deepcopy.go:890] Hit an unsupported type open-cluster-management.io/api/cluster/v1alpha1.ClusterRolloutStatusFunc[T] for open-cluster-management.io/api/cluster/v1alpha1.ClusterRolloutStatusFunc[T], from open-cluster-management.io/api/cluster/v1alpha1.RolloutHandler[T]

The generic type was introduced by this PR: https://github.com/open-cluster-management-io/api/pull/276

In this PR, we change the strategy from 'opt-out'(via deepcopy-gen=false comment) to 'opt-in'(via deepcopy-gen=true comment)

Related issue(s)

Fixes #

xuezhaojun commented 1 year ago

/assign @qiujian16

cc @haoqing0110 @serngawy

xuezhaojun commented 1 year ago

After this PR merged, we should add make update as a ci step.

serngawy commented 1 year ago

Thanks @xuezhaojun , I'm able to reproduce the error as well. It should be enough to set the // +k8s:deepcopy-gen=false to ClusterRolloutStatusFunc[T any] type similar to RolloutHandler[T any] type here . It is strange that it didn't show up with original PR . Based on the deepCopy code doc here setting the deepcopy-gen=false should work.

xuezhaojun commented 1 year ago

Thanks @xuezhaojun , I'm able to reproduce the error as well. It should be enough to set the // +k8s:deepcopy-gen=false to ClusterRolloutStatusFunc[T any] type similar to RolloutHandler[T any] type here . It is strange that it didn't show up with original PR . Based on the deepCopy code doc here setting the deepcopy-gen=false should work.

Hi, @serngawy , unfortunately, adding // +k8s:deepcopy-gen=false to ClusterRolloutStatusFunc[T any] doesn't work, because the root cause is generic type, when deepcopy-gen parsing the code of a generic type, it end up with getting 2 types:

I1017 16:48:42.317012    5707 deepcopy.go:273] Type open-cluster-management.io/api/cluster/v1alpha1.RolloutHandler[T any] is not copyable
I1017 16:48:42.317023    5707 deepcopy.go:276] Type open-cluster-management.io/api/cluster/v1alpha1.RolloutHandler[T] is copyable

You can see from the above log, // +k8s:deepcopy-gen=false only works for RolloutHandler[T any] so it's shown as not capable. But for RolloutHandler[T](which I think is a result of incorrect parsering) is copyable.

qiujian16 commented 1 year ago

hrm, I thought make verify already cover the process of make update...

qiujian16 commented 1 year ago

/approve

openshift-ci[bot] commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, xuezhaojun

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: - ~~[OWNERS](https://github.com/open-cluster-management-io/api/blob/main/OWNERS)~~ [qiujian16] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
xuezhaojun commented 1 year ago

This PR is not the final solution, I will follow up to watch the progress of code-gen libs's generic type supporting, and we will solve this before upgrade these generic types to v1beta1.

xuezhaojun commented 1 year ago

/assign @haoqing0110

haoqing0110 commented 1 year ago

/lgtm

dhaiducek commented 1 year ago

@xuezhaojun I noticed this PR merged, but I opened a PR off of my branch in case there was interest in going the controller-gen route instead:

serngawy commented 1 year ago

@xuezhaojun I noticed this PR merged, but I opened a PR off of my branch in case there was interest in going the controller-gen route instead:

+1 for using controller-gen it's more cleaner , we don't need to set deep-copy for every struct

xuezhaojun commented 1 year ago

@serngawy @dhaiducek Thanks for submitting the fix! vote to this alternative, great work!