fix(countly): align template validation standards#655
Conversation
github-actions
Bot
added
area:templates
area:tests
area:values
chart:countly
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Helm render-time ChangesCountly chart template standards alignment
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HelmRender
participant ValidateTemplate
participant CountlyValidate
HelmRender->>ValidateTemplate: render validate.yaml with chart context
ValidateTemplate->>CountlyValidate: invoke countly.validate
CountlyValidate->>CountlyValidate: check externalMongodb, ingress, gateway, externalSecrets, podLabels
CountlyValidate->>CountlyValidate: check backup.enabled and backup.s3 fields
alt invalid configuration
CountlyValidate-->>HelmRender: fail with error message
else valid configuration
CountlyValidate-->>HelmRender: no output
end
Related issues: None specified. Related PRs: None specified. Suggested labels: helm, validation, ci Suggested reviewers: None specified. 🐰 A rabbit checks each field with care, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Standards Check (GR-079) — PASSEvery changed chart fully passes standards-check. |
🟢 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/countly/templates/_helpers.tpl (1)
102-119: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReuse
countly.mongodbHostincountly.mongodbUri.
The bundled MongoDB subchart follows the samecontainsfullname convention, so release names likemongodbormy-mongodb-appmake the host helper and URI diverge; the URI still appends-mongodband points at a nonexistent service.🤖 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/countly/templates/_helpers.tpl` around lines 102 - 119, The MongoDB URI helper is duplicating host naming logic instead of reusing the existing countly.mongodbHost helper, which can make the URI point to a different service name. Update countly.mongodbUri to call countly.mongodbHost for the bundled MongoDB case, so it follows the same contains-based fullname behavior as countly.mongodbHost and stays consistent for release names like mongodb or my-mongodb-app.
🧹 Nitpick comments (2)
charts/countly/tests/validation_test.yaml (1)
155-161: 📐 Maintainability & Code Quality | 🔵 TrivialMissing test for
app.kubernetes.io/instanceselector-label override.Only the
app.kubernetes.io/nameoverride path is tested; the guard also checksapp.kubernetes.io/instance(Line 96 in_helpers.tpl) but that branch is untested.🤖 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/countly/tests/validation_test.yaml` around lines 155 - 161, Add a validation test that covers podLabels overriding the app.kubernetes.io/instance selector label, since only app.kubernetes.io/name is currently asserted. Update the tests in validation_test.yaml alongside the existing podLabels override case, and make the new assertion expect the same failedTemplate behavior and the instance-specific error message from the selector-label guard in _helpers.tpl.charts/countly/templates/deployment.yaml (1)
42-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVerify bundled MongoDB is always deployed when
mongodb.enabled=true, even alongsideexternalMongodb.enabled=true.With the simplified condition, whenever
mongodb.enabled=truethe init container always waits oncountly.mongodbHost(the bundled subchart hostname), regardless ofexternalMongodb.enabled. Per the env block below (Line 60), whenexternalMongodb.enabled=truethe app actually connects toexternalMongodb.uri/secret instead of the bundled instance. This means enabling both flags simultaneously deploys a bundled MongoDB the app doesn't use, wasting resources, and only "works" correctly when the external URI happens to alias back to the bundled subchart's host (as done in the CI fixtures). This is confirmed intentional by the testshould wait for bundled MongoDB when external URI targets the subchart, but worth flagging as a potentially confusing coupling for chart consumers who set both flags with an unrelated external host.🤖 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/countly/templates/deployment.yaml` around lines 42 - 47, The init container logic in deployment template is coupled only to mongodb.enabled, so it always waits on the bundled subchart host even when externalMongodb.enabled is also set. Update the condition around the wait-for-mongodb initContainer in the deployment template to match the actual MongoDB target selected by the env/config block, using the existing helpers like countly.mongodbHost and the externalMongodb flags so the wait behavior aligns with the database the app will really use.
🤖 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.
Inline comments:
In `@charts/countly/templates/_helpers.tpl`:
- Around line 78-94: The backup validation logic in countly.validate is
duplicated and should be delegated to the existing helpers instead of rechecked
inline. Update countly.validate to call countly.backupEnabled and
countly.backupMongodbUri so the backup.s3 and backup.database.uri requirements
live in one place. Keep the existing fail behavior/messages in those helper
templates and remove the duplicated conditional blocks from countly.validate.
---
Outside diff comments:
In `@charts/countly/templates/_helpers.tpl`:
- Around line 102-119: The MongoDB URI helper is duplicating host naming logic
instead of reusing the existing countly.mongodbHost helper, which can make the
URI point to a different service name. Update countly.mongodbUri to call
countly.mongodbHost for the bundled MongoDB case, so it follows the same
contains-based fullname behavior as countly.mongodbHost and stays consistent for
release names like mongodb or my-mongodb-app.
---
Nitpick comments:
In `@charts/countly/templates/deployment.yaml`:
- Around line 42-47: The init container logic in deployment template is coupled
only to mongodb.enabled, so it always waits on the bundled subchart host even
when externalMongodb.enabled is also set. Update the condition around the
wait-for-mongodb initContainer in the deployment template to match the actual
MongoDB target selected by the env/config block, using the existing helpers like
countly.mongodbHost and the externalMongodb flags so the wait behavior aligns
with the database the app will really use.
In `@charts/countly/tests/validation_test.yaml`:
- Around line 155-161: Add a validation test that covers podLabels overriding
the app.kubernetes.io/instance selector label, since only app.kubernetes.io/name
is currently asserted. Update the tests in validation_test.yaml alongside the
existing podLabels override case, and make the new assertion expect the same
failedTemplate behavior and the instance-specific error message from the
selector-label guard in _helpers.tpl.
🪄 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 Plus
Run ID: 53b56ed7-01cc-4eb6-8830-2b58b229e6e6
📒 Files selected for processing (12)
charts/countly/ci/dual-stack.yamlcharts/countly/ci/external-mongodb-values.yamlcharts/countly/ci/external-secrets.yamlcharts/countly/ci/gateway-api.yamlcharts/countly/templates/_helpers.tplcharts/countly/templates/deployment.yamlcharts/countly/templates/ingress.yamlcharts/countly/templates/validate.yamlcharts/countly/tests/deployment_test.yamlcharts/countly/tests/ingress_test.yamlcharts/countly/tests/service_test.yamlcharts/countly/tests/validation_test.yaml
💤 Files with no reviewable changes (1)
- charts/countly/ci/gateway-api.yaml
mberlofa
force-pushed
the
fix/countly-template-standards
branch
from
4b32673 to
ce2c3c9
Compare
mberlofa
force-pushed
the
fix/countly-template-standards
branch
from
ce2c3c9 to
2513dd0
Compare
|
Reviewed all CodeRabbit feedback in review threads and review-summary comments. Changes:
Validation:
Note: the two latest CodeRabbit items were posted in the review summary/body, not as active review threads, so there are no thread IDs to reply to or resolve for them. The existing inline CodeRabbit thread is already resolved. |
## Summary - add countly to the Helm values playground eligibility map required by the chart sync check ## Validation - make site-sync-check CHART=countly - npm run lint - npm run format:check - npm run build - make release-check REPO=site - make attribution-check REPO=site Chart PR: helmforgedev/charts#655 Issue: helmforgedev/charts#633 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added the Countly chart to the playground’s available chart list, making it visible even when it isn’t marked as stable. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Summary
Validation
Site PR: helmforgedev/site#334
Issue: #633
Summary by CodeRabbit
New Features
Bug Fixes