Conversation
WalkthroughAdds a conditional metrics Service template and a conditional Prometheus ServiceMonitor; adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Helm Values
participant Tpl as Chart Templates
participant K8s as Kubernetes API
User->>Tpl: Render chart with values
alt .Values.prometheus.enabled == true
Tpl->>K8s: Create ServiceMonitor (name: <fullname>)
note right of Tpl: selector includes chart labels + component\ncomponent = app if metricsPort == port\nelse component = metrics
Tpl->>K8s: ServiceMonitor endpoint -> port: http, path: /metrics
else
note right of Tpl: Skip ServiceMonitor
end
alt metricsPort is set AND metricsPort != port
Tpl->>K8s: Create Service (<fullname>-metrics)
note right of Tpl: metadata.labels includes component: metrics\nport name: http -> targetPort: "metrics"
else
note right of Tpl: Skip metrics Service
end
Tpl->>K8s: Create primary Service (component: app)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
charts/chart-sentinel/templates/servicemonitor.yaml (2)
1-1: Yamllint false-positive due to Helm trim marker; add trailing newline anyway.Template directives at file start confuse YAML linters; that’s okay. Do add a newline at EOF to satisfy basic linters.
17-19: Consider making scrape settings configurable.Expose
intervalandscrapeTimeoutvia values (defaults retained) for flexibility.charts/chart-sentinel/templates/service-metrics.yaml (1)
1-18: Name the exposed port “metrics” and (optionally) surface the Service port from values.
- Set
name: metricsso the ServiceMonitor can target it unambiguously.- Optional: use
.Values.server.metricsPortas the Service’sportto mirror the container port (keeps things self‑describing).Apply:
ports: - - port: {{ .Values.service.port }} - targetPort: metrics + - port: {{ .Values.server.metricsPort | default .Values.service.port }} + targetPort: metrics protocol: TCP - name: http + name: metricsAlso add a trailing newline at EOF.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
charts/chart-sentinel/templates/service-metrics.yaml(1 hunks)charts/chart-sentinel/templates/service.yaml(1 hunks)charts/chart-sentinel/templates/servicemonitor.yaml(1 hunks)charts/chart-sentinel/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/chart-sentinel/templates/service-metrics.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
charts/chart-sentinel/templates/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 20-20: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (4)
charts/chart-sentinel/values.yaml (1)
45-45: No behavioral change; format tweak LGTM.Single‑line empty map is fine and equivalent to the previous form.
charts/chart-sentinel/templates/service.yaml (1)
7-7: Adding component: app label is good; keep ServiceMonitor selector in sync.This aligns Services by role. Ensure the ServiceMonitor’s selector also filters on
component: appwhen scraping on the main port.charts/chart-sentinel/templates/service-metrics.yaml (1)
1-1: Template gate is correct.The
and(metricsPort, ne(metricsPort, port))guard cleanly avoids duplicate Services when sharing the port.Ensure the Deployment/Pod defines a container port named
metricswhenserver.metricsPortis set; otherwise the Service will be broken.charts/chart-sentinel/templates/servicemonitor.yaml (1)
1-20: Quick render checks (two scenarios) — run locally to verify ServiceMonitor selector/portSandbox run failed: 'helm' not installed. Run the commands below locally and paste the outputs.
Location: charts/chart-sentinel/templates/servicemonitor.yaml (lines 1–20)
#!/bin/bash set -euo pipefail # Split port: expect component: metrics and port: metrics helm template chart-sentinel charts/chart-sentinel \ --set prometheus.enabled=true \ --set server.port=8080 \ --set server.metricsPort=8081 \ | awk 'BEGIN{doc=0} /^---/{doc++} {print > "out."doc".yaml"} END{print NR > "/dev/stderr"}' >/dev/null rg -nP -C2 'kind:\s*ServiceMonitor|\bcomponent:\s*metrics\b|\bport:\s*metrics\b' out.*.yaml # Same port: expect component: app and port: http helm template chart-sentinel charts/chart-sentinel \ --set prometheus.enabled=true \ --set server.port=8080 \ --set server.metricsPort=8080 \ | awk 'BEGIN{doc=0} /^---/{doc++} {print > "out2."doc".yaml"}' rg -nP -C2 'kind:\s*ServiceMonitor|\bcomponent:\s*app\b|\bport:\s*http\b' out2.*.yamlExpected outcomes:
- Split port (metricsPort != port): ServiceMonitor label component: metrics and endpoint port showing metrics.
- Same port (metricsPort == port): ServiceMonitor label component: app and endpoint port http.
| spec: | ||
| selector: | ||
| matchLabels: | ||
| {{- include "chart-sentinel.labels" . | nindent 6 }} | ||
| endpoints: | ||
| - port: http | ||
| path: /api/v1/metrics |
There was a problem hiding this comment.
Selector misses the component label; may match both Services and scrape the wrong port.
spec.selector.matchLabels only includes chart-sentinel.labels, so it will match both the app and metrics Services. Add the same conditional component label here, and switch the endpoint port name based on whether metrics share the main port.
Apply:
spec:
selector:
matchLabels:
- {{- include "chart-sentinel.labels" . | nindent 6 }}
+ {{- include "chart-sentinel.labels" . | nindent 6 }}
+ {{- if eq .Values.server.metricsPort .Values.server.port }}
+ component: app
+ {{- else }}
+ component: metrics
+ {{- end }}
endpoints:
- - port: http
+ {{- if eq .Values.server.metricsPort .Values.server.port }}
+ - port: http
+ {{- else }}
+ - port: metrics
+ {{- end }}
path: /api/v1/metrics📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| spec: | |
| selector: | |
| matchLabels: | |
| {{- include "chart-sentinel.labels" . | nindent 6 }} | |
| endpoints: | |
| - port: http | |
| path: /api/v1/metrics | |
| spec: | |
| selector: | |
| matchLabels: | |
| {{- include "chart-sentinel.labels" . | nindent 6 }} | |
| {{- if eq .Values.server.metricsPort .Values.server.port }} | |
| component: app | |
| {{- else }} | |
| component: metrics | |
| {{- end }} | |
| endpoints: | |
| {{- if eq .Values.server.metricsPort .Values.server.port }} | |
| - port: http | |
| {{- else }} | |
| - port: metrics | |
| {{- end }} | |
| path: /api/v1/metrics |
🤖 Prompt for AI Agents
In charts/chart-sentinel/templates/servicemonitor.yaml around lines 13 to 19,
the spec.selector.matchLabels only includes the generic chart-sentinel.labels
which can match both the app Service and the metrics Service; add the same
conditional component label used elsewhere (e.g., include a conditional block
that adds component: metrics when .Values.service.component or template context
indicates a separate metrics Service) to the matchLabels so only the metrics
Service is selected, and adjust the endpoints.port value to be conditional (use
the main port name when metrics share the app port, otherwise use the metrics
port name) so the correct port is scraped.
ArnaudTA
force-pushed
the
test
branch
3 times, most recently
from
aa2e73b to
9c055d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
charts/chart-sentinel/templates/servicemonitor.yaml (1)
15-21: Selector now disambiguates Services — resolves prior concern.Adding the same conditional
componentlabel inmatchLabelsprevents scraping the wrong Service. Thanks for addressing this.
🧹 Nitpick comments (5)
charts/chart-sentinel/templates/servicemonitor.yaml (3)
22-24: Consider naming the metrics Service port “metrics” and selecting it conditionally.Today both Services expose a port named
http, which works but is ambiguous. Recommend:
- In the separate metrics Service, name the port
metrics.- Here, choose the endpoint port by condition.
Apply:
endpoints: - - port: http + {{- if eq .Values.server.metricsPort .Values.server.port }} + - port: http + {{- else }} + - port: metrics + {{- end }} path: /metrics
24-24: Make scrape path configurable (default to /metrics).Expose a value like
.Values.prometheus.pathwith default/metricsto avoid hard-coding.- path: /metrics + path: {{ .Values.prometheus.path | default "/metrics" }}
25-25: Add trailing newline.Ends without a newline; add one for tooling friendliness.
charts/chart-sentinel/templates/service-metrics.yaml (2)
10-10: Avoid reusing the app Service type for metrics.Metrics scraping typically needs an internal ClusterIP. Consider a separate value with safe default:
- type: {{ .Values.service.type }} + type: {{ .Values.serviceMetrics.type | default "ClusterIP" }}
18-18: Add trailing newline.Ends without a newline; add one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
charts/chart-sentinel/Chart.yaml(1 hunks)charts/chart-sentinel/README.md(1 hunks)charts/chart-sentinel/templates/service-metrics.yaml(1 hunks)charts/chart-sentinel/templates/service.yaml(1 hunks)charts/chart-sentinel/templates/servicemonitor.yaml(1 hunks)charts/chart-sentinel/values.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- charts/chart-sentinel/README.md
- charts/chart-sentinel/Chart.yaml
- charts/chart-sentinel/templates/service.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/chart-sentinel/values.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/chart-sentinel/templates/service-metrics.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
charts/chart-sentinel/templates/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[error] 25-25: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (4)
charts/chart-sentinel/templates/servicemonitor.yaml (2)
1-1: Good conditional gating on Prometheus flag.Chart renders the ServiceMonitor only when enabled. LGTM.
8-12: Metadata component label: aligned with port-sharing logic.Conditional component label is correct when metrics share the app port vs. a separate metrics Service.
charts/chart-sentinel/templates/service-metrics.yaml (2)
6-9: Labels look good.
component: metricsplus chart labels provide a clear selector target.
17-17: Selector is correct.Using
selectorLabelsto match the Pod labels is consistent with the primary Service.
| - port: {{ .Values.service.port }} | ||
| targetPort: metrics | ||
| protocol: TCP | ||
| name: http |
There was a problem hiding this comment.
🧩 Analysis chain
Service port should reflect metricsPort and be named “metrics”.
Using .Values.service.port for a dedicated metrics Service is misleading and can break consumers relying on the numeric port. Set it to the metrics port and give the port a distinct name.
ports:
- - port: {{ .Values.service.port }}
- targetPort: metrics
+ - port: {{ .Values.server.metricsPort }}
+ targetPort: metrics
protocol: TCP
- name: http
+ name: metricsIf you adopt this, update the ServiceMonitor to use port: metrics when a separate metrics Service is created.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "=== Exists checks ==="
[ -d charts ] && echo "charts/ exists" || echo "charts/ missing"
[ -d charts/chart-sentinel ] && echo "charts/chart-sentinel/ exists" || echo "charts/chart-sentinel/ missing"
[ -d charts/chart-sentinel/templates ] && echo "charts/chart-sentinel/templates/ exists" || echo "charts/chart-sentinel/templates/ missing"
echo
echo "=== charts/chart-sentinel/templates/service-metrics.yaml (if present) ==="
if [ -f charts/chart-sentinel/templates/service-metrics.yaml ]; then
nl -ba charts/chart-sentinel/templates/service-metrics.yaml | sed -n '1,240p'
else
echo "MISSING: charts/chart-sentinel/templates/service-metrics.yaml"
fi
echo
echo "=== charts/chart-sentinel/values.yaml (if present) ==="
if [ -f charts/chart-sentinel/values.yaml ]; then
nl -ba charts/chart-sentinel/values.yaml | sed -n '1,400p'
else
echo "MISSING: charts/chart-sentinel/values.yaml"
fi
echo
echo "=== Searching repo for relevant tokens ==="
rg -n --hidden --no-ignore-vcs 'metricsPort' -S || true
rg -n --hidden --no-ignore-vcs '\.Values\.server\.metricsPort' -S || true
rg -n --hidden --no-ignore-vcs '\.Values\.service\.port' -S || true
rg -n --hidden --no-ignore-vcs 'targetPort:\s*metrics' -n -S || true
rg -n --hidden --no-ignore-vcs 'port:\s*metrics' -n -S || true
rg -n --hidden --no-ignore-vcs 'ServiceMonitor' -n -S || true
rg -n --hidden --no-ignore-vcs 'servicemonitor' -n -S || true
echo
echo "=== Listing charts/chart-sentinel/templates/ ==="
if [ -d charts/chart-sentinel/templates ]; then
ls -la charts/chart-sentinel/templates || true
fiLength of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "=== Exists checks ==="
[ -d charts ] && echo "charts/ exists" || echo "charts/ missing"
[ -d charts/chart-sentinel ] && echo "charts/chart-sentinel/ exists" || echo "charts/chart-sentinel/ missing"
[ -d charts/chart-sentinel/templates ] && echo "charts/chart-sentinel/templates/ exists" || echo "charts/chart-sentinel/templates/ missing"
echo
echo "=== charts/chart-sentinel/templates/service-metrics.yaml (if present) ==="
if [ -f charts/chart-sentinel/templates/service-metrics.yaml ]; then
echo '--- start of file ---'
sed -n '1,240p' charts/chart-sentinel/templates/service-metrics.yaml || true
echo '--- end of file ---'
else
echo "MISSING: charts/chart-sentinel/templates/service-metrics.yaml"
fi
echo
echo "=== charts/chart-sentinel/values.yaml (if present) ==="
if [ -f charts/chart-sentinel/values.yaml ]; then
echo '--- start of values ---'
sed -n '1,400p' charts/chart-sentinel/values.yaml || true
echo '--- end of values ---'
else
echo "MISSING: charts/chart-sentinel/values.yaml"
fi
echo
echo "=== Searching repo for relevant tokens ==="
if command -v rg >/dev/null 2>&1; then
rg -n --hidden --no-ignore-vcs 'metricsPort' -S || true
rg -n --hidden --no-ignore-vcs '\.Values\.server\.metricsPort' -S || true
rg -n --hidden --no-ignore-vcs '\.Values\.service\.port' -S || true
rg -n --hidden --no-ignore-vcs 'targetPort:\s*metrics' -S || true
rg -n --hidden --no-ignore-vcs 'port:\s*metrics' -S || true
rg -n --hidden --no-ignore-vcs 'ServiceMonitor' -S || true
rg -n --hidden --no-ignore-vcs 'servicemonitor' -S || true
else
echo "rg not found; using grep fallback"
grep -nR --line-number --exclude-dir=.git -E 'metricsPort|\.Values\.server\.metricsPort|\.Values\.service\.port|targetPort:\s*metrics|port:\s*metrics|ServiceMonitor|servicemonitor' . || true
fi
echo
echo "=== Listing charts/chart-sentinel/templates/ ==="
if [ -d charts/chart-sentinel/templates ]; then
ls -la charts/chart-sentinel/templates || true
fiLength of output: 9442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== deployment.yaml (full) ==="
sed -n '1,240p' charts/chart-sentinel/templates/deployment.yaml || true
echo
echo "=== servicemonitor.yaml (full) ==="
sed -n '1,240p' charts/chart-sentinel/templates/servicemonitor.yaml || true
echo
echo "=== podmonitor.yaml (full) ==="
sed -n '1,240p' charts/chart-sentinel/templates/podmonitor.yaml || true
echo
echo "=== service.yaml (full) ==="
sed -n '1,240p' charts/chart-sentinel/templates/service.yaml || trueLength of output: 4984
Expose metrics port and name it "metrics"
service-metrics.yaml currently exposes .Values.service.port named "http" while the metrics container uses .Values.server.metricsPort named "metrics" — this mismatches and will break consumers/ServiceMonitor. Change the Service port and update the ServiceMonitor endpoint.
Files: charts/chart-sentinel/templates/service-metrics.yaml, charts/chart-sentinel/templates/servicemonitor.yaml
ports:
- - port: {{ .Values.service.port }}
- targetPort: metrics
- protocol: TCP
- name: http
+ - port: {{ .Values.server.metricsPort }}
+ targetPort: metrics
+ protocol: TCP
+ name: metrics endpoints:
- - port: http
+ - port: {{ and (.Values.server.metricsPort) (ne .Values.server.metricsPort .Values.server.port) | ternary "metrics" "http" }}
path: /metricsCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In charts/chart-sentinel/templates/service-metrics.yaml around lines 12 to 15,
the Service currently exposes .Values.service.port and names the port "http"
which mismatches the metrics container; change the Service port to use
.Values.server.metricsPort, keep targetPort: metrics and rename the port to
"metrics" so the Service maps to the metrics container; then update
charts/chart-sentinel/templates/servicemonitor.yaml to reference the Service
endpoint port name "metrics" (or .Values.server.metricsPort if numeric) so the
ServiceMonitor targets the correct port.
1 participant
Summary by CodeRabbit