fix(cloudflared): align template validation standards#653
Conversation
Standards Check (GR-079) — PASSEvery changed chart fully passes standards-check. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds render-time validation for cloudflared chart configuration, wires it into template rendering, adds validation tests, and reformats ChangesCloudflared chart validation and NOTES updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User as Helm user
participant Helm as Helm render
participant validateyaml as validate.yaml
participant Helper as cloudflared.validate
participant Tests as validation_test.yaml
User->>Helm: helm install/template
Helm->>validateyaml: render chart
validateyaml->>Helper: include "cloudflared.validate" .
Helper->>Helper: validate chart values
alt invalid configuration
Helper-->>Helm: fail
Helm-->>User: render error
else valid configuration
Helper-->>Helm: continue
Helm->>Tests: execute validation cases
end
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🟢 Security Scan:
|
| Framework | Score |
|---|---|
| MITRE + NSA + SOC2 | 87.878784% |
✅ Security posture acceptable.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/cloudflared/templates/_helpers.tpl (1)
68-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded selector-label keys duplicate the labels helper.
The loop hardcodes
"app.kubernetes.io/name"and"app.kubernetes.io/instance"rather than deriving them from the chart's selector-labels helper. If that helper is ever extended with additional keys, this validation silently misses them.Consider deriving the guarded keys from the same source used by
cloudflared.selectorLabels, or at minimum add a comment noting they must be kept in sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/cloudflared/templates/_helpers.tpl` around lines 68 - 72, The podLabels validation in cloudflared.selectorLabels-related helpers hardcodes selector keys instead of using the same source as the selector labels themselves. Update the guard in _helpers.tpl so it derives the forbidden keys from the chart’s selector-labels helper or shared selector-label definition, and ensure the fail check in the podLabels loop stays in sync with any future selector-label additions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@charts/cloudflared/templates/_helpers.tpl`:
- Around line 68-72: The podLabels validation in
cloudflared.selectorLabels-related helpers hardcodes selector keys instead of
using the same source as the selector labels themselves. Update the guard in
_helpers.tpl so it derives the forbidden keys from the chart’s selector-labels
helper or shared selector-label definition, and ensure the fail check in the
podLabels loop stays in sync with any future selector-label additions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 48d99cf9-e3dd-4c2d-8ec6-0daff0d322bc
📒 Files selected for processing (4)
charts/cloudflared/templates/NOTES.txtcharts/cloudflared/templates/_helpers.tplcharts/cloudflared/templates/validate.yamlcharts/cloudflared/tests/validation_test.yaml
49df97c to
8fd482c
Compare
|
Addressed the CodeRabbit review-summary nitpick for selector-label validation. Changes:
Validation:
Note: this CodeRabbit item was posted in the review summary/body, not as an active review thread, so there is no thread ID to reply to or resolve for it. |
## Summary - add cloudflared to the Helm values playground eligibility map required by the chart sync check ## Validation - make site-sync-check CHART=cloudflared - npm run lint - npm run format:check - npm run build - make release-check REPO=site - make attribution-check REPO=site Chart PR: helmforgedev/charts#653 Issue: helmforgedev/charts#633 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for the **cloudflared** chart in the playground chart list, making it available alongside other supported charts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Validation
Site PR: helmforgedev/site#333
Issue: #633
Summary by CodeRabbit