feat: remove the config folder and generate rbac and crds with helm templating enabled#2363
Merged
Merged
Conversation
…emplating enabled On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
…d maintained On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
✅ Deploy Preview for ocm-website canceled.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes Kustomize-based manifests and samples, relocates controller-gen outputs to Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI / Task runner
participant ControllerGen as controller-gen
participant BinGen as bin/gen (raw outputs)
participant Scripts as hack/*.sh
participant HelmChart as chart/templates
rect rgba(200,150,100,0.5)
CI->>ControllerGen: run controller-gen (manifests)
ControllerGen-->>BinGen: emit raw CRD & RBAC YAML to bin/gen
end
rect rgba(100,150,200,0.5)
CI->>Scripts: run helm/generate-crds & helm/generate-rbac
Scripts->>BinGen: read raw YAML files
Scripts->>HelmChart: write templated outputs (templates/crd/, templates/rbac/manager-role.yaml)
CI->>HelmChart: run task helm/validate (generate → lint/schema/docs/template → drift check)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
kubernetes/controller/hack/rbac.generate.sh (1)
36-40: Consider validating theyqstep and constraining thesedsubstitution.Two small robustness improvements:
- The pipeline
yq ... | sed ... > "${OUT}"won't fail ifyqerrors out silently (only the last command's exit status is propagated by default withoutpipefailbeing enough on its own here — actuallyset -o pipefaildoes cover this, so this is fine). Ignore this point.- The
sedreplacement is unanchored, so ifOCM_K8S_TOOLKIT_MANAGER_ROLE_NAMEever appeared elsewhere in the source (e.g., in a future comment or label), it would also be rewritten. Anchoring to thename:line makes the intent explicit:🛡️ Optional tightening
-"${YQ}" eval ".metadata.name = \"${NAME_SENTINEL}\"" "${SRC}" \ - | sed "s|${NAME_SENTINEL}|${NAME_TPL}|" > "${OUT}" +"${YQ}" eval ".metadata.name = \"${NAME_SENTINEL}\"" "${SRC}" \ + | sed "s|^\(\s*name:\s*\)${NAME_SENTINEL}\s*\$|\1${NAME_TPL}|" > "${OUT}"Non-blocking; current behavior is correct for the controller-gen output shape.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/controller/hack/rbac.generate.sh` around lines 36 - 40, Ensure the yq transformation succeeded before piping to sed (e.g., run yq and check its exit status or write yq output to a temp file and verify it) so failures from "${YQ}" are caught rather than masked, and tighten the sed substitution so it only replaces the sentinel on the metadata name line (anchor to a line matching optional whitespace + "name:" containing NAME_SENTINEL) instead of a global unanchored replace of NAME_SENTINEL; locate the pipeline using "${YQ}" eval ".metadata.name = \"${NAME_SENTINEL}\"" "${SRC}" | sed ... > "${OUT}" and change it to validate yq success then perform an anchored sed that targets only the metadata name field replacing NAME_SENTINEL with NAME_TPL.kubernetes/controller/Taskfile.yml (1)
193-208: Consider running generators before lint/template inhelm/validate.Currently
helm/lintandhelm/templaterun against the committed (potentially stale) chart templates, andhelm/generate-crds/helm/generate-rbacexecute afterward only to feed thegit diffcheck. If a newly added API type produces an invalid CRD template or the regeneratedmanager-role.yamlbreaks rendering, the failure is only caught on the next validate run after the diff has been committed. Regenerating first makes the same run verify that the fresh output passes both lint and template.♻️ Proposed reordering
helm/validate: desc: "Validate the Helm chart (lint, template, and verify CRDs/RBAC/schema/docs are in sync)" silent: true cmds: - - task: helm/lint - - task: helm/template - - task: helm/schema - - task: helm/docs - task: helm/generate-crds - task: helm/generate-rbac + - task: helm/schema + - task: helm/docs + - task: helm/lint + - task: helm/template - | if ! git diff --quiet chart/templates/crd chart/templates/rbac/manager-role.yaml chart/values.schema.json chart/README.md; then echo "ERROR: CRDs, manager-role, schema or docs are out of sync. Run 'task helm/generate' (and helm/schema / helm/docs as needed) and commit the changes." git diff chart/templates/crd chart/templates/rbac/manager-role.yaml chart/values.schema.json chart/README.md exit 1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/controller/Taskfile.yml` around lines 193 - 208, In the helm/validate Taskfile target, run the generators before linting and templating so the validators operate on freshly generated artifacts: move helm/generate-crds and helm/generate-rbac (and optionally helm/schema/helm/docs if they affect outputs) to execute before helm/lint and helm/template, then keep the existing git diff check against chart/templates/crd, chart/templates/rbac/manager-role.yaml, chart/values.schema.json and chart/README.md to fail when generators produce changes that aren't committed; update the task order in helm/validate accordingly so helm/lint and helm/template run on the generated output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kubernetes/controller/hack/helm.generate.sh`:
- Around line 35-38: The loop that sets plural and group using YQ (variables
plural, group) can produce empty or "null" values leading to bad filenames;
after calling plural=$("${YQ}" e '.spec.names.plural' "${src}") and
group=$("${YQ}" e '.spec.group' "${src}"), validate both variables (e.g., check
for empty string or the literal "null") and either skip that source file or fail
fast with a clear error; update the loop that writes out
out="${TARGET_DIR}/${plural}.${group}.yaml" to only run when both plural and
group are non-empty and not "null", and log which src was skipped/failing so the
chart generation isn’t silently producing malformed filenames.
---
Nitpick comments:
In `@kubernetes/controller/hack/rbac.generate.sh`:
- Around line 36-40: Ensure the yq transformation succeeded before piping to sed
(e.g., run yq and check its exit status or write yq output to a temp file and
verify it) so failures from "${YQ}" are caught rather than masked, and tighten
the sed substitution so it only replaces the sentinel on the metadata name line
(anchor to a line matching optional whitespace + "name:" containing
NAME_SENTINEL) instead of a global unanchored replace of NAME_SENTINEL; locate
the pipeline using "${YQ}" eval ".metadata.name = \"${NAME_SENTINEL}\"" "${SRC}"
| sed ... > "${OUT}" and change it to validate yq success then perform an
anchored sed that targets only the metadata name field replacing NAME_SENTINEL
with NAME_TPL.
In `@kubernetes/controller/Taskfile.yml`:
- Around line 193-208: In the helm/validate Taskfile target, run the generators
before linting and templating so the validators operate on freshly generated
artifacts: move helm/generate-crds and helm/generate-rbac (and optionally
helm/schema/helm/docs if they affect outputs) to execute before helm/lint and
helm/template, then keep the existing git diff check against
chart/templates/crd, chart/templates/rbac/manager-role.yaml,
chart/values.schema.json and chart/README.md to fail when generators produce
changes that aren't committed; update the task order in helm/validate
accordingly so helm/lint and helm/template run on the generated output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77456240-1a13-4c29-b598-f7a4a1de8e02
📒 Files selected for processing (48)
.github/workflows/kubernetes-controller.ymlkubernetes/controller/.envkubernetes/controller/Dockerfilekubernetes/controller/Makefilekubernetes/controller/PROJECTkubernetes/controller/Taskfile.ymlkubernetes/controller/Tiltfilekubernetes/controller/chart/README.mdkubernetes/controller/chart/README.md.gotmplkubernetes/controller/chart/templates/crd/components.delivery.ocm.software.yamlkubernetes/controller/chart/templates/crd/deployers.delivery.ocm.software.yamlkubernetes/controller/chart/templates/crd/repositories.delivery.ocm.software.yamlkubernetes/controller/chart/templates/crd/resources.delivery.ocm.software.yamlkubernetes/controller/chart/templates/rbac/manager-role.yamlkubernetes/controller/config/crd/bases/delivery.ocm.software_components.yamlkubernetes/controller/config/crd/bases/delivery.ocm.software_deployers.yamlkubernetes/controller/config/crd/bases/delivery.ocm.software_repositories.yamlkubernetes/controller/config/crd/bases/delivery.ocm.software_resources.yamlkubernetes/controller/config/crd/kustomization.yamlkubernetes/controller/config/crd/kustomizeconfig.yamlkubernetes/controller/config/default/kustomization.yamlkubernetes/controller/config/rbac/component_editor_role.yamlkubernetes/controller/config/rbac/component_viewer_role.yamlkubernetes/controller/config/rbac/deployer_editor_role.yamlkubernetes/controller/config/rbac/deployer_viewer_role.yamlkubernetes/controller/config/rbac/kustomization.yamlkubernetes/controller/config/rbac/leader_election_role.yamlkubernetes/controller/config/rbac/leader_election_role_binding.yamlkubernetes/controller/config/rbac/replication_editor_role.yamlkubernetes/controller/config/rbac/replication_viewer_role.yamlkubernetes/controller/config/rbac/repository_editor_role.yamlkubernetes/controller/config/rbac/repository_viewer_role.yamlkubernetes/controller/config/rbac/resource_editor_role.yamlkubernetes/controller/config/rbac/resource_viewer_role.yamlkubernetes/controller/config/rbac/role.yamlkubernetes/controller/config/rbac/role_binding.yamlkubernetes/controller/config/rbac/service_account.yamlkubernetes/controller/config/samples/components.delivery.ocm.software_sample.yamlkubernetes/controller/config/samples/delivery_v1alpha1_component.yamlkubernetes/controller/config/samples/delivery_v1alpha1_ocmdeployer.yamlkubernetes/controller/config/samples/delivery_v1alpha1_ocmrepository.yamlkubernetes/controller/config/samples/delivery_v1alpha1_resource.yamlkubernetes/controller/config/samples/kustomization.yamlkubernetes/controller/config/samples/repository.delivery.ocm.software_sample.yamlkubernetes/controller/config/samples/resource.delivery.ocm.software_sample.yamlkubernetes/controller/hack/helm.generate.shkubernetes/controller/hack/rbac.generate.shkubernetes/controller/tilt.dockerfile
💤 Files with no reviewable changes (36)
- kubernetes/controller/.env
- kubernetes/controller/tilt.dockerfile
- kubernetes/controller/config/samples/kustomization.yaml
- kubernetes/controller/PROJECT
- kubernetes/controller/config/samples/delivery_v1alpha1_ocmrepository.yaml
- kubernetes/controller/config/rbac/deployer_viewer_role.yaml
- kubernetes/controller/config/rbac/leader_election_role.yaml
- kubernetes/controller/config/crd/kustomization.yaml
- kubernetes/controller/config/rbac/service_account.yaml
- kubernetes/controller/config/rbac/replication_viewer_role.yaml
- kubernetes/controller/config/default/kustomization.yaml
- kubernetes/controller/config/rbac/repository_viewer_role.yaml
- kubernetes/controller/config/samples/delivery_v1alpha1_ocmdeployer.yaml
- kubernetes/controller/Makefile
- kubernetes/controller/config/samples/delivery_v1alpha1_component.yaml
- kubernetes/controller/config/rbac/role_binding.yaml
- kubernetes/controller/config/rbac/resource_viewer_role.yaml
- kubernetes/controller/config/rbac/component_editor_role.yaml
- kubernetes/controller/config/samples/repository.delivery.ocm.software_sample.yaml
- kubernetes/controller/config/rbac/component_viewer_role.yaml
- kubernetes/controller/config/rbac/resource_editor_role.yaml
- kubernetes/controller/config/rbac/leader_election_role_binding.yaml
- kubernetes/controller/config/samples/resource.delivery.ocm.software_sample.yaml
- kubernetes/controller/config/rbac/deployer_editor_role.yaml
- kubernetes/controller/config/crd/kustomizeconfig.yaml
- kubernetes/controller/Tiltfile
- kubernetes/controller/config/samples/delivery_v1alpha1_resource.yaml
- kubernetes/controller/config/samples/components.delivery.ocm.software_sample.yaml
- kubernetes/controller/config/rbac/repository_editor_role.yaml
- kubernetes/controller/config/crd/bases/delivery.ocm.software_resources.yaml
- kubernetes/controller/config/crd/bases/delivery.ocm.software_components.yaml
- kubernetes/controller/config/rbac/replication_editor_role.yaml
- kubernetes/controller/config/crd/bases/delivery.ocm.software_repositories.yaml
- kubernetes/controller/config/crd/bases/delivery.ocm.software_deployers.yaml
- kubernetes/controller/config/rbac/role.yaml
- kubernetes/controller/config/rbac/kustomization.yaml
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
kubernetes/controller/Taskfile.yml (1)
172-176: Optional: collapse duplication inhelm/validate.
helm/validate(lines 197–198) invokeshelm/generate-crdsandhelm/generate-rbacseparately, duplicating the composition thathelm/generatealready does. You could just calltask: helm/generatethere to keep a single source of truth for the generation sequence. Purely cosmetic; feel free to ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kubernetes/controller/Taskfile.yml` around lines 172 - 176, The helm/validate target duplicates the composition already defined by helm/generate (which runs helm/generate-crds and helm/generate-rbac); update the helm/validate task to call the helm/generate task instead of invoking helm/generate-crds and helm/generate-rbac directly so the generation sequence is maintained in one place (edit the helm/validate entry to replace the two task entries with a single "task: helm/generate").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kubernetes/controller/Taskfile.yml`:
- Around line 203-208: The helm/validate step in Taskfile.yml currently uses
"git diff --quiet" which misses newly created untracked files; replace that
check with a git status --porcelain invocation against the same paths
(chart/templates/crd, chart/templates/rbac/manager-role.yaml,
chart/values.schema.json, chart/README.md) so both modified and untracked files
are detected, and update the conditional that calls echo/git diff/exit to run
when git status --porcelain returns any output; locate the block around the if !
git diff --quiet ... fi and change it to run git status --porcelain -- <paths>
and fail when the output is non-empty.
---
Nitpick comments:
In `@kubernetes/controller/Taskfile.yml`:
- Around line 172-176: The helm/validate target duplicates the composition
already defined by helm/generate (which runs helm/generate-crds and
helm/generate-rbac); update the helm/validate task to call the helm/generate
task instead of invoking helm/generate-crds and helm/generate-rbac directly so
the generation sequence is maintained in one place (edit the helm/validate entry
to replace the two task entries with a single "task: helm/generate").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ce254b47-a62d-4c47-8e19-1b33b6b9fba2
📒 Files selected for processing (7)
.github/workflows/kubernetes-controller.ymlkubernetes/controller/Taskfile.ymlkubernetes/controller/hack/helm.generate.shkubernetes/controller/internal/controller/component/suite_test.gokubernetes/controller/internal/controller/deployer/suite_test.gokubernetes/controller/internal/controller/repository/suite_test.gokubernetes/controller/internal/controller/resource/suite_test.go
✅ Files skipped from review due to trivial changes (1)
- kubernetes/controller/internal/controller/resource/suite_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/kubernetes-controller.yml
- kubernetes/controller/hack/helm.generate.sh
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
…nt-model into remove-kustomize-v2
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kubernetes/controller/Taskfile.yml`:
- Around line 43-47: The controller-gen invocation in the Taskfile currently
includes the webhook generator but only redirects CRD and RBAC outputs to
bin/gen; update that command in Taskfile.yml (the line containing
"{{.CONTROLLER_GEN}} rbac:roleName=manager-role crd webhook ...") to either
remove the webhook generator entirely or redirect its output to bin/gen by
adding an artifacts redirect (e.g.
output:webhook:artifacts:config=bin/gen/webhook) so webhook manifests aren’t
written to config/webhook after config/ is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 61495af2-cf46-46bc-9364-e00619222865
📒 Files selected for processing (1)
kubernetes/controller/Taskfile.yml
Contributor
|
e2e tests etc work fine - just a few comments and a gif :D |
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
…nt-model into remove-kustomize-v2
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kubernetes/controller/Taskfile.yml`:
- Around line 43-47: The task currently creates bin/gen directories but doesn't
remove stale files; update the Taskfile task cmds (replace or augment the
existing mkdir -p bin/gen/crd bin/gen/rbac command and the controller-gen
invocation) to first remove old generated artifacts (e.g., rm -rf bin/gen/* or
rm -rf bin/gen/crd/* bin/gen/rbac/*) and then recreate directories before
running the controller-gen command so stale CRD/RBAC YAML cannot persist between
runs; ensure the exact cmd string containing "{{.CONTROLLER_GEN}} ...
output:rbac:artifacts:config=bin/gen/rbac
output:crd:artifacts:config=bin/gen/crd" remains unchanged except for the
directory-clean step inserted before it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a368751a-fb27-4bd3-9295-6fe467b73094
📒 Files selected for processing (1)
kubernetes/controller/Taskfile.yml
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com> Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
matthiasbruns
approved these changes
Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Let's see what changed.
Kustomize is removed completely. The config folder and sync is no longer there either.
There are two main scripts that have been added.
helm.generate.sh- this will create the necessary annotations and the webhook and the conversion settingsrbac.generate.sh- this templates the RBAC file so kubebuilder annotation changes are still applied as you'd expectNew task targets have been updated to run the updates and the generations which are
helm/generate-crdsandhelm/generate-rbac. These will run generators and validations. These have been added to the relevant release flows.Marker changes will work and RBAC will be kept updated running the right tasks. Helm validation now detects stale CRDs and linting and schema and docs are all kept as well.
Further, Tilt has been removed as it's unused.
I checked most of the controllers that use and deal with generations and crds including ESO and found that these flows are kinda uniform in one way or another. Which is basically, let controller-gen generate its stuff and then post edit the outcome into the bundled CRDs. What we differ in is that some projects are manually keeping RBAC up-to-date where as I opted to still rely on kubebuilder markers because I didn't want our users to keep forgetting to add RBAC permissions for new objects.
Which issue(s) this PR fixes
Fixes open-component-model/ocm-project#1030
Testing
How to test the changes
Verification
task testandtask test/integrationif applicable)go workis enabled (seego.work)ocm