fix(dolibarr): centralize template validation#660
Conversation
github-actions
Bot
added
area:templates
area:tests
chart:dolibarr
Standards Check (GR-079) — PASSEvery changed chart fully passes standards-check. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 Helm validation helper, wires it into chart rendering, and extends tests for ingress, gateway, backup, externalSecrets, and podLabels validation. ChangesChart Validation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80fcdb64c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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/dolibarr/tests/validation_test.yaml (1)
1-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage gap: Gateway API and remaining externalSecrets branches untested.
The PR objectives state unit tests cover "Gateway API" and "External Secrets" validation, but this suite only exercises ingress, podLabels, backup, and the
admin.existingSecretexternalSecrets branch. Missing negative cases for the newdolibarr.validatebranches added in_helpers.tpl:
gateway.enabledwith nogateway.parentRefs(Line 275-277 of_helpers.tpl).gateway.parentRefsentries missingname(Line 279-281).externalSecrets.dataempty (Line 288-290).externalSecrets.datamissing the admin-password key mapping (Line 296-298).Recommend adding these cases to keep the validation helper's test coverage in sync with its actual branches.
➕ Example additional test cases
- it: should reject gateway without parentRefs set: gateway.enabled: true asserts: - failedTemplate: errorMessage: "gateway.enabled requires gateway.parentRefs to be populated to create a valid HTTPRoute." - it: should reject gateway parentRefs entry missing name set: gateway.enabled: true gateway.parentRefs: - namespace: default asserts: - failedTemplate: errorMessage: "Each gateway.parentRefs entry must define a 'name' field" - it: should reject externalSecrets without data set: admin.existingSecret: dolibarr-admin externalSecrets.enabled: true externalSecrets.data: [] asserts: - failedTemplate: errorMessage: "externalSecrets.data must not be empty when externalSecrets.enabled=true" - it: should reject externalSecrets missing admin password mapping set: admin.existingSecret: dolibarr-admin externalSecrets.enabled: true externalSecrets.data: - secretKey: some-other-key remoteRef: key: dolibarr/other asserts: - failedTemplate: errorMessage: "externalSecrets.data must include a mapping for key 'admin-password' (admin password)"🤖 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/dolibarr/tests/validation_test.yaml` around lines 1 - 40, Add missing validation tests in validation_test.yaml to cover the remaining branches in dolibarr.validate from _helpers.tpl. Include negative cases for gateway.enabled without gateway.parentRefs, gateway.parentRefs entries missing name, externalSecrets.enabled with empty externalSecrets.data, and externalSecrets.data that does not map admin-password. Use the existing test style and failedTemplate assertions so the suite matches the actual validation paths.
🤖 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/dolibarr/tests/validation_test.yaml`:
- Around line 1-40: Add missing validation tests in validation_test.yaml to
cover the remaining branches in dolibarr.validate from _helpers.tpl. Include
negative cases for gateway.enabled without gateway.parentRefs,
gateway.parentRefs entries missing name, externalSecrets.enabled with empty
externalSecrets.data, and externalSecrets.data that does not map admin-password.
Use the existing test style and failedTemplate assertions so the suite matches
the actual validation paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10e22b92-f9b1-4dba-89e9-2f709b35d386
📒 Files selected for processing (3)
charts/dolibarr/templates/_helpers.tplcharts/dolibarr/templates/validate.yamlcharts/dolibarr/tests/validation_test.yaml
mberlofa
force-pushed
the
fix/dolibarr-template-standards
branch
from
b636270 to
581f747
Compare
mberlofa
force-pushed
the
fix/dolibarr-template-standards
branch
from
581f747 to
7e95859
Compare
|
Addressed the remaining CodeRabbit review-summary nitpick for centralized validation coverage. 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. The existing inline thread is already resolved. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
charts/dolibarr/tests/validation_test.yaml (2)
31-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also testing the
app.kubernetes.io/instancereserved label.The helper (
charts/dolibarr/templates/_helpers.tpl:298-303) validates bothapp.kubernetes.io/nameandapp.kubernetes.io/instanceas reserved podLabels keys, but only the former is covered here.♻️ Proposed additional test
- it: should reject reserved selector label overrides set: podLabels: app.kubernetes.io/name: custom asserts: - failedTemplate: errorMessage: "podLabels must not override selector label app.kubernetes.io/name" + + - it: should reject reserved instance label overrides + set: + podLabels: + app.kubernetes.io/instance: custom + asserts: + - failedTemplate: + errorMessage: "podLabels must not override selector label app.kubernetes.io/instance"🤖 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/dolibarr/tests/validation_test.yaml` around lines 31 - 38, The validation coverage only checks the reserved podLabels override for app.kubernetes.io/name, but the _helpers.tpl selector-label guard also protects app.kubernetes.io/instance. Update the validation test in validation_test.yaml to add a case that sets podLabels.app.kubernetes.io/instance and asserts the same failedTemplate behavior, using the existing reserved-label validation pattern so both reserved keys are covered.
5-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo happy-path test for a fully valid configuration.
All tests in this suite assert
failedTemplate(plus onenotFailedTemplatefor null podLabels). Adding a positive test with validingress,gateway, andexternalSecretsvalues all populated together would guard against false-positive failures introduced by future edits todolibarr.validate.🤖 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/dolibarr/tests/validation_test.yaml` around lines 5 - 83, Add a happy-path validation test in validation_test.yaml for a fully valid configuration, since the current suite only checks failures and one null podLabels case. Create a new `it` block that uses valid `ingress`, `gateway`, and `externalSecrets` values together, and assert `notFailedTemplate` to cover the expected pass case. Use the existing test patterns around `failedTemplate`/`notFailedTemplate` in this suite to place the new scenario alongside the current `dolibarr.validate` coverage.
🤖 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/dolibarr/tests/validation_test.yaml`:
- Around line 31-38: The validation coverage only checks the reserved podLabels
override for app.kubernetes.io/name, but the _helpers.tpl selector-label guard
also protects app.kubernetes.io/instance. Update the validation test in
validation_test.yaml to add a case that sets
podLabels.app.kubernetes.io/instance and asserts the same failedTemplate
behavior, using the existing reserved-label validation pattern so both reserved
keys are covered.
- Around line 5-83: Add a happy-path validation test in validation_test.yaml for
a fully valid configuration, since the current suite only checks failures and
one null podLabels case. Create a new `it` block that uses valid `ingress`,
`gateway`, and `externalSecrets` values together, and assert `notFailedTemplate`
to cover the expected pass case. Use the existing test patterns around
`failedTemplate`/`notFailedTemplate` in this suite to place the new scenario
alongside the current `dolibarr.validate` coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5441b5db-ce63-46f8-94a1-f306b991db85
📒 Files selected for processing (3)
charts/dolibarr/templates/_helpers.tplcharts/dolibarr/templates/validate.yamlcharts/dolibarr/tests/validation_test.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/dolibarr/templates/_helpers.tpl
|
Addressed the latest CodeRabbit review-summary findings for Dolibarr. Changes:
Validation:
The CodeRabbit feedback was posted in a review summary, not a resolvable review thread, so there is no thread to resolve. |
## Summary - add `dolibarr` to the playground coverage allowlist required by the chart site-sync gate ## Validation - make site-sync-check CHART=dolibarr - npm run lint - npm run format:check - npm run build - make release-check REPO=site - make attribution-check REPO=site Chart PR: helmforgedev/charts#660 Issue: helmforgedev/charts#633 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for the `dolibarr` chart in the playground, making it available alongside other approved charts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Summary
dolibarr.validatehelper andvalidate.yamlentrypoint for fail-fast chart validationpodLabelsis unsetValidation
Site PR: helmforgedev/site#339
Issue: #633
Summary by CodeRabbit
Bug Fixes
Tests
podLabels: nullis accepted.