✨ Integrate CRS code into regular code structure #11943

mcbenjemaa · 2025-03-08T15:48:09Z

What this PR does / why we need it:

This PR moves the CRS code from addons to regular CAPI including api and controllers.
Ref: #11114 (comment)

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

Closes #11114

/area clusterresourceset

/cc @fabriziopandini sbueringer

main.go

.golangci.yml

controllers/alias.go

main.go

mcbenjemaa · 2025-03-11T16:23:33Z

I'm getting many lint issues with:

addons/v1beta1/clusterresourcesetbinding_types.go:192:2: maxlength: field Bindings must have a maximum items, add kubebuilder:validation:MaxItems marker (kal)
        Bindings []*ResourceSetBinding `json:"bindings,omitempty"`
        ^
api/addons/v1beta1/clusterresourcesetbinding_types.go:197:2: maxlength: field ClusterName must have a maximum length, add kubebuilder:validation:MaxLength marker (kal)
        ClusterName string `json:"clusterName,omitempty"`
        ^
api/addons/v1beta1/clusterresourcesetbinding_types.go:211:2: maxlength: field Items must have a maximum items, add kubebuilder:validation:MaxItems marker (kal)

can i disable this?

sbueringer · 2025-03-11T16:52:38Z

@mcbenjemaa In general yes by changing excludes. But I should have them fixed until tomorrow on main. So I would just ignore them for now and then rebase

sbueringer · 2025-03-11T18:10:41Z

#11909 is merged
I'll try to get #11949 merged tomorrow during the day

Afterwards there should be no other PRs conflicting with this PR. But I think if you have time already we can already try to get this PR into a state where tests and e2e tests are green (as I mentioned linter can be ignored for now, I would take another look after both other PRs are merged)

mcbenjemaa · 2025-03-12T11:50:25Z

How, we can fix pull-cluster-api-apidiff-main

sbueringer · 2025-03-12T11:53:07Z

How, we can fix pull-cluster-api-apidiff-main

Doesn't have to be fixed. apidiff just shows the API differences. It's then up to maintainers to determine if the changes are intended / acceptable. At a first glance it looks like they are, but I'll double check when I review the PR itself

These two are much more important:

mcbenjemaa · 2025-03-12T12:26:01Z

I will ignore all kal related lints for now.

sbueringer · 2025-03-12T12:28:01Z

@mcbenjemaa Started the CAPI controller locally with tilt. It doesn't come up because of this error

runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0x4020954390 stack=[0x4020954000, 0x4040954000]

** execution is paused because your program is panicking **
To continue the execution please connect your client to the debugger.
Stack trace:
 0  0x0000000000087a60 in runtime.throw
    at /Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/runtime/panic.go:1058
 1  0x000000000006add8 in runtime.newstack
    at /Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/runtime/stack.go:1117
 2  0x0000000002463cec in sigs.k8s.io/cluster-api/webhooks.(*ClusterResourceSet).SetupWebhookWithManager
    at /Users/buringerst/code/src/sigs.k8s.io/cluster-api/webhooks/alias.go:117
 3  0x0000000002463cbc in sigs.k8s.io/cluster-api/webhooks.(*ClusterResourceSet).SetupWebhookWithManager
    at /Users/buringerst/code/src/sigs.k8s.io/cluster-api/webhooks/alias.go:118
 4  0x0000000002463cbc in sigs.k8s.io/cluster-api/webhooks.(*ClusterResourceSet).SetupWebhookWithManager
    at /Users/buringerst/code/src/sigs.k8s.io/cluster-api/webhooks/alias.go:118
...

Basically the SetupWebhookWithManager funcs in alias.go are calling themselves. They should call the corresonding SetupWebhookWithManager func's in the internal/webhooks package instead

mcbenjemaa · 2025-03-12T12:43:34Z

Regarding pull-cluster-api-e2e-blocking-main

I tested in my local machine, but I'm getting another error.
the error in the CI, seems to be related to the capi controller manager not running.

mcbenjemaa · 2025-03-12T12:47:23Z

@mcbenjemaa Started the CAPI controller locally with tilt. It doesn't come up because of this error

runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0x4020954390 stack=[0x4020954000, 0x4040954000]

** execution is paused because your program is panicking **
To continue the execution please connect your client to the debugger.
Stack trace:
 0  0x0000000000087a60 in runtime.throw
    at /Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/runtime/panic.go:1058
 1  0x000000000006add8 in runtime.newstack
    at /Users/buringerst/code/pkg/mod/golang.org/[email protected]/src/runtime/stack.go:1117
 2  0x0000000002463cec in sigs.k8s.io/cluster-api/webhooks.(*ClusterResourceSet).SetupWebhookWithManager
    at /Users/buringerst/code/src/sigs.k8s.io/cluster-api/webhooks/alias.go:117
 3  0x0000000002463cbc in sigs.k8s.io/cluster-api/webhooks.(*ClusterResourceSet).SetupWebhookWithManager
    at /Users/buringerst/code/src/sigs.k8s.io/cluster-api/webhooks/alias.go:118
 4  0x0000000002463cbc in sigs.k8s.io/cluster-api/webhooks.(*ClusterResourceSet).SetupWebhookWithManager
    at /Users/buringerst/code/src/sigs.k8s.io/cluster-api/webhooks/alias.go:118
...

Basically the SetupWebhookWithManager funcs in alias.go are calling themselves. They should call the corresonding SetupWebhookWithManager func's in the internal/webhooks package instead

You are right 😄

mcbenjemaa · 2025-03-12T15:41:03Z

what's wrong with netlify
deploy/netlify

sbueringer · 2025-03-12T17:08:46Z

Not sure. But I would just ignore Netlify. Usually it always works but once in a month it fails with something. In this case

4:35:31 PM: mise [email protected]   download cpython-3.13.2+20250311-x86_64-unknown-linux-gnu-install_only_stripped.tar.gz
4:35:31 PM: mise ERROR failed to install core:[email protected]
4:35:31 PM: mise ERROR HTTP status server error (500 Internal Server Error) for url (https://github.com/astral-sh/python-build-standalone/releases/download/20250311/cpython-3.13.2+20250311-x86_64-unknown-linux-gnu-install_only_stripped.tar.gz)

sbueringer

Did a quick review. I think these findings should fix the test failures as well

test/framework/ownerreference_helpers.go

cmd/clusterctl/internal/scheme/scheme.go

api/addons/v1beta1/v1beta2_condition_consts.go

cmd/clusterctl/internal/test/fake_proxy.go

internal/apis/addons/v1alpha3/clusterresourceset_types.go

internal/webhooks/clusterresourcesetbinding_webhook.go

test/framework/convenience.go

.golangci.yml

.golangci-kal.yml

sbueringer · 2025-03-12T21:12:09Z

@mcbenjemaa #11949 merged. Let's rebase on top of main. I think that was the last PR that had conflict potential

mcbenjemaa · 2025-03-13T11:31:03Z

@sbueringer thanks for your notes.

sbueringer · 2025-03-13T11:49:07Z

@mcbenjemaa Btw feel free to squash

sbueringer · 2025-03-13T11:50:19Z

Regarding the findings here: https://github.com/kubernetes-sigs/cluster-api/actions/runs/13833818179/job/38704018628?pr=11943

You can simply add the api/addons/v1beta1 path to the existing exclude:

cluster-api/.golangci-kal.yml

Line 89 in afd68cd

- path: "api/v1beta1/*|api/v1alpha1/*"

controllers/alias.go

sbueringer · 2025-03-13T12:15:41Z

/test pull-cluster-api-test-main

I think this one is an unrelated flake

So unit tests seems to be fine, but I think we'll need the linter fixes to get everything green (especially e2e tests)

(expectation is that we should be able to get the linter green now with some fixes + #11943 (comment))

Refactor: MOVE CRS to his own addons group Fix go licenses Fix webhook alias SetupWebhookWithManager call Fix envtest, add addonsv1 scheme Fix Test_* with no addonsv1 scheme Fix APIGroup for addonsv1 Address reviews and fix missing imports Generate API Fix CRS controllers call

k8s-ci-robot · 2025-03-13T13:34:07Z

@mcbenjemaa: 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	`912fd81`	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.

mcbenjemaa · 2025-03-13T13:34:25Z

Should be ready.

sbueringer · 2025-03-13T13:40:26Z

/test pull-cluster-api-e2e-main

sbueringer · 2025-03-13T14:12:32Z

Thank you very much!

/lgtm
/approve

/hold cancel

k8s-ci-robot · 2025-03-13T14:12:41Z

LGTM label has been added.

Git tree hash: f1dc68474ccd10e890bb013c755503a7679a2ab0

k8s-ci-robot · 2025-03-13T14:12:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

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

sbueringer · 2025-03-13T16:13:23Z

@mcbenjemaa Thank you very much!

k8s-ci-robot requested review from fabriziopandini and sbueringer March 8, 2025 15:48

k8s-ci-robot added area/clusterresourceset Issues or PRs related to clusterresourcesets cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 8, 2025

vincepri reviewed Mar 9, 2025

View reviewed changes

main.go Outdated Show resolved Hide resolved

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

sbueringer reviewed Mar 10, 2025

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

controllers/alias.go Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 10, 2025

mcbenjemaa force-pushed the crs branch from 67d071b to 81c8565 Compare March 11, 2025 16:45

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 11, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2025

sbueringer mentioned this pull request Mar 12, 2025

⚠️ Add MaxLength & MinLength markers #11949

Merged

mcbenjemaa force-pushed the crs branch from 81c8565 to 3fa4cf7 Compare March 12, 2025 11:54

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2025

sbueringer reviewed Mar 12, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2025

mcbenjemaa force-pushed the crs branch from f6cb9c8 to 8020548 Compare March 13, 2025 11:30

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 13, 2025

sbueringer reviewed Mar 13, 2025

View reviewed changes

controllers/alias.go Outdated Show resolved Hide resolved

mcbenjemaa force-pushed the crs branch from 7b21eff to 0316f57 Compare March 13, 2025 13:05

mcbenjemaa force-pushed the crs branch from 0316f57 to 912fd81 Compare March 13, 2025 13:30

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

k8s-ci-robot assigned sbueringer Mar 13, 2025

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

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

k8s-ci-robot merged commit 01bc3b5 into kubernetes-sigs:main Mar 13, 2025
19 of 20 checks passed

k8s-ci-robot added this to the v1.10 milestone Mar 13, 2025

This was referenced Mar 13, 2025

Feature: ClusterResourceSet #11114

Open

🌱 Cleanup after CRS move #11968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Integrate CRS code into regular code structure #11943

✨ Integrate CRS code into regular code structure #11943

mcbenjemaa commented Mar 8, 2025

mcbenjemaa commented Mar 11, 2025

sbueringer commented Mar 11, 2025

sbueringer commented Mar 11, 2025 •

edited

Loading

mcbenjemaa commented Mar 12, 2025

sbueringer commented Mar 12, 2025 •

edited

Loading

mcbenjemaa commented Mar 12, 2025

sbueringer commented Mar 12, 2025

mcbenjemaa commented Mar 12, 2025

mcbenjemaa commented Mar 12, 2025

mcbenjemaa commented Mar 12, 2025

sbueringer commented Mar 12, 2025

sbueringer left a comment

sbueringer commented Mar 12, 2025 •

edited

Loading

mcbenjemaa commented Mar 13, 2025

sbueringer commented Mar 13, 2025

sbueringer commented Mar 13, 2025

sbueringer commented Mar 13, 2025 •

edited

Loading

k8s-ci-robot commented Mar 13, 2025

mcbenjemaa commented Mar 13, 2025

sbueringer commented Mar 13, 2025

sbueringer commented Mar 13, 2025

k8s-ci-robot commented Mar 13, 2025

k8s-ci-robot commented Mar 13, 2025

sbueringer commented Mar 13, 2025

✨ Integrate CRS code into regular code structure #11943

✨ Integrate CRS code into regular code structure #11943

Conversation

mcbenjemaa commented Mar 8, 2025

mcbenjemaa commented Mar 11, 2025

sbueringer commented Mar 11, 2025

sbueringer commented Mar 11, 2025 • edited Loading

mcbenjemaa commented Mar 12, 2025

sbueringer commented Mar 12, 2025 • edited Loading

mcbenjemaa commented Mar 12, 2025

sbueringer commented Mar 12, 2025

mcbenjemaa commented Mar 12, 2025

mcbenjemaa commented Mar 12, 2025

mcbenjemaa commented Mar 12, 2025

sbueringer commented Mar 12, 2025

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer commented Mar 12, 2025 • edited Loading

mcbenjemaa commented Mar 13, 2025

sbueringer commented Mar 13, 2025

sbueringer commented Mar 13, 2025

sbueringer commented Mar 13, 2025 • edited Loading

k8s-ci-robot commented Mar 13, 2025

mcbenjemaa commented Mar 13, 2025

sbueringer commented Mar 13, 2025

sbueringer commented Mar 13, 2025

k8s-ci-robot commented Mar 13, 2025

k8s-ci-robot commented Mar 13, 2025

sbueringer commented Mar 13, 2025

sbueringer commented Mar 11, 2025 •

edited

Loading

sbueringer commented Mar 12, 2025 •

edited

Loading

sbueringer commented Mar 12, 2025 •

edited

Loading

sbueringer commented Mar 13, 2025 •

edited

Loading