feat(helm): add commonLabels support to propagate custom labels across all chart resources - #1929
Merged
Merged
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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:
WalkthroughAdds two Helm helpers ( Changes
Sequence Diagram(s)(Skipped — changes are metadata injections and merges; no multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
✅ Deploy Preview for holmes-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
helm/holmes/values.yaml (1)
8-8: Consider documentingcommonLabelswith an example.A short comment with a usage example (and a note that Kubernetes label key/value constraints apply — keys ≤63 chars, values must match
[a-z0-9A-Z]([-a-z0-9A-Z_.]*[a-z0-9A-Z])?) will help users avoid template-render-success / apply-time-failure surprises.Suggested docs
-commonLabels: {} +# Labels applied to every Kubernetes object created by this chart +# (Deployments, Services, ServiceAccounts, RBAC, HPAs, ConfigMaps, MCP resources, etc.). +# Must conform to Kubernetes label syntax (keys ≤63 chars; values ≤63 chars, alphanumerics/._-). +# Example: +# commonLabels: +# team: platform +# app.kubernetes.io/part-of: holmesgpt +commonLabels: {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/values.yaml` at line 8, Add an inline comment above the commonLabels key in values.yaml documenting its purpose and usage: show a brief example map (e.g., app: my-app, env: staging) and note Kubernetes label constraints (key length ≤63 chars and values must match the regex [a-z0-9A-Z]([-a-z0-9A-Z_.]*[a-z0-9A-Z])?) so users understand keys/values must follow K8s label rules and avoid template-success/apply-time-failure issues; update the comment next to the commonLabels entry to include that guidance and the example.helm/holmes/templates/toolset-config.yaml (1)
7-10: Use theholmes.commonLabelshelper here for consistency.The whole point of the new helper in
_helpers.tplis to centralize this block. Other templates in this PR (e.g.,operator-deployment.yaml,hpa.yaml, the MCPnetworkpolicy.yamlfiles) use the helper; this file inlines the same logic. If the helper's rendering ever changes (e.g., adding chart-default labels), this site will silently drift.♻️ Suggested refactor
- {{- with .Values.commonLabels }} labels: - {{- toYaml . | nindent 4 }} - {{- end }} + {{- include "holmes.commonLabels" . | nindent 4 }}Note: unlike the inlined
withform, the helper call always emits thelabels:key. That's fine and arguably better — an emptylabels:map is valid and keeps manifests stable across values. If you want to preserve the "omitlabels:entirely when unset" behavior, wrap the include:{{- with .Values.commonLabels }} labels: {{- toYaml . | nindent 4 }} {{- end }}(i.e., keep as-is). Either way, align with the pattern used in the rest of the chart.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/templates/toolset-config.yaml` around lines 7 - 10, Replace the inline label block that uses .Values.commonLabels with the chart helper holmes.commonLabels to centralize label rendering: remove the {{- with .Values.commonLabels }} ... labels: ... {{- end }} block and call the helper include for holmes.commonLabels (which emits the labels: key and body); if you need to preserve the previous behavior of omitting the labels key when none are set, wrap the include in a conditional that checks .Values.commonLabels before calling the helper.helm/holmes/templates/holmesgpt-service-account.yaml (1)
7-10: Replace inlinewith .Values.commonLabelsblocks with theholmes.commonLabelshelper.All four label blocks in this file (ClusterRole L7-10, ServiceAccount L419-422, ClusterRoleBinding L438-441, OpenShift monitoring ClusterRoleBinding L456-459) duplicate the logic that was just centralized in
_helpers.tpl. Prefer the helper so any future changes (e.g., adding chart-default labels) only need to happen in one place.♻️ Suggested refactor (apply to each site)
Option A — preserve "omit
labels:when empty" (current behavior):- {{- with .Values.commonLabels }} labels: - {{- toYaml . | nindent 4 }} - {{- end }} + {{- include "holmes.commonLabels" . | nindent 4 }}(Wrap the
labels:line in{{- with .Values.commonLabels }} ... {{- end }}if you want to keep omitting the key entirely when unset.)Option B — simpler, always emits
labels:(valid even when empty):- {{- with .Values.commonLabels }} - labels: - {{- toYaml . | nindent 4 }} - {{- end }} + labels: + {{- include "holmes.commonLabels" . | nindent 4 }}Also worth double-checking: the
ClusterRoleBindingat L438 currently emits alabels:key only whencommonLabelsis set, which is fine — just make sure the chosen pattern is applied uniformly across all chart templates.Also applies to: 419-422, 438-441, 456-459
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/templates/holmesgpt-service-account.yaml` around lines 7 - 10, The four inline label blocks in holmesgpt-service-account.yaml (the ClusterRole, ServiceAccount, ClusterRoleBinding, and OpenShift monitoring ClusterRoleBinding label sections) duplicate logic centralized in the holmes.commonLabels helper; replace each "{{- with .Values.commonLabels }} ... {{- end }}" block with a call to the holmes.commonLabels helper (or wrap the helper with a surrounding with if you want to preserve omitting the "labels:" key when empty), ensuring you update the label rendering for the ClusterRole, ServiceAccount, ClusterRoleBinding, and OpenShift monitoring ClusterRoleBinding sections to use holmes.commonLabels uniformly.helm/holmes/templates/mcp-servers/sentry/deployment.yaml (1)
14-14: Consider guarding against duplicate label keys.If a user sets a key in
commonLabelsthat collides with one of the statically-rendered labels above (e.g.app.kubernetes.io/name,app.kubernetes.io/instance,app.kubernetes.io/component,app.kubernetes.io/part-of, orapp), the rendered manifest will contain duplicate YAML keys. Helm/kubectlmay reject it (strict YAML: "mapping key already defined") or silently keep the last occurrence, which is surprising.Two common mitigations — either is fine, pick one and apply consistently in the
holmes.commonLabelshelper (not in each template):
- Merge via
mustMerge/mergeso user-provided keys override chart-managed keys deterministically, and render the merged map in one block.- Document in
values.yamlthatcommonLabelsmust not reuse the reservedapp.kubernetes.io/*andappkeys used by this chart.This applies equally to every sibling template touched in this PR (prefect, mariadb, aws, github, azure, kubernetes-remediation, gcp, plus the operator/HPA/RBAC/NetworkPolicy files not shown here).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/templates/mcp-servers/sentry/deployment.yaml` at line 14, The rendered templates risk duplicate YAML keys when users supply labels via the holmes.commonLabels helper that collide with the chart's static labels (app.kubernetes.io/name, instance, component, part-of, or app); update the holmes.commonLabels helper to merge maps deterministically (using merge or mustMerge) so the chart-managed static labels are merged with user-provided labels into a single map (choose whether user labels override or chart labels win and apply consistently), then render that single merged map in templates (e.g., deployment.yaml) instead of including holmes.commonLabels separately; alternatively, if you prefer the other mitigation, add a clear note in values.yaml documenting that users must not set the reserved app.kubernetes.io/* and app keys — but pick one approach and apply it in the holmes.commonLabels helper across all sibling templates.helm/holmes/templates/mcp-servers/azure/deployment.yaml (1)
33-70: Verify label ordering beside the workload-identity label.On line 33 and line 70,
holmes.commonLabelsis injected immediately before the conditionalazure.workload.identity/use: "true"label. Rendering order is fine as-is, but if a user happens to setazure.workload.identity/useincommonLabels, the static"true"line below will override the user's value (because it appears last in the YAML document). Minor — likely acceptable given this label is Azure-managed, but worth calling out alongside the broader collision caveat.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/templates/mcp-servers/azure/deployment.yaml` around lines 33 - 70, The injected static label azure.workload.identity/use: "true" currently appears after the included holmes.commonLabels, so a user-provided azure.workload.identity/use in commonLabels gets overridden; to fix, move the conditional block that renders azure.workload.identity/use so it appears before the {{- include "holmes.commonLabels" . }} invocation (both where labels are built near the top and inside the pod template), ensuring user-supplied values in holmes.commonLabels take precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/holmes/templates/holmes.yaml`:
- Line 21: The holmes.commonLabels helper currently emits user-supplied labels
verbatim which can override reserved keys (e.g., app, app.kubernetes.io/*,
helm.sh/chart, release, heritage) and break Deployment selector matching; update
the define "holmes.commonLabels" in helm/holmes/templates/_helpers.tpl to
validate .Values.commonLabels by iterating keys (use range and hasPrefix where
appropriate) and if any reserved key is present call the Helm fail function with
a clear message rejecting those keys, otherwise render the safe toYaml output;
keep the include "holmes.commonLabels" usage in
helm/holmes/templates/holmes.yaml unchanged so invalid user labels are rejected
at template render time.
---
Nitpick comments:
In `@helm/holmes/templates/holmesgpt-service-account.yaml`:
- Around line 7-10: The four inline label blocks in
holmesgpt-service-account.yaml (the ClusterRole, ServiceAccount,
ClusterRoleBinding, and OpenShift monitoring ClusterRoleBinding label sections)
duplicate logic centralized in the holmes.commonLabels helper; replace each "{{-
with .Values.commonLabels }} ... {{- end }}" block with a call to the
holmes.commonLabels helper (or wrap the helper with a surrounding with if you
want to preserve omitting the "labels:" key when empty), ensuring you update the
label rendering for the ClusterRole, ServiceAccount, ClusterRoleBinding, and
OpenShift monitoring ClusterRoleBinding sections to use holmes.commonLabels
uniformly.
In `@helm/holmes/templates/mcp-servers/azure/deployment.yaml`:
- Around line 33-70: The injected static label azure.workload.identity/use:
"true" currently appears after the included holmes.commonLabels, so a
user-provided azure.workload.identity/use in commonLabels gets overridden; to
fix, move the conditional block that renders azure.workload.identity/use so it
appears before the {{- include "holmes.commonLabels" . }} invocation (both where
labels are built near the top and inside the pod template), ensuring
user-supplied values in holmes.commonLabels take precedence.
In `@helm/holmes/templates/mcp-servers/sentry/deployment.yaml`:
- Line 14: The rendered templates risk duplicate YAML keys when users supply
labels via the holmes.commonLabels helper that collide with the chart's static
labels (app.kubernetes.io/name, instance, component, part-of, or app); update
the holmes.commonLabels helper to merge maps deterministically (using merge or
mustMerge) so the chart-managed static labels are merged with user-provided
labels into a single map (choose whether user labels override or chart labels
win and apply consistently), then render that single merged map in templates
(e.g., deployment.yaml) instead of including holmes.commonLabels separately;
alternatively, if you prefer the other mitigation, add a clear note in
values.yaml documenting that users must not set the reserved app.kubernetes.io/*
and app keys — but pick one approach and apply it in the holmes.commonLabels
helper across all sibling templates.
In `@helm/holmes/templates/toolset-config.yaml`:
- Around line 7-10: Replace the inline label block that uses
.Values.commonLabels with the chart helper holmes.commonLabels to centralize
label rendering: remove the {{- with .Values.commonLabels }} ... labels: ... {{-
end }} block and call the helper include for holmes.commonLabels (which emits
the labels: key and body); if you need to preserve the previous behavior of
omitting the labels key when none are set, wrap the include in a conditional
that checks .Values.commonLabels before calling the helper.
In `@helm/holmes/values.yaml`:
- Line 8: Add an inline comment above the commonLabels key in values.yaml
documenting its purpose and usage: show a brief example map (e.g., app: my-app,
env: staging) and note Kubernetes label constraints (key length ≤63 chars and
values must match the regex [a-z0-9A-Z]([-a-z0-9A-Z_.]*[a-z0-9A-Z])?) so users
understand keys/values must follow K8s label rules and avoid
template-success/apply-time-failure issues; update the comment next to the
commonLabels entry to include that guidance and the example.
🪄 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: f4c20202-0828-46ef-9a17-ad915b7e53bc
📥 Commits
Reviewing files that changed from the base of the PR and between b24a252 and cce822b553e94bef8fdbe73e1bc03ef5975dbfc7.
📒 Files selected for processing (28)
helm/holmes/templates/_helpers.tplhelm/holmes/templates/holmes.yamlhelm/holmes/templates/holmesgpt-service-account.yamlhelm/holmes/templates/hpa.yamlhelm/holmes/templates/mcp-servers/aws/deployment.yamlhelm/holmes/templates/mcp-servers/aws/networkpolicy.yamlhelm/holmes/templates/mcp-servers/azure/deployment.yamlhelm/holmes/templates/mcp-servers/azure/networkpolicy.yamlhelm/holmes/templates/mcp-servers/confluence-mcp/deployment.yamlhelm/holmes/templates/mcp-servers/confluence-mcp/networkpolicy.yamlhelm/holmes/templates/mcp-servers/gcp/deployment.yamlhelm/holmes/templates/mcp-servers/gcp/networkpolicy.yamlhelm/holmes/templates/mcp-servers/github/deployment.yamlhelm/holmes/templates/mcp-servers/github/networkpolicy.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/networkpolicy.yamlhelm/holmes/templates/mcp-servers/mariadb/deployment.yamlhelm/holmes/templates/mcp-servers/mariadb/networkpolicy.yamlhelm/holmes/templates/mcp-servers/prefect/deployment.yamlhelm/holmes/templates/mcp-servers/prefect/networkpolicy.yamlhelm/holmes/templates/mcp-servers/sentry/deployment.yamlhelm/holmes/templates/mcp-servers/sentry/networkpolicy.yamlhelm/holmes/templates/operator-deployment.yamlhelm/holmes/templates/operator-rbac.yamlhelm/holmes/templates/toolset-config.yamlhelm/holmes/values.yamlholmes/core/tools.pytests/test_mcp_toolset.py
leegin
force-pushed
the
holmes-helm-labels
branch
from
April 20, 2026 07:07
cce822b to
137abec
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
helm/holmes/templates/_helpers.tpl (1)
50-54:⚠️ Potential issue | 🔴 CriticalGuard reserved selector labels before rendering
commonLabels.This helper is injected after chart-owned labels, so values like
commonLabels.apporcommonLabels.app.kubernetes.io/instancecan override pod template labels and break Deployment selector matching. Centralize the guard here so every include gets the same protection. Also normalize values to strings beforetoYamlto avoid unquoted numeric/bool label values.🛡️ Proposed helper hardening
{{- define "holmes.commonLabels" -}} +{{- $reserved := list "app" "app.kubernetes.io/name" "app.kubernetes.io/instance" -}} {{- with .Values.commonLabels }} -{{- toYaml . }} +{{- $labels := dict -}} +{{- range $key, $value := . }} +{{- if has $key $reserved }} +{{- fail (printf "commonLabels contains reserved selector label %q; use a non-chart-owned label key" $key) }} +{{- end }} +{{- $_ := set $labels $key ($value | toString) -}} +{{- end }} +{{- toYaml $labels }} {{- end }} {{- end }}Verification expectation: the first render should fail after the guard is added.
#!/bin/bash set -euo pipefail if ! command -v helm >/dev/null 2>&1; then echo "helm is not installed; install Helm to run this verification." >&2 exit 0 fi if helm template label-guard helm/holmes --set commonLabels.app=custom >/tmp/holmes-label-guard.yaml 2>/tmp/holmes-label-guard.err; then echo "Unexpected success: reserved commonLabels.app was accepted" sed -n '1,80p' /tmp/holmes-label-guard.yaml exit 1 else cat /tmp/holmes-label-guard.err fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/templates/_helpers.tpl` around lines 50 - 54, Update the holmes.commonLabels helper to validate/guard .Values.commonLabels: check for reserved selector keys (e.g., "app", "app.kubernetes.io/instance", "app.kubernetes.io/name", "app.kubernetes.io/managed-by", and other chart-owned selector labels) and abort rendering with a clear fail message if any are present, and ensure values are normalized to strings before calling toYaml so numeric/boolean values are quoted; modify the define "holmes.commonLabels" block (which currently reads .Values.commonLabels and calls toYaml) to perform the reserved-key check and stringify values prior to output.
🧹 Nitpick comments (1)
helm/holmes/values.yaml (1)
8-8: Document the new public value’s constraints.Add a short comment so users know these labels are applied to Kubernetes objects and should not use chart-owned selector keys.
📝 Proposed values comment
podAnnotations: {} +# Labels added to every Kubernetes object rendered by this chart. +# Avoid chart-owned selector labels such as `app`, `app.kubernetes.io/name`, and `app.kubernetes.io/instance`. commonLabels: {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/values.yaml` at line 8, Add a short comment above the new public value commonLabels explaining that these labels are merged onto Kubernetes objects and therefore must not include keys that are owned by the chart (e.g., any selector labels used by Deployments/Services) or other reserved keys; state that users should only add non-selector, non-reserved labels and give a brief example usage to clarify intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/holmes/templates/holmesgpt-service-account.yaml`:
- Around line 7-10: Replace direct rendering of .Values.commonLabels with the
shared helper that validates reserved keys: call the helper holmes.commonLabels
(e.g., use {{ include "holmes.commonLabels" . | nindent 4 }}) wherever you
currently have the labels block that expands .Values.commonLabels. Specifically,
find occurrences of the literal .Values.commonLabels used inside a labels: block
(the block starting with "labels:" and then "{{- toYaml . | nindent 4 }}") and
swap it to call the holmes.commonLabels helper so reserved labels (app,
app.kubernetes.io/*, etc.) are validated and cannot be overridden.
---
Duplicate comments:
In `@helm/holmes/templates/_helpers.tpl`:
- Around line 50-54: Update the holmes.commonLabels helper to validate/guard
.Values.commonLabels: check for reserved selector keys (e.g., "app",
"app.kubernetes.io/instance", "app.kubernetes.io/name",
"app.kubernetes.io/managed-by", and other chart-owned selector labels) and abort
rendering with a clear fail message if any are present, and ensure values are
normalized to strings before calling toYaml so numeric/boolean values are
quoted; modify the define "holmes.commonLabels" block (which currently reads
.Values.commonLabels and calls toYaml) to perform the reserved-key check and
stringify values prior to output.
---
Nitpick comments:
In `@helm/holmes/values.yaml`:
- Line 8: Add a short comment above the new public value commonLabels explaining
that these labels are merged onto Kubernetes objects and therefore must not
include keys that are owned by the chart (e.g., any selector labels used by
Deployments/Services) or other reserved keys; state that users should only add
non-selector, non-reserved labels and give a brief example usage to clarify
intent.
🪄 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: 3a03836f-922a-41eb-ba91-5ea7becf6537
📥 Commits
Reviewing files that changed from the base of the PR and between cce822b553e94bef8fdbe73e1bc03ef5975dbfc7 and 137abecd8c8f554867be4324a4d667679cd60874.
📒 Files selected for processing (26)
helm/holmes/templates/_helpers.tplhelm/holmes/templates/holmes.yamlhelm/holmes/templates/holmesgpt-service-account.yamlhelm/holmes/templates/hpa.yamlhelm/holmes/templates/mcp-servers/aws/deployment.yamlhelm/holmes/templates/mcp-servers/aws/networkpolicy.yamlhelm/holmes/templates/mcp-servers/azure/deployment.yamlhelm/holmes/templates/mcp-servers/azure/networkpolicy.yamlhelm/holmes/templates/mcp-servers/confluence-mcp/deployment.yamlhelm/holmes/templates/mcp-servers/confluence-mcp/networkpolicy.yamlhelm/holmes/templates/mcp-servers/gcp/deployment.yamlhelm/holmes/templates/mcp-servers/gcp/networkpolicy.yamlhelm/holmes/templates/mcp-servers/github/deployment.yamlhelm/holmes/templates/mcp-servers/github/networkpolicy.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/networkpolicy.yamlhelm/holmes/templates/mcp-servers/mariadb/deployment.yamlhelm/holmes/templates/mcp-servers/mariadb/networkpolicy.yamlhelm/holmes/templates/mcp-servers/prefect/deployment.yamlhelm/holmes/templates/mcp-servers/prefect/networkpolicy.yamlhelm/holmes/templates/mcp-servers/sentry/deployment.yamlhelm/holmes/templates/mcp-servers/sentry/networkpolicy.yamlhelm/holmes/templates/operator-deployment.yamlhelm/holmes/templates/operator-rbac.yamlhelm/holmes/templates/toolset-config.yamlhelm/holmes/values.yaml
✅ Files skipped from review due to trivial changes (13)
- helm/holmes/templates/mcp-servers/aws/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/sentry/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/confluence-mcp/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/azure/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/kubernetes-remediation/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/mariadb/networkpolicy.yaml
- helm/holmes/templates/toolset-config.yaml
- helm/holmes/templates/mcp-servers/aws/deployment.yaml
- helm/holmes/templates/mcp-servers/prefect/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/prefect/deployment.yaml
- helm/holmes/templates/hpa.yaml
- helm/holmes/templates/mcp-servers/gcp/deployment.yaml
- helm/holmes/templates/mcp-servers/sentry/deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- helm/holmes/templates/mcp-servers/gcp/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/mariadb/deployment.yaml
- helm/holmes/templates/mcp-servers/github/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/github/deployment.yaml
- helm/holmes/templates/mcp-servers/azure/deployment.yaml
- helm/holmes/templates/mcp-servers/confluence-mcp/deployment.yaml
- helm/holmes/templates/operator-rbac.yaml
- helm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yaml
leegin
force-pushed
the
holmes-helm-labels
branch
from
April 20, 2026 07:16
b984a0b to
5291833
Compare
Signed-off-by: leegin <leegin.t@gmail.com>
Signed-off-by: leegin <leegin.t@gmail.com>
leegin
force-pushed
the
holmes-helm-labels
branch
from
April 20, 2026 08:20
505814b to
38351a0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
helm/holmes/values.yaml (1)
8-8: Document the new user-facing value.A short comment in
values.yamlwould make the reserved-key behavior discoverable without reading_helpers.tpl.Suggested values comment
podAnnotations: {} +# Labels applied to every Kubernetes object rendered by this chart. +# Chart-managed selector labels such as `app` and `app.kubernetes.io/name` are reserved. commonLabels: {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/values.yaml` at line 8, Add a short user-facing comment above the commonLabels entry in values.yaml explaining that commonLabels is a reserved key merged into all templates (see _helpers.tpl) and that users should not override system-managed labels; mention the expected type (map/object) and give a tiny example of usage to make behavior discoverable (e.g., commonLabels: {team: "foo"}). This comment should be placed next to the existing commonLabels: {} key so it’s visible when users open values.yaml.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helm/holmes/templates/_helpers.tpl`:
- Around line 60-66: The template currently renders .Values.commonLabels with
toYaml which preserves scalar types; convert every label value to a string
before rendering to ensure map[string]string semantics. Update the block that
iterates over commonLabels (the range using $key, $val and the reserved-key
check using has and fail) to build a new dict (e.g., start with an empty dict
and repeatedly call set) where each value is coerced with printf "%v" or quote
(or both) when assigned, then call toYaml on that new dict instead of the
original .Values.commonLabels so all label values are emitted as quoted strings.
---
Nitpick comments:
In `@helm/holmes/values.yaml`:
- Line 8: Add a short user-facing comment above the commonLabels entry in
values.yaml explaining that commonLabels is a reserved key merged into all
templates (see _helpers.tpl) and that users should not override system-managed
labels; mention the expected type (map/object) and give a tiny example of usage
to make behavior discoverable (e.g., commonLabels: {team: "foo"}). This comment
should be placed next to the existing commonLabels: {} key so it’s visible when
users open values.yaml.
🪄 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: d52c3f61-9436-4e5c-b1e4-9bc23919b6fe
📥 Commits
Reviewing files that changed from the base of the PR and between 529183316c3d7a2136a0b8cbc5e7b8c38b7ee85e and 38351a0.
📒 Files selected for processing (26)
helm/holmes/templates/_helpers.tplhelm/holmes/templates/holmes.yamlhelm/holmes/templates/holmesgpt-service-account.yamlhelm/holmes/templates/hpa.yamlhelm/holmes/templates/mcp-servers/aws/deployment.yamlhelm/holmes/templates/mcp-servers/aws/networkpolicy.yamlhelm/holmes/templates/mcp-servers/azure/deployment.yamlhelm/holmes/templates/mcp-servers/azure/networkpolicy.yamlhelm/holmes/templates/mcp-servers/confluence-mcp/deployment.yamlhelm/holmes/templates/mcp-servers/confluence-mcp/networkpolicy.yamlhelm/holmes/templates/mcp-servers/gcp/deployment.yamlhelm/holmes/templates/mcp-servers/gcp/networkpolicy.yamlhelm/holmes/templates/mcp-servers/github/deployment.yamlhelm/holmes/templates/mcp-servers/github/networkpolicy.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/networkpolicy.yamlhelm/holmes/templates/mcp-servers/mariadb/deployment.yamlhelm/holmes/templates/mcp-servers/mariadb/networkpolicy.yamlhelm/holmes/templates/mcp-servers/prefect/deployment.yamlhelm/holmes/templates/mcp-servers/prefect/networkpolicy.yamlhelm/holmes/templates/mcp-servers/sentry/deployment.yamlhelm/holmes/templates/mcp-servers/sentry/networkpolicy.yamlhelm/holmes/templates/operator-deployment.yamlhelm/holmes/templates/operator-rbac.yamlhelm/holmes/templates/toolset-config.yamlhelm/holmes/values.yaml
✅ Files skipped from review due to trivial changes (15)
- helm/holmes/templates/mcp-servers/mariadb/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/confluence-mcp/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/kubernetes-remediation/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/sentry/networkpolicy.yaml
- helm/holmes/templates/hpa.yaml
- helm/holmes/templates/mcp-servers/azure/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/github/deployment.yaml
- helm/holmes/templates/mcp-servers/sentry/deployment.yaml
- helm/holmes/templates/mcp-servers/prefect/deployment.yaml
- helm/holmes/templates/mcp-servers/confluence-mcp/deployment.yaml
- helm/holmes/templates/operator-deployment.yaml
- helm/holmes/templates/holmes.yaml
- helm/holmes/templates/mcp-servers/aws/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/prefect/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/gcp/networkpolicy.yaml
🚧 Files skipped from review as they are similar to previous changes (7)
- helm/holmes/templates/toolset-config.yaml
- helm/holmes/templates/mcp-servers/mariadb/deployment.yaml
- helm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yaml
- helm/holmes/templates/holmesgpt-service-account.yaml
- helm/holmes/templates/mcp-servers/gcp/deployment.yaml
- helm/holmes/templates/mcp-servers/aws/deployment.yaml
- helm/holmes/templates/operator-rbac.yaml
Signed-off-by: leegin <leegin.t@gmail.com>
Collaborator
|
Hi there! This looks good overall.
|
…lper Signed-off-by: leegin <leegin.t@gmail.com>
Contributor
Author
|
Thanks for the feedback! I've addressed both points: For the first one I missed routing the raw toYaml calls in holmesgpt-service-account.yaml and toolset-config.yaml through the holmes.commonLabels helper. All call sites now go through the helper so the reserved-key validation is enforced consistently everywhere. Also went ahead and added commonAnnotations support as well. Same pattern as commonLabels, wired into all resource and pod template metadata sections. No reserved-key validation on annotations since they don't affect selectors. Let me know if anything looks off! |
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 `@helm/holmes/templates/mcp-servers/mariadb/deployment.yaml`:
- Around line 59-62: The checksum/config annotation is being emitted before the
include "holmes.commonAnnotations", which allows a user-supplied
commonAnnotations["checksum/config"] to override the computed hash; change the
template so that the output of include "holmes.commonAnnotations" is rendered
before emitting checksum/config (or remove/filter the checksum/config key from
commonAnnotations before rendering) so the computed checksum (checksum/config)
cannot be overwritten; locate the block that calls include
"holmes.commonAnnotations" and move it above the checksum/config line or apply a
filter to exclude "checksum/config" from the commonAnnotations map prior to
rendering.
🪄 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: 2d4f17cf-32d3-41b6-9866-c6f60c1b938b
📒 Files selected for processing (26)
helm/holmes/templates/_helpers.tplhelm/holmes/templates/holmes.yamlhelm/holmes/templates/holmesgpt-service-account.yamlhelm/holmes/templates/hpa.yamlhelm/holmes/templates/mcp-servers/aws/deployment.yamlhelm/holmes/templates/mcp-servers/aws/networkpolicy.yamlhelm/holmes/templates/mcp-servers/azure/deployment.yamlhelm/holmes/templates/mcp-servers/azure/networkpolicy.yamlhelm/holmes/templates/mcp-servers/confluence-mcp/deployment.yamlhelm/holmes/templates/mcp-servers/confluence-mcp/networkpolicy.yamlhelm/holmes/templates/mcp-servers/gcp/deployment.yamlhelm/holmes/templates/mcp-servers/gcp/networkpolicy.yamlhelm/holmes/templates/mcp-servers/github/deployment.yamlhelm/holmes/templates/mcp-servers/github/networkpolicy.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/networkpolicy.yamlhelm/holmes/templates/mcp-servers/mariadb/deployment.yamlhelm/holmes/templates/mcp-servers/mariadb/networkpolicy.yamlhelm/holmes/templates/mcp-servers/prefect/deployment.yamlhelm/holmes/templates/mcp-servers/prefect/networkpolicy.yamlhelm/holmes/templates/mcp-servers/sentry/deployment.yamlhelm/holmes/templates/mcp-servers/sentry/networkpolicy.yamlhelm/holmes/templates/operator-deployment.yamlhelm/holmes/templates/operator-rbac.yamlhelm/holmes/templates/toolset-config.yamlhelm/holmes/values.yaml
✅ Files skipped from review due to trivial changes (8)
- helm/holmes/templates/mcp-servers/kubernetes-remediation/networkpolicy.yaml
- helm/holmes/values.yaml
- helm/holmes/templates/mcp-servers/confluence-mcp/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/github/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/mariadb/networkpolicy.yaml
- helm/holmes/templates/operator-deployment.yaml
- helm/holmes/templates/_helpers.tpl
- helm/holmes/templates/mcp-servers/aws/networkpolicy.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- helm/holmes/templates/toolset-config.yaml
- helm/holmes/templates/mcp-servers/azure/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/sentry/networkpolicy.yaml
- helm/holmes/templates/holmes.yaml
- helm/holmes/templates/mcp-servers/azure/deployment.yaml
- helm/holmes/templates/hpa.yaml
- helm/holmes/templates/mcp-servers/gcp/networkpolicy.yaml
- helm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yaml
- helm/holmes/templates/operator-rbac.yaml
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 `@helm/holmes/templates/mcp-servers/aws/deployment.yaml`:
- Around line 49-57: The annotations block currently renders two separate YAML
maps (serviceAccount annotations and holmes.commonAnnotations) which can produce
duplicate keys; instead build a single merged map (e.g. create a local
$annotations using Sprig/Helm merge or deepMerge between
.Values.mcpAddons.aws.serviceAccount.annotations and the output of include
"holmes.commonAnnotations" .) then render that merged map exactly once with
toYaml $annotations | nindent 4 and remove the two separate toYaml/with blocks;
keep the existing guard that checks multiAccount.enabled/profiles when deciding
whether to include serviceAccount annotations.
🪄 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: 31a5123a-cacd-490e-b999-bc56f2f5589a
📒 Files selected for processing (3)
helm/holmes/templates/mcp-servers/aws/deployment.yamlhelm/holmes/templates/toolset-config.yamlhelm/holmes/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- helm/holmes/values.yaml
- helm/holmes/templates/toolset-config.yaml
Signed-off-by: leegin <leegin.t@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
helm/holmes/templates/mcp-servers/azure/deployment.yaml (1)
89-92: 💤 Low valueConsider adding a
checksum/configannotation for consistency.Unlike the sentry, gcp, and kubernetes-remediation deployments which always include a
checksum/configannotation on the pod template (enabling automatic pod restarts when configuration changes), the azure pod template's annotations block is entirely conditional oncommonAnnotations. IfcommonAnnotationsis empty, the pod will have no annotations at all.If config-change-triggered rolling updates are desired, consider aligning with the other MCP deployments:
♻️ Suggested change for consistency
- {{- with (include "holmes.commonAnnotations" .) }} annotations: - {{- . | nindent 8 }} - {{- end }} + {{- with (include "holmes.commonAnnotations" .) }} + {{- . | nindent 8 }} + {{- end }} + checksum/config: {{ .Values.mcpAddons.azure | toYaml | sha256sum }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@helm/holmes/templates/mcp-servers/azure/deployment.yaml` around lines 89 - 92, Ensure the pod template always includes a checksum/config annotation so config changes trigger rolling updates: modify the annotations block that currently relies on (include "holmes.commonAnnotations" .) to always add a `checksum/config` entry (computed from the relevant ConfigMap/Secret content) alongside any common annotations; keep using the existing include "holmes.commonAnnotations" call to merge additional annotations, but append an unconditional `checksum/config` key in the pod template metadata annotations so the azure deployment behavior matches sentry/gcp/kubernetes-remediation deployments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@helm/holmes/templates/mcp-servers/azure/deployment.yaml`:
- Around line 89-92: Ensure the pod template always includes a checksum/config
annotation so config changes trigger rolling updates: modify the annotations
block that currently relies on (include "holmes.commonAnnotations" .) to always
add a `checksum/config` entry (computed from the relevant ConfigMap/Secret
content) alongside any common annotations; keep using the existing include
"holmes.commonAnnotations" call to merge additional annotations, but append an
unconditional `checksum/config` key in the pod template metadata annotations so
the azure deployment behavior matches sentry/gcp/kubernetes-remediation
deployments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5148526e-dfbb-4530-bdb4-89bd30ca834d
📒 Files selected for processing (11)
helm/holmes/templates/holmes.yamlhelm/holmes/templates/holmesgpt-service-account.yamlhelm/holmes/templates/mcp-servers/aws/deployment.yamlhelm/holmes/templates/mcp-servers/azure/deployment.yamlhelm/holmes/templates/mcp-servers/confluence-mcp/deployment.yamlhelm/holmes/templates/mcp-servers/gcp/deployment.yamlhelm/holmes/templates/mcp-servers/github/deployment.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yamlhelm/holmes/templates/mcp-servers/mariadb/deployment.yamlhelm/holmes/templates/mcp-servers/prefect/deployment.yamlhelm/holmes/templates/mcp-servers/sentry/deployment.yaml
✅ Files skipped from review due to trivial changes (4)
- helm/holmes/templates/mcp-servers/github/deployment.yaml
- helm/holmes/templates/holmes.yaml
- helm/holmes/templates/mcp-servers/confluence-mcp/deployment.yaml
- helm/holmes/templates/mcp-servers/prefect/deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- helm/holmes/templates/mcp-servers/mariadb/deployment.yaml
- helm/holmes/templates/holmesgpt-service-account.yaml
- helm/holmes/templates/mcp-servers/aws/deployment.yaml
Signed-off-by: leegin <leegin.t@gmail.com>
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 `@helm/holmes/templates/mcp-servers/azure/deployment.yaml`:
- Around line 41-47: The merge order is reversed so chart-wide annotations
override Azure ServiceAccount-specific ones; change the two mergeOverwrite calls
to merge holmes.commonAnnotations into $ann first and then merge
.Values.mcpAddons.azure.serviceAccount.annotations so that keys in
.Values.mcpAddons.azure.serviceAccount.annotations take precedence; update the
block that builds $ann (referencing $ann, mergeOverwrite,
.Values.mcpAddons.azure.serviceAccount.annotations and the include
"holmes.commonAnnotations") to perform the merges in that order.
🪄 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: ab138a59-f76d-462e-92a7-edde9f3b391b
📒 Files selected for processing (1)
helm/holmes/templates/mcp-servers/azure/deployment.yaml
…ion overrides. Signed-off-by: leegin <leegin.t@gmail.com>
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 `@helm/holmes/templates/mcp-servers/azure/deployment.yaml`:
- Around line 15-18: The template emits holmes.commonAnnotations directly into
metadata.annotations which can produce non-string annotation values; update the
helper or rendering so all annotation values are coerced to strings before
emitting. Specifically, modify the "holmes.commonAnnotations" helper (or the
usage block in the deployment template) to iterate the map and apply quote (or
otherwise stringify each value) so the output is a map[string]string, then
render that stringified map into annotations (the block using {{- with (include
"holmes.commonAnnotations" .) }} ... {{- end }}). Ensure the helper/usage
returns YAML where every annotation value is quoted/stringified to prevent
invalid manifests.
🪄 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: f0ac7ab6-e04d-4356-874d-0ac9303a1b17
📒 Files selected for processing (5)
helm/holmes/templates/holmesgpt-service-account.yamlhelm/holmes/templates/mcp-servers/aws/deployment.yamlhelm/holmes/templates/mcp-servers/azure/deployment.yamlhelm/holmes/templates/mcp-servers/gcp/deployment.yamlhelm/holmes/templates/mcp-servers/kubernetes-remediation/deployment.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- helm/holmes/templates/mcp-servers/gcp/deployment.yaml
- helm/holmes/templates/mcp-servers/aws/deployment.yaml
- helm/holmes/templates/holmesgpt-service-account.yaml
Signed-off-by: leegin <leegin.t@gmail.com>
…nore Signed-off-by: leegin <leegin.t@gmail.com>
moshemorad
approved these changes
May 12, 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
4 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.
Adds a new
commonLabelsvalue that applies user-defined labels to every K8s object created by the Holmes Helm chart like Deployments, Services, ServiceAccounts, ClusterRoles, ClusterRoleBindings, HPA, ConfigMaps, and all MCP server resources (Deployments, Services, ConfigMaps, ServiceAccounts, NetworkPolicies, Secrets, and RBAC objects).Usage:
Summary by CodeRabbit