fix(fastmcp-server): align template standards#665
Open
mberlofa wants to merge 3 commits into
Open
Conversation
github-actions
Bot
added
area:schema
area:templates
area:tests
area:values
chart:fastmcp-server
Contributor
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 (11)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds shared Helm validation for chart values, removes inline HTTPRoute parentRefs validation, adds NetworkPolicy egress support, and reformats NOTES output into numbered sections. ChangesChart validation, gateway route checks, and notes updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
🟢 Security Scan:
|
| Framework | Score |
|---|---|
| MITRE + NSA + SOC2 | 90.90909% |
✅ 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: 75fabd34f3
ℹ️ 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
charts/fastmcp-server/templates/networkpolicy.yaml (1)
17-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor: duplicate truthiness check on
extraEgress.
.Values.networkPolicy.extraEgressis evaluated twice (once viaiffor the policyType, once viawithfor the egress block). Could consolidate into a singlewithblock that sets both the policyType and the egress rules to avoid the duplicated condition.♻️ Optional consolidation
policyTypes: - Ingress - {{- if .Values.networkPolicy.extraEgress }} - - Egress - {{- end }} + {{- if .Values.networkPolicy.extraEgress }} + - Egress + {{- end }} ingress: - ports: - port: {{ .Values.server.port }} protocol: TCP {{- with .Values.networkPolicy.extraEgress }} egress: {{- toYaml . | nindent 4 }} {{- end }}Not blocking, purely stylistic; logic is otherwise correct.
🤖 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/fastmcp-server/templates/networkpolicy.yaml` around lines 17 - 27, The NetworkPolicy template currently checks .Values.networkPolicy.extraEgress twice, once to add the Egress policy type and again to render the egress rules. Consolidate this logic in the networkpolicy.yaml template by using a single with block around extraEgress and have it emit both the policyType entry and the egress section from the same condition, keeping the behavior in sync and reducing duplicated truthiness checks.
🤖 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/fastmcp-server/templates/_helpers.tpl`:
- Around line 108-112: The `gatewayAPI.parentRefs` validation in
`fastmcp-server.validate` is running even when `gatewayAPI.enabled` is false, so
gate the `range .Values.gatewayAPI.parentRefs` / `fail` check behind the enabled
flag in `_helpers.tpl`. Also update the validation coverage by adding a test
case for the disabled path in the `templates/validate.yaml`-driven tests to
ensure leftover `parentRefs` entries do not fail when
`gatewayAPI.enabled=false`.
In `@charts/fastmcp-server/templates/NOTES.txt`:
- Around line 37-40: The Gateway API routes section in NOTES.txt is using the
wrong TLS source for the URL scheme. Update the template logic around the
gatewayAPI hostname range so it does not depend on `.Values.ingress.tls`;
instead use the Gateway API TLS configuration if available, or leave the scheme
unspecified when Gateway API TLS is not modeled separately. Keep the change
localized to the gatewayAPI notes rendering and the hostname/path output.
---
Nitpick comments:
In `@charts/fastmcp-server/templates/networkpolicy.yaml`:
- Around line 17-27: The NetworkPolicy template currently checks
.Values.networkPolicy.extraEgress twice, once to add the Egress policy type and
again to render the egress rules. Consolidate this logic in the
networkpolicy.yaml template by using a single with block around extraEgress and
have it emit both the policyType entry and the egress section from the same
condition, keeping the behavior in sync and reducing duplicated truthiness
checks.
🪄 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: 92a8ab29-ae01-471d-9d57-3875867b506d
📒 Files selected for processing (10)
charts/fastmcp-server/templates/NOTES.txtcharts/fastmcp-server/templates/_helpers.tplcharts/fastmcp-server/templates/httproute.yamlcharts/fastmcp-server/templates/networkpolicy.yamlcharts/fastmcp-server/templates/validate.yamlcharts/fastmcp-server/tests/httproute_test.yamlcharts/fastmcp-server/tests/networkpolicy_test.yamlcharts/fastmcp-server/tests/validation_test.yamlcharts/fastmcp-server/values.schema.jsoncharts/fastmcp-server/values.yaml
💤 Files with no reviewable changes (2)
- charts/fastmcp-server/templates/httproute.yaml
- charts/fastmcp-server/tests/httproute_test.yaml
mberlofa
force-pushed
the
fix/fastmcp-server-template-standards
branch
from
149c5ba to
ed14e45
Compare
mberlofa
force-pushed
the
fix/fastmcp-server-template-standards
branch
from
ed14e45 to
153fe37
Compare
Contributor
Author
|
Reviewed the remaining CodeRabbit feedback across review threads and the review summary. Current status:
Validation:
Note: the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fastmcp-server.validatechecks for invalid ingress, Gateway API, auth, source, autoscaling, and selector-label configurationsnetworkPolicy.extraEgresssupport and schema coverageValidation
Site PR: helmforgedev/site#343
Issue: #633
Summary by CodeRabbit