Skip to content

fix: use unused values from the helm values section#2376

Merged
Skarlso merged 2 commits into
open-component-model:mainfrom
Skarlso:unused-helm-values
Apr 23, 2026
Merged

fix: use unused values from the helm values section#2376
Skarlso merged 2 commits into
open-component-model:mainfrom
Skarlso:unused-helm-values

Conversation

@Skarlso

@Skarlso Skarlso commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

What this PR does / why we need it

Notices during an investigation on the attached issue that we have a couple of values in values.yaml that aren't using in the deployment template.

Which issue(s) this PR fixes

Part of open-component-model/service-provider-ocm#95.

Testing

How to test the changes
Verification
  • I have added/updated tests for my changes (see Test Requirements)
  • Tests pass locally (task test and task test/integration if applicable)
  • If touching multiple modules, go work is enabled (see go.work)
  • My changes do not decrease test coverage
  • I have tested the changes locally by running ocm

@Skarlso Skarlso requested a review from a team as a code owner April 23, 2026 11:13
@netlify

netlify Bot commented Apr 23, 2026 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for ocm-website canceled.

Name Link
🔨 Latest commit cf71b7d
🔍 Latest deploy log https://app.netlify.com/projects/ocm-website/deploys/69ea02fd16867c00083b5a35

@github-actions github-actions Bot added the kind/bugfix Bug label Apr 23, 2026
@coderabbitai

coderabbitai Bot commented Apr 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@Skarlso has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 42 minutes and 9 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 42 minutes and 9 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6459e9d2-11bd-46cb-888b-1d21217996b1

📥 Commits

Reviewing files that changed from the base of the PR and between e21802d and cf71b7d.

📒 Files selected for processing (9)
  • kubernetes/controller/chart/README.md
  • kubernetes/controller/chart/templates/crd/components.delivery.ocm.software.yaml
  • kubernetes/controller/chart/templates/crd/deployers.delivery.ocm.software.yaml
  • kubernetes/controller/chart/templates/crd/repositories.delivery.ocm.software.yaml
  • kubernetes/controller/chart/templates/crd/resources.delivery.ocm.software.yaml
  • kubernetes/controller/chart/templates/manager/manager.yaml
  • kubernetes/controller/chart/values.schema.json
  • kubernetes/controller/chart/values.yaml
  • kubernetes/controller/hack/helm.generate.sh
📝 Walkthrough

Walkthrough

This pull request adds conditional Helm annotations to four CRD templates to control resource lifecycle management, makes the controller-manager deployment configurable with replicas and environment variables, and removes unused top-level metrics configuration from the chart values schema.

Changes

Cohort / File(s) Summary
CRD Templates with Helm Resource Policy
kubernetes/controller/chart/templates/crd/components.delivery.ocm.software.yaml, kubernetes/controller/chart/templates/crd/deployers.delivery.ocm.software.yaml, kubernetes/controller/chart/templates/crd/repositories.delivery.ocm.software.yaml, kubernetes/controller/chart/templates/crd/resources.delivery.ocm.software.yaml		 Each CRD template now conditionally injects the helm.sh/resource-policy: keep annotation in metadata when .Values.crd.keep is enabled, preventing Helm from deleting CRDs during release operations.
Manager Deployment Configuration
kubernetes/controller/chart/templates/manager/manager.yaml		 Makes controller-manager replica count configurable via manager.replicas, conditionally includes imagePullSecrets from manager.imagePullSecrets, and optionally injects environment variables through manager.env.
Chart Schema and Values
kubernetes/controller/chart/values.schema.json, kubernetes/controller/chart/values.yaml		 Removes top-level metrics object definition (including enable and port properties) from both the JSON schema and values file, eliminating unused metrics configuration surface from the chart.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

kind/chore, size/m

Suggested reviewers

  • jakobmoellerdev
  • morri-son

Poem

🐰 A rabbit hops through templates with glee,
Adding keep to CRDs so they'll forever be free,
Replicas now dance at the chart's command,
Stray metrics removed by the rabbit's own hand,
Clean configs and order across all the land!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: making previously unused Helm values actually be used in the deployment templates by adding conditional annotations and configuration references.
Description check ✅ Passed The description clearly relates to the changeset, explaining that unused values in values.yaml were identified and are now being used in the deployment template.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the size/s Small label Apr 23, 2026
coderabbitai[bot]
coderabbitai Bot reviewed Apr 23, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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/chart/templates/manager/manager.yaml`:
- Line 13: The template currently allows manager.replicas to be set >1 while
manager.leaderElection.enabled is false; add a Helm render-time guard in the
manager.yaml template that checks .Values.manager.replicas and
.Values.manager.leaderElection.enabled and fails rendering if replicas > 1 and
leaderElection.enabled is false (use Helm's conditional and fail function) so
the chart render aborts with a clear message referencing manager.replicas and
manager.leaderElection.enabled; alternatively enforce replicas=1 when leader
election is disabled by overriding the replicas value in the same template.
🪄 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: fa548c98-d10b-49b0-a51e-049b99a9ca35

📥 Commits

Reviewing files that changed from the base of the PR and between dd111f3 and e21802d.

📒 Files selected for processing (7)
  • kubernetes/controller/chart/templates/crd/components.delivery.ocm.software.yaml
  • kubernetes/controller/chart/templates/crd/deployers.delivery.ocm.software.yaml
  • kubernetes/controller/chart/templates/crd/repositories.delivery.ocm.software.yaml
  • kubernetes/controller/chart/templates/crd/resources.delivery.ocm.software.yaml
  • kubernetes/controller/chart/templates/manager/manager.yaml
  • kubernetes/controller/chart/values.schema.json
  • kubernetes/controller/chart/values.yaml
💤 Files with no reviewable changes (2)
  • kubernetes/controller/chart/values.yaml
  • kubernetes/controller/chart/values.schema.json

Comment thread kubernetes/controller/chart/templates/manager/manager.yaml
@Skarlso
fix: use unused values from the helm values section
ae2f04a 
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com>

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
@Skarlso Skarlso force-pushed the unused-helm-values branch from e21802d to ae2f04a Compare April 23, 2026 11:27
@Skarlso
addressed coderabbitai comment
cf71b7d 
On-behalf-of: Gergely Brautigam <gergely.brautigam@sap.com>

Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
@Skarlso Skarlso merged commit 7e91841 into open-component-model:main Apr 23, 2026
32 checks passed
@frewilhelm frewilhelm mentioned this pull request Apr 29, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@piotrjanik piotrjanik piotrjanik approved these changes

@matthiasbruns matthiasbruns matthiasbruns approved these changes

Assignees

No one assigned

Labels

kind/bugfix Bug size/s Small

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Skarlso @piotrjanik @matthiasbruns