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

🐛 Use controller-runtime for deepcopy generation for cluster:v1alpha1 #291

Merged
merged 2 commits into from
Oct 27, 2023
Merged

🐛 Use controller-runtime for deepcopy generation for cluster:v1alpha1 #291

merged 2 commits into from
Oct 27, 2023

Conversation

dhaiducek
Copy link
Member

@dhaiducek dhaiducek commented Oct 18, 2023 •
edited

Summary

GenGo doesn't seem to respect the +k8s:deepcopy-gen:false tag, so this is an alternative to #288, where controller-gen is used for cluster:v1alpha1 deepcopy generation rather than selective deepcopy generation.

Related issue(s)

Followup to (and reverts):

@dhaiducek dhaiducek changed the title 🐛 🐛 Use controller-runtime for deepcopy generation for cluster:v1alpha1 Oct 18, 2023
@dhaiducek
Revert "Fix: run make update fail because deepcopy doesn't support …
8f0ff2d 
…generic type. (open-cluster-management-io#288)"

This reverts commit ae208c8.

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>
@dhaiducek dhaiducek force-pushed the controller-gen-2 branch 2 times, most recently from 1bc0888 to 817eca9 Compare October 18, 2023 21:53
@dhaiducek dhaiducek mentioned this pull request Oct 18, 2023
Merged
@xuezhaojun
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 19, 2023
@xuezhaojun
Copy link
Member

/assign @qiujian16

@xuezhaojun
Copy link
Member

@dhaiducek The bot may squash commits after merging the PR, and also, we may be able to replace deepcopy-gen with controller-gen entirely so that there is no exception in the makefile flow.

@dhaiducek
Copy link
Member Author

@dhaiducek The bot may squash commits after merging the PR, and also, we may be able to replace deepcopy-gen with controller-gen entirely so that there is no exception in the makefile flow.

Good point--I'll create a separate PR for the other updates.

@dhaiducek dhaiducek mentioned this pull request Oct 19, 2023
Merged
@openshift-ci openshift-ci bot removed the lgtm label Oct 19, 2023
@dhaiducek
Copy link
Member Author

I've split out the architecture updates into a new PR:

@dhaiducek
Copy link
Member Author

we may be able to replace deepcopy-gen with controller-gen entirely so that there is no exception in the makefile flow.

I'm working on implementing a controller-gen generation, but it also has an issue where it's generating everything without errors but then this generic sets.Set isn't getting properly instantiated with string as it's defined for some reason, so my code editor is complaining:

api/cluster/v1beta1/helpers.go

Line 217 in d7736a1

type ClusterGroupsMap map[GroupKey]sets.Set[string]

@dhaiducek
Copy link
Member Author

For now, this PR might be the easiest path forward given controller-gen it seems might also need a different exception for that edge case? I've spent quite a bit of time on this without a clear resolution, so I'm going to pause on this work for now, but this PR it seems would work as-is.

qiujian16
qiujian16 reviewed Oct 23, 2023
View reviewed changes
Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

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

@xuezhaojun will this impact the make verify?

Makefile Show resolved Hide resolved
hack/update-deepcopy.sh Show resolved Hide resolved
@xuezhaojun
Copy link
Member

@xuezhaojun will this impact the make verify?

No, from the code, verify and update are using the same update-script under the hood.

api/hack/verify-deepcopy.sh

Line 6 in d7736a1

VERIFY=--verify-only ${SCRIPT_ROOT}/hack/update-deepcopy.sh

@dhaiducek
Use controller-gen for deepcopy cluster:v1alpha1
baf4645 
GenGo isn't respecting the `+k8s:deepcopy-gen=false` tag to skip
generation for the generic type

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>
qiujian16
qiujian16 reviewed Oct 27, 2023
View reviewed changes
Copy link
Member

@qiujian16 qiujian16 left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 27, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@openshift-ci openshift-ci bot merged commit 2b7f212 into open-cluster-management-io:main Oct 27, 2023
10 checks passed
@dhaiducek dhaiducek deleted the controller-gen-2 branch October 27, 2023 02:29
haoqing0110 pushed a commit to haoqing0110/api that referenced this pull request Nov 24, 2023
@dhaiducek @haoqing0110
🐛 Use controller-runtime for deepcopy generation for cluster:v1alpha1 (…
0c16273 
…open-cluster-management-io#291)

* Revert "Fix: run `make update` fail because deepcopy doesn't support generic type. (open-cluster-management-io#288)"

This reverts commit ae208c8.

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>

* Use `controller-gen` for deepcopy cluster:v1alpha1

GenGo isn't respecting the `+k8s:deepcopy-gen=false` tag to skip
generation for the generic type

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>

---------

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>
haoqing0110 pushed a commit to haoqing0110/api that referenced this pull request Nov 24, 2023
@dhaiducek @haoqing0110
🐛 Use controller-runtime for deepcopy generation for cluster:v1alpha1 (…
9434526 
…open-cluster-management-io#291)

* Revert "Fix: run `make update` fail because deepcopy doesn't support generic type. (open-cluster-management-io#288)"

This reverts commit ae208c8.

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>

* Use `controller-gen` for deepcopy cluster:v1alpha1

GenGo isn't respecting the `+k8s:deepcopy-gen=false` tag to skip
generation for the generic type

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>

---------

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>
haoqing0110 pushed a commit to haoqing0110/api that referenced this pull request Nov 24, 2023
@serngawy @haoqing0110
Update rollout lib (open-cluster-management-io#276)
55ed983 
Signed-off-by: melserngawy <melserng@redhat.com>

Fix: run `make update` fail because deepcopy doesn't support generic type. (open-cluster-management-io#288)

Signed-off-by: xuezhaojun <zxue@redhat.com>

Fix verify ci step missing. (open-cluster-management-io#289)

Signed-off-by: xuezhaojun <zxue@redhat.com>

:bug: Use controller-runtime for deepcopy generation for cluster:v1alpha1 (open-cluster-management-io#291)

* Revert "Fix: run `make update` fail because deepcopy doesn't support generic type. (open-cluster-management-io#288)"

This reverts commit ae208c8.

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>

* Use `controller-gen` for deepcopy cluster:v1alpha1

GenGo isn't respecting the `+k8s:deepcopy-gen=false` tag to skip
generation for the generic type

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>

---------

Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>

🐛 add ca bundle to addon proxy settings (open-cluster-management-io#293)

Signed-off-by: Yang Le <yangle@redhat.com>

Revert "remove ClusterSet ClusterSetBinding API version v1beta1 (open-cluster-management-io#266)"

This reverts commit 9675ab7.

Signed-off-by: haoqing0110 <qhao@redhat.com>
openshift-merge-bot bot pushed a commit that referenced this pull request Nov 24, 2023
@haoqing0110 @serngawy
Update rollout lib (#276) (#298)
63d09c6 
Fix: run `make update` fail because deepcopy doesn't support generic type. (#288)



Fix verify ci step missing. (#289)



:bug: Use controller-runtime for deepcopy generation for cluster:v1alpha1 (#291)

* Revert "Fix: run `make update` fail because deepcopy doesn't support generic type. (#288)"

This reverts commit ae208c8.



* Use `controller-gen` for deepcopy cluster:v1alpha1

GenGo isn't respecting the `+k8s:deepcopy-gen=false` tag to skip
generation for the generic type



---------



🐛 add ca bundle to addon proxy settings (#293)



Revert "remove ClusterSet ClusterSetBinding API version v1beta1 (#266)"

This reverts commit 9675ab7.

Signed-off-by: haoqing0110 <qhao@redhat.com>
Co-authored-by: Mohamed ElSerngawy <melserng@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qiujian16 qiujian16 qiujian16 left review comments

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@qiujian16 qiujian16

@xuezhaojun xuezhaojun

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@dhaiducek @xuezhaojun @qiujian16