fix(cronicle): align template validation standards#656
Conversation
github-actions
Bot
added
area:templates
area:tests
chart:cronicle
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a render-time validation helper for secret, ingress, and pod label checks, wires it through a new template, and updates ingress host rendering to allow hostless rules. Adds tests for validation failures and hostless ingress output. ChangesCronicle Chart Standards Alignment
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🟢 Security Scan:
|
| Framework | Score |
|---|---|
| MITRE + NSA + SOC2 | 72.72727% |
✅ Security posture acceptable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/cronicle/templates/_helpers.tpl (1)
46-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuardrails look correct; selector-key list duplicates
cronicle.selectorLabels.The three checks (secret dependency, ingress hosts, podLabels vs. selector keys) are logically sound and align with the downstream consumers (secret.yaml, ingress.yaml, deployment.yaml). One maintainability note: the
app.kubernetes.io/name/app.kubernetes.io/instancekey list at Lines 54 is hardcoded separately from whatevercronicle.selectorLabelsactually defines. If that helper is ever extended with another selector key, this guard will silently miss it.Consider deriving the guarded key set from
cronicle.selectorLabelsoutput (e.g., viafromYaml (include "cronicle.selectorLabels" .)) so the two stay in sync automatically.♻️ Example approach
-{{- range $key, $_ := .Values.podLabels -}} -{{- if or (eq $key "app.kubernetes.io/name") (eq $key "app.kubernetes.io/instance") -}} -{{- fail (printf "podLabels must not override selector label %q" $key) -}} -{{- end -}} -{{- end -}} +{{- $selectorKeys := fromYaml (include "cronicle.selectorLabels" .) -}} +{{- range $key, $_ := .Values.podLabels -}} +{{- if hasKey $selectorKeys $key -}} +{{- fail (printf "podLabels must not override selector label %q" $key) -}} +{{- end -}} +{{- end -}}🤖 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/cronicle/templates/_helpers.tpl` around lines 46 - 59, The podLabels selector-key guard in cronicle.validate is duplicating the selector label list instead of reusing cronicle.selectorLabels, so it can drift when selector keys change. Update the validation logic to derive the blocked keys from the cronicle.selectorLabels helper output and compare podLabels against that set, keeping the guard aligned automatically 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/cronicle/templates/_helpers.tpl`:
- Around line 46-59: The podLabels selector-key guard in cronicle.validate is
duplicating the selector label list instead of reusing cronicle.selectorLabels,
so it can drift when selector keys change. Update the validation logic to derive
the blocked keys from the cronicle.selectorLabels helper output and compare
podLabels against that set, keeping the guard aligned automatically with any
future selector label additions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75ac9922-804f-40df-860a-8aac58b2a3c5
📒 Files selected for processing (5)
charts/cronicle/templates/_helpers.tplcharts/cronicle/templates/ingress.yamlcharts/cronicle/templates/validate.yamlcharts/cronicle/tests/ingress_test.yamlcharts/cronicle/tests/validation_test.yaml
mberlofa
force-pushed
the
fix/cronicle-template-standards
branch
from
33ad17f to
ec3018b
Compare
|
Addressed the CodeRabbit review-summary nitpick for selector-label validation. Change:
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 cronicle to the Helm values playground eligibility map required by the chart sync check ## Validation - make site-sync-check CHART=cronicle - npm run lint - npm run format:check - npm run build - make release-check REPO=site - make attribution-check REPO=site Chart PR: helmforgedev/charts#656 Issue: helmforgedev/charts#633 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for the `cronicle` chart in the playground’s sync/config selection, making it available alongside other eligible chart slugs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Summary
Validation
Site PR: helmforgedev/site#335
Issue: #633
Summary by CodeRabbit
hostfield when no host is provided.podLabelsfrom overriding the chart’s selector label keys.