⚠️ Change ClusterResourceSetBinding Bindings field from []*ResourceSetBinding to []ResourceSetBinding #12476

sbueringer · 2025-07-10T12:41:54Z

Signed-off-by: Stefan Büringer buringerst@vmware.com

What this PR does / why we need it:
One of the more interesting conversion PRs..

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Part of #10852

sbueringer · 2025-07-10T12:42:09Z

/test pull-cluster-api-e2e-main

sbueringer · 2025-07-10T12:42:18Z

/assign @fabriziopandini @JoelSpeed @sivchari @mboersma

sbueringer · 2025-07-10T12:52:43Z

/test pull-cluster-api-e2e-main

api/addons/v1beta1/conversion_test.go

[]*ResourceSetBinding to []ResourceSetBinding Signed-off-by: Stefan Büringer buringerst@vmware.com

sbueringer · 2025-07-10T13:13:03Z

/test pull-cluster-api-e2e-main

k8s-ci-robot · 2025-07-10T15:17:51Z

@sbueringer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-apidiff-main	`71a7db1`	link	false	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

sbueringer · 2025-07-10T17:17:36Z

/override pull-cluster-api-e2e-main

k8s-ci-robot · 2025-07-10T17:17:39Z

@sbueringer: Overrode contexts on behalf of sbueringer: pull-cluster-api-e2e-main

In response to this:

/override pull-cluster-api-e2e-main

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.

JoelSpeed · 2025-07-11T09:32:24Z

/lgtm
/approve

k8s-ci-robot · 2025-07-11T09:32:31Z

LGTM label has been added.

Git tree hash: b972e176da7c4ea4cb7bc6ea93a35f42dabe55aa

k8s-ci-robot · 2025-07-11T09:32:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JoelSpeed · 2025-07-11T09:32:46Z

/hold do we want anyone else to check on this first?

sivchari

/lgtm

I left nit, but if you decide to go with it, feel free to merge.

sivchari · 2025-07-11T09:54:20Z

api/addons/v1beta2/clusterresourcesetbinding_types.go

-	for _, binding := range c.Spec.Bindings {
-		if binding.ClusterResourceSetName == clusterResourceSet.Name {
-			return binding
+	for i := range c.Spec.Bindings {


I've read the discussion and commented.
I can confirm that the copyloopvar linter is already in place and the original code is fine.
but it's certainly a good idea to keep it this way in terms of visualization.

The original code was not fine, that's why I changed it :) (tests broke)

The original code with the pointer change returned a copy of the binding and not the same binding that is part of the c.Spec.Bindings slice

The code is fine for the linter, but that doesn't mean it works as expected :)

Interesting. Surely the only this codes aren't problem, but considering about entire code base, the copy element might be invalid. This case is good example.
Thanks!

To give some more context.

The previous problem (a few Go versions ago) with returning a pointer to a loop var was that the pointer was basically always referring to the last element that was stored in the loop var.

Then they changed it in go so a pointer to the loop var refers to the correct element.

But the loop var is a copy of the slice element not the actual element

sivchari · 2025-07-11T09:57:47Z

api/addons/v1beta2/clusterresourcesetbinding_types.go

 	c.Spec.Bindings = append(c.Spec.Bindings, binding)
-	return binding
+	return &c.Spec.Bindings[len(c.Spec.Bindings)-1]


Suggested change

return &c.Spec.Bindings[len(c.Spec.Bindings)-1]

return &binding

I think it's simpler but is this to make the format consistent with the above?

Simpler but it's not necessarily correct. I'm sure about the case above, I'm not entirely sure if binding here is the same element as in the slice (probably both would work here in this case, but I want to play it safe)

sbueringer · 2025-07-11T12:50:26Z

Fine to let Fabrizio review as well pre-merge

fabriziopandini · 2025-07-14T10:19:36Z

/lgtm
Nice to get rid of this pointer in front of each element

sbueringer · 2025-07-14T10:34:43Z

/hold cancel

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 10, 2025

k8s-ci-robot requested review from chrischdi and elmiko July 10, 2025 12:42

k8s-ci-robot assigned fabriziopandini, JoelSpeed, mboersma and sivchari Jul 10, 2025

sbueringer added the area/clusterresourceset Issues or PRs related to clusterresourcesets label Jul 10, 2025

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Jul 10, 2025

sbueringer mentioned this pull request Jul 10, 2025

✨ Enable ssatags KAL linter #12470

Merged

sbueringer changed the title ~~⚠️ Change ClusterResourceSetBinding Bindings field from []*ResourceSetBinding to []ResourceSetBinding~~ [WIP] ⚠️ Change ClusterResourceSetBinding Bindings field from []*ResourceSetBinding to []ResourceSetBinding Jul 10, 2025

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 Jul 10, 2025

sbueringer force-pushed the pr-fix-crb branch 2 times, most recently from a711313 to 55cf6dc Compare July 10, 2025 12:52

sbueringer changed the title ~~[WIP] ⚠️ Change ClusterResourceSetBinding Bindings field from []*ResourceSetBinding to []ResourceSetBinding~~ ⚠️ Change ClusterResourceSetBinding Bindings field from []*ResourceSetBinding to []ResourceSetBinding Jul 10, 2025

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 Jul 10, 2025

JoelSpeed reviewed Jul 10, 2025

View reviewed changes

api/addons/v1beta1/conversion_test.go Outdated Show resolved Hide resolved

Change ClusterResourceSetBinding Bindings field from

71a7db1

[]*ResourceSetBinding to []ResourceSetBinding Signed-off-by: Stefan Büringer buringerst@vmware.com

sbueringer force-pushed the pr-fix-crb branch from 55cf6dc to 71a7db1 Compare July 10, 2025 13:12

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 11, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2025

sivchari reviewed Jul 11, 2025

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2025

k8s-ci-robot merged commit 5256214 into kubernetes-sigs:main Jul 14, 2025
19 of 20 checks passed

k8s-ci-robot added this to the v1.11 milestone Jul 14, 2025

sbueringer deleted the pr-fix-crb branch July 14, 2025 11:50

	return &c.Spec.Bindings[len(c.Spec.Bindings)-1]
	return &binding

⚠️ Change ClusterResourceSetBinding Bindings field from []*ResourceSetBinding to []ResourceSetBinding #12476

⚠️ Change ClusterResourceSetBinding Bindings field from []*ResourceSetBinding to []ResourceSetBinding #12476

Conversation

sbueringer commented Jul 10, 2025

Uh oh!

sbueringer commented Jul 10, 2025

Uh oh!

sbueringer commented Jul 10, 2025

Uh oh!

sbueringer commented Jul 10, 2025

Uh oh!

Uh oh!

sbueringer commented Jul 10, 2025

Uh oh!

k8s-ci-robot commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbueringer commented Jul 10, 2025

Uh oh!

k8s-ci-robot commented Jul 10, 2025

Uh oh!

JoelSpeed commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 11, 2025

Uh oh!

k8s-ci-robot commented Jul 11, 2025

Uh oh!

JoelSpeed commented Jul 11, 2025

Uh oh!

sivchari left a comment

Choose a reason for hiding this comment

Uh oh!

sivchari Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivchari Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivchari Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Jul 11, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer commented Jul 11, 2025

Uh oh!

fabriziopandini commented Jul 14, 2025

Uh oh!

sbueringer commented Jul 14, 2025

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Jul 10, 2025 •

edited

Loading

sbueringer Jul 11, 2025 •

edited

Loading

sbueringer Jul 11, 2025 •

edited

Loading