fix(ext-proxy): remove hardcoded ports in ext-proxy helm chart#368
fix(ext-proxy): remove hardcoded ports in ext-proxy helm chart#368
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughThe PR updates the ext-proxy Helm chart: Chart.yaml 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/ext-proxy/templates/proxy.yaml (1)
46-50: Add fail-fast validation for themetricsport when PodMonitor is enabled.The container ports on Lines 46-50 are rendered dynamically from
.Values.proxy.ports, butpodmonitor.yaml:16has a hardcoded dependency onport: metrics. Users who removemetricsfrom custom values while keepingpodMonitor.enabled: truewill silently create a broken configuration. Add validation to require themetricsport key whenever PodMonitor is enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/ext-proxy/templates/proxy.yaml` around lines 46 - 50, The template renders container ports from .Values.proxy.ports but doesn't validate presence of the "metrics" key while podMonitor.enabled can reference it; add a Helm template guard that checks podMonitor.enabled and ensures hasKey .Values.proxy.ports "metrics" (or use required on .Values.proxy.ports.metrics) and fail early with a clear message if missing, placing this check before the range in the proxy.yaml template so PodMonitor (podmonitor) won't be generated against a non-existent metrics port.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@charts/ext-proxy/templates/proxy.yaml`:
- Around line 46-50: The template renders container ports from
.Values.proxy.ports but doesn't validate presence of the "metrics" key while
podMonitor.enabled can reference it; add a Helm template guard that checks
podMonitor.enabled and ensures hasKey .Values.proxy.ports "metrics" (or use
required on .Values.proxy.ports.metrics) and fail early with a clear message if
missing, placing this check before the range in the proxy.yaml template so
PodMonitor (podmonitor) won't be generated against a non-existent metrics port.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: feb60374-cb58-4df2-828a-5c3fc5ba7857
📒 Files selected for processing (2)
charts/ext-proxy/Chart.yamlcharts/ext-proxy/templates/proxy.yaml
Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
5a0c24a to
7380963
Compare
Summary by cubic
Replaced hardcoded ports in the ext-proxy Helm chart with a values-driven loop so ports are configurable and can include multiple entries. Made PodMonitor metrics port configurable via
podMonitor.metricsPort; bumped chart version to 0.0.7.Written for commit 7380963. Summary will update on new commits.
Summary by CodeRabbit
Chores
Refactor