Skip to content

Conversation

@KJone1
Copy link
Contributor

@KJone1 KJone1 commented Nov 9, 2025

closes: #1112

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 9, 2025

Walkthrough

The changes introduce a dedicated clusterName configuration field to the Holmes Helm chart that automatically injects the CLUSTER_NAME environment variable into the container when defined, eliminating the need for boilerplate configuration through additionalEnvVars.

Changes

Cohort / File(s) Summary
Helm Configuration
helm/holmes/values.yaml
Added new top-level configuration field clusterName: "" to enable optional cluster identification parameter.
Helm Template
helm/holmes/templates/holmes.yaml
Added conditional environment variable block that injects CLUSTER_NAME env var from .Values.clusterName when the value is defined.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Simple addition of a new optional configuration parameter following existing patterns
  • Straightforward conditional templating logic
  • Limited scope affecting only two files with homogeneous changes

Suggested reviewers

  • moshemorad

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly describes the main change: adding a dedicated clusterName Helm value to the holmes chart.
Description check ✅ Passed The PR description references the linked issue #1112, which relates to the changeset's objective of adding a clusterName Helm value.
Linked Issues check ✅ Passed The PR successfully implements the requirement from issue #1112 by adding a dedicated clusterName field that automatically injects CLUSTER_NAME environment variable.
Out of Scope Changes check ✅ Passed All changes are directly related to the objective of adding a clusterName Helm value; no out-of-scope modifications detected.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
helm/holmes/templates/holmes.yaml (1)

53-56: Consider quoting the value for defensive robustness.

While consistent with existing patterns in this template (e.g., logLevel, sentryDSN), applying the | quote filter would make the injection more resilient to user input containing special YAML characters:

-          - name: CLUSTER_NAME
-            value: {{ .Values.clusterName }}
+          - name: CLUSTER_NAME
+            value: {{ .Values.clusterName | quote }}

This is optional, but recommended as a follow-up if cluster names might ever contain special characters (spaces, colons, etc.).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a3672b6 and 4d80fb9.

📒 Files selected for processing (2)
  • helm/holmes/templates/holmes.yaml (1 hunks)
  • helm/holmes/values.yaml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Pre-commit checks
🔇 Additional comments (2)
helm/holmes/values.yaml (1)

3-3: LGTM! Appropriate placement and default value for optional cluster name configuration.

The empty string default ensures the feature is opt-in and aligns with the PR objective to eliminate repeated additionalEnvVars boilerplate across deployments.

helm/holmes/templates/holmes.yaml (1)

53-56: LGTM! Template implementation correctly consumes the new clusterName value with proper conditional rendering.

The whitespace control (- operators), indentation, and conditional syntax align with existing optional environment variable patterns in the template. The env var is properly excluded when clusterName is not defined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

helm: Add dedicated clusterName value to streamline cluster identification

1 participant