fix(metrics-server): align template standards#681
Conversation
|
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 (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds ChangesNetworkPolicy extraEgress feature
NOTES.txt heading restructure
Estimated code review effort: 2 (Simple) | ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 | 89.898994% |
✅ 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
Keep the Helm test hook under templates
With this file now under charts/metrics-server/tests/, it is no longer part of Helm's template set; I checked helm template test charts/metrics-server --show-only tests/test-connection.yaml, which reports could not find template, and Helm's generated chart layout puts test hooks in templates/tests/test-connection.yaml. As a result, default installs won't store any helm.sh/hook: test pod, so helm test has nothing to run even though the chart still documents and configures a Helm test.
ℹ️ 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 (2)
charts/metrics-server/README.md (1)
92-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
egress.enableddependency.
extraEgressonly renders whennetworkPolicy.egress.enabled: true(per the template gating). Worth noting that in the description so users don't set it alone and get no output.📝 Suggested doc tweak
-| `networkPolicy.extraEgress` | `[]` | Append full custom NetworkPolicy egress rules | +| `networkPolicy.extraEgress` | `[]` | Append full custom NetworkPolicy egress rules (requires `networkPolicy.egress.enabled: true`) |🤖 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/metrics-server/README.md` at line 92, Document the dependency for networkPolicy.extraEgress in the README table: note that it only renders when networkPolicy.egress.enabled is true, matching the template gating. Update the description near the networkPolicy.extraEgress entry so users know to enable egress first; use the networkPolicy.egress.enabled and networkPolicy.extraEgress identifiers to locate the spot.charts/metrics-server/values.yaml (1)
168-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInconsistent placement of
extraEgress.
extraEgresssits at thenetworkPolicytop level, but its siblingextraTolives undernetworkPolicy.egress, andextraFromlives undernetworkPolicy.ingress. The template also only rendersextraEgresswhennetworkPolicy.egress.enabled: true(it's evaluated inside that gate), so its effective scope is already tied toegress, but its placement breaks that convention and can confuse users configuring it withoutegress.enabled: true.Consider nesting it as
networkPolicy.egress.extraEgressfor consistency, since the chart hasn't been released with this field yet.♻️ Suggested restructure
networkPolicy: enabled: false - extraEgress: [] ingress: ... egress: enabled: false + extraEgress: [] ... extraTo: []Would also require updating
templates/networkpolicy.yaml(.Values.networkPolicy.extraEgress→.Values.networkPolicy.egress.extraEgress) andvalues.schema.json.🤖 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/metrics-server/values.yaml` around lines 168 - 171, `extraEgress` is placed at the top level of `networkPolicy` while its related knobs live under `networkPolicy.egress` and `networkPolicy.ingress`. Move this setting to `networkPolicy.egress.extraEgress` for consistency, then update `templates/networkpolicy.yaml` to read from `.Values.networkPolicy.egress.extraEgress` and adjust `values.schema.json` accordingly. Use the existing `networkPolicy`, `egress`, and `ingress` value blocks as the main anchors when relocating the field.
🤖 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/metrics-server/README.md`:
- Line 92: Document the dependency for networkPolicy.extraEgress in the README
table: note that it only renders when networkPolicy.egress.enabled is true,
matching the template gating. Update the description near the
networkPolicy.extraEgress entry so users know to enable egress first; use the
networkPolicy.egress.enabled and networkPolicy.extraEgress identifiers to locate
the spot.
In `@charts/metrics-server/values.yaml`:
- Around line 168-171: `extraEgress` is placed at the top level of
`networkPolicy` while its related knobs live under `networkPolicy.egress` and
`networkPolicy.ingress`. Move this setting to `networkPolicy.egress.extraEgress`
for consistency, then update `templates/networkpolicy.yaml` to read from
`.Values.networkPolicy.egress.extraEgress` and adjust `values.schema.json`
accordingly. Use the existing `networkPolicy`, `egress`, and `ingress` value
blocks as the main anchors when relocating the field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f649a5df-8e8e-452e-9b4e-168106171d26
📒 Files selected for processing (7)
charts/metrics-server/README.mdcharts/metrics-server/templates/NOTES.txtcharts/metrics-server/templates/networkpolicy.yamlcharts/metrics-server/tests/operations_test.yamlcharts/metrics-server/tests/test-connection.yamlcharts/metrics-server/values.schema.jsoncharts/metrics-server/values.yaml
mberlofa
force-pushed
the
fix/metrics-server-template-standards
branch
from
6c75326 to
786c15a
Compare
mberlofa
force-pushed
the
fix/metrics-server-template-standards
branch
from
786c15a to
6abb962
Compare
|
Addressed the still-valid CodeRabbit review-body notes:
Validation completed after the fix:
These CodeRabbit notes were in the review summary body, not as separate resolvable review threads, so there was no thread ID to resolve. |
1 participant
Summary
templates/testssohelm testremains functionalValidation
make validate-chart CHART=metrics-server TIMEOUT=900: FULLY VALIDATED (16 layers), including k3d behavioral scenarios for default, dual-stack, HA, host-network, k3d, networkpolicy, and servicemonitor.make site-sync-check CHART=metrics-servermake release-check REPO=chartspassed with the expected GR-077 post-merge release confirmation warning.make attribution-check REPO=chartsSite PR: helmforgedev/site#359
Issue: #633
Summary by CodeRabbit
networkPolicy.egress.extraEgressto let you append custom NetworkPolicy egress rules (defaults to[]).networkPolicy.extraEgressis included in the rendered egress rules.