fix(memcached): align template standards#680
Conversation
github-actions
Bot
added
area:docs
area:schema
area:templates
area:tests
area:values
chart:memcached
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 (6)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesNetworkPolicy extraEgress feature
NOTES.txt restructuring
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🟢 Security Scan:
|
| Framework | Score |
|---|---|
| MITRE + NSA + SOC2 | 78.78788% |
✅ 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
Restore the Helm test under templates/
Because this hook manifest was moved to charts/memcached/tests, Helm will not render it as a chart test; Helm's Chart Tests docs state tests live under templates/, and I confirmed helm template test charts/memcached --namespace default now emits no helm.sh/hook: test pod while the parent commit did. As a result, helm test no longer runs the memcached connectivity check after install, so the hook manifest should remain under templates/tests/ and only the unit-test suites should live in charts/memcached/tests.
ℹ️ 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/memcached/values.schema.json (1)
267-270: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider nesting
extraEgressundernetworkPolicy.egressfor consistency.
extraEgressis placed as a top-levelnetworkPolicyproperty, but it only takes effect whennetworkPolicy.egress.enabledis true (pertemplates/networkpolicy.yamlline 57's outerif), and every other egress-related extension point (allowSameNamespace,allowDNS,allowHTTPS,extraTo) lives undernetworkPolicy.egress.*. This asymmetry could confuse users configuring egress rules, and this is the last easy point to fix it since the field is new/unreleased.Since this touches
values.schema.json,values.yaml,templates/networkpolicy.yaml,tests/networkpolicy_test.yaml, andREADME.md, consider renaming tonetworkPolicy.egress.extraEgressbefore release.🤖 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/memcached/values.schema.json` around lines 267 - 270, Move the new egress extension point to match the existing structure by nesting extraEgress under networkPolicy.egress instead of keeping it as a top-level networkPolicy field. Update the schema and defaults in values.schema.json and values.yaml, then adjust templates/networkpolicy.yaml to read networkPolicy.egress.extraEgress alongside allowSameNamespace, allowDNS, allowHTTPS, and extraTo, and finally update tests/networkpolicy_test.yaml and README.md to use the renamed path consistently.
🤖 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/memcached/values.schema.json`:
- Around line 267-270: Move the new egress extension point to match the existing
structure by nesting extraEgress under networkPolicy.egress instead of keeping
it as a top-level networkPolicy field. Update the schema and defaults in
values.schema.json and values.yaml, then adjust templates/networkpolicy.yaml to
read networkPolicy.egress.extraEgress alongside allowSameNamespace, allowDNS,
allowHTTPS, and extraTo, and finally update tests/networkpolicy_test.yaml and
README.md to use the renamed path consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c2be5588-b74a-43c7-92b6-1cedb4f5aa1f
📒 Files selected for processing (8)
charts/memcached/README.mdcharts/memcached/templates/NOTES.txtcharts/memcached/templates/networkpolicy.yamlcharts/memcached/tests/networkpolicy_test.yamlcharts/memcached/tests/test-connection.yamlcharts/memcached/tests/testconnection_test.yamlcharts/memcached/values.schema.jsoncharts/memcached/values.yaml
💤 Files with no reviewable changes (1)
- charts/memcached/tests/testconnection_test.yaml
mberlofa
force-pushed
the
fix/memcached-template-standards
branch
2 times, most recently
from
850be74 to
32b6b15
Compare
mberlofa
force-pushed
the
fix/memcached-template-standards
branch
from
32b6b15 to
1c300ac
Compare
|
Addressed the CodeRabbit review-summary note about the new What changed:
Validation:
This feedback was present in the CodeRabbit review summary rather than an unresolved review thread, so there is no thread ID to reply to or resolve. |
1 participant
Summary
templates/tests/sohelm testrenders and executes it, with unittest coverage preserved.networkPolicy.extraEgressso callers can append full custom egress rules.NOTES.txtsections and sync README values documentation.Related
Validation
helm template test charts/memcached | rg -n "helm.sh/hook|test-connection"(hook rendered fromtemplates/tests/test-connection.yaml)helm unittest charts/memcached(41 tests, 9 suites)make template-standards-check CHART=memcachednode scripts/charts/validate-chart.js --chart memcached --no-k3dmake validate-chart CHART=memcached TIMEOUT=900(FULLY VALIDATED, 19 layers)Summary by CodeRabbit
networkPolicy.egress.extraEgressto append additional egress rules to Memcached NetworkPolicy.networkPolicy.extraEgress.