fix(github-mcp-server): align template standards#668
Open
mberlofa wants to merge 3 commits into
Open
Conversation
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 (12)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR centralizes chart validation, makes ingress and network policy rendering conditional, updates deployment labels, and refreshes chart notes and install documentation. Tests and schema/default values were updated to match the new behavior. Changesgithub-mcp-server chart 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 | 75.757576% |
✅ 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: 09ce38d862
ℹ️ 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: 1
🧹 Nitpick comments (2)
charts/github-mcp-server/templates/NOTES.txt (1)
29-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider mentioning
networkPolicy.extraEgressin Security guidance.The Security section documents
networkPolicy.ingressFrombut not the newly addedextraEgressoption, which is also security-relevant for restricting outbound traffic.📝 Suggested addition
- Enable networkPolicy.enabled and set networkPolicy.ingressFrom for restricted clients. +- Use networkPolicy.extraEgress to restrict outbound traffic to required destinations only.🤖 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/github-mcp-server/templates/NOTES.txt` around lines 29 - 34, The Security guidance in NOTES.txt should also mention the newly added networkPolicy.extraEgress setting alongside networkPolicy.enabled and networkPolicy.ingressFrom. Update the Security section near the existing networkPolicy references to note that extraEgress can be used to restrict outbound traffic, keeping the wording consistent with the other security recommendations.charts/github-mcp-server/tests/validation_test.yaml (1)
20-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd symmetric test for
app.kubernetes.io/instanceoverride.This covers the
namekey branch of the new podLabels guard; the siblinginstancekey branch (helpers.tpl lines 41-43) has no test.♻️ Suggested addition
- it: fails when podLabels override selector labels set: podLabels: app.kubernetes.io/name: custom asserts: - failedTemplate: errorMessage: "podLabels must not override the selector label app.kubernetes.io/name" + - it: fails when podLabels override the selector instance label + set: + podLabels: + app.kubernetes.io/instance: custom + asserts: + - failedTemplate: + errorMessage: "podLabels must not override the 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/github-mcp-server/tests/validation_test.yaml` around lines 20 - 26, Add a symmetric validation test for the podLabels guard covering app.kubernetes.io/instance, since the current validation only exercises the app.kubernetes.io/name branch. Update the tests around validation_test.yaml to add a failing case that sets podLabels.app.kubernetes.io/instance and asserts the same kind of failedTemplate error message, matching the behavior enforced in the helpers.tpl podLabels selector 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/github-mcp-server/templates/_helpers.tpl`:
- Around line 34-36: The validation in _helpers.tpl is too strict because it
checks persistence.accessModes even when persistence.existingClaim is set.
Update the replicaCount/persistence guard so the fail only runs when the chart
is actually creating a PVC, matching the pvc.yaml behavior that skips
accessModes for existingClaim. Use the same symbols here, especially
replicaCount, persistence.enabled, persistence.existingClaim, and
persistence.accessModes, to ensure users with an external RWX claim are not
blocked.
---
Nitpick comments:
In `@charts/github-mcp-server/templates/NOTES.txt`:
- Around line 29-34: The Security guidance in NOTES.txt should also mention the
newly added networkPolicy.extraEgress setting alongside networkPolicy.enabled
and networkPolicy.ingressFrom. Update the Security section near the existing
networkPolicy references to note that extraEgress can be used to restrict
outbound traffic, keeping the wording consistent with the other security
recommendations.
In `@charts/github-mcp-server/tests/validation_test.yaml`:
- Around line 20-26: Add a symmetric validation test for the podLabels guard
covering app.kubernetes.io/instance, since the current validation only exercises
the app.kubernetes.io/name branch. Update the tests around validation_test.yaml
to add a failing case that sets podLabels.app.kubernetes.io/instance and asserts
the same kind of failedTemplate error message, matching the behavior enforced in
the helpers.tpl podLabels selector 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: 5249a15e-a96c-4106-b1b9-0239c00adb2d
📒 Files selected for processing (12)
charts/github-mcp-server/README.mdcharts/github-mcp-server/templates/NOTES.txtcharts/github-mcp-server/templates/_helpers.tplcharts/github-mcp-server/templates/deployment.yamlcharts/github-mcp-server/templates/ingress.yamlcharts/github-mcp-server/templates/networkpolicy.yamlcharts/github-mcp-server/templates/validate.yamlcharts/github-mcp-server/tests/networkpolicy_test.yamlcharts/github-mcp-server/tests/templates_test.yamlcharts/github-mcp-server/tests/validation_test.yamlcharts/github-mcp-server/values.schema.jsoncharts/github-mcp-server/values.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/github-mcp-server/tests/validation_test.yaml (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding coverage for the
app.kubernetes.io/instanceoverride branch.The helper also fails when
podLabelsoverridesapp.kubernetes.io/instance, but only theapp.kubernetes.io/namebranch is tested here.♻️ Proposed additional test case
- it: fails when podLabels override selector labels set: podLabels: app.kubernetes.io/name: custom asserts: - failedTemplate: errorMessage: "podLabels must not override the selector label app.kubernetes.io/name" + - it: fails when podLabels override the instance selector label + set: + podLabels: + app.kubernetes.io/instance: custom + asserts: + - failedTemplate: + errorMessage: "podLabels must not override the 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/github-mcp-server/tests/validation_test.yaml` around lines 28 - 34, The validation tests only cover the selector-label override path for app.kubernetes.io/name, but the same helper also rejects app.kubernetes.io/instance overrides. Add a new case alongside the existing validation in validation_test.yaml that uses podLabels to override app.kubernetes.io/instance and asserts the same failedTemplate behavior, so both branches of the chart validation are covered.
🤖 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/github-mcp-server/tests/validation_test.yaml`:
- Around line 28-34: The validation tests only cover the selector-label override
path for app.kubernetes.io/name, but the same helper also rejects
app.kubernetes.io/instance overrides. Add a new case alongside the existing
validation in validation_test.yaml that uses podLabels to override
app.kubernetes.io/instance and asserts the same failedTemplate behavior, so both
branches of the chart validation are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a268cf2-225c-48dc-99e7-e01b0bdcba84
📒 Files selected for processing (7)
charts/github-mcp-server/README.mdcharts/github-mcp-server/templates/_helpers.tplcharts/github-mcp-server/templates/networkpolicy.yamlcharts/github-mcp-server/tests/networkpolicy_test.yamlcharts/github-mcp-server/tests/validation_test.yamlcharts/github-mcp-server/values.schema.jsoncharts/github-mcp-server/values.yaml
✅ Files skipped from review due to trivial changes (1)
- charts/github-mcp-server/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- charts/github-mcp-server/values.yaml
- charts/github-mcp-server/templates/networkpolicy.yaml
- charts/github-mcp-server/tests/networkpolicy_test.yaml
- charts/github-mcp-server/templates/_helpers.tpl
- charts/github-mcp-server/values.schema.json
mberlofa
force-pushed
the
fix/github-mcp-server-template-standards
branch
from
4e77237 to
fa46a72
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/github-mcp-server/templates/NOTES.txt (1)
29-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider mentioning
networkPolicy.extraEgressin the Security/Networking guidance.This section now covers
networkPolicy.enabled/ingressFrombut omits the newly addedextraEgresssupport. Since default-deny egress policies can silently block traffic to dependencies the user hasn't allow-listed, a pointer here could reduce troubleshooting friction for operators enabling the policy.✏️ Optional wording addition
- Enable networkPolicy.enabled and set networkPolicy.ingressFrom for restricted clients. +- If networkPolicy.enabled=true, add required destinations via networkPolicy.extraEgress (DNS/HTTPS are allowed by default).🤖 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/github-mcp-server/templates/NOTES.txt` around lines 29 - 41, The Security/Networking guidance in NOTES.txt mentions networkPolicy.enabled and networkPolicy.ingressFrom but omits the newly added networkPolicy.extraEgress setting. Update the Security section to reference networkPolicy.extraEgress alongside the existing networkPolicy guidance, so operators know to allow-list outbound dependencies when using default-deny egress policies and can find the relevant configuration option easily.
🤖 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/github-mcp-server/templates/NOTES.txt`:
- Around line 29-41: The Security/Networking guidance in NOTES.txt mentions
networkPolicy.enabled and networkPolicy.ingressFrom but omits the newly added
networkPolicy.extraEgress setting. Update the Security section to reference
networkPolicy.extraEgress alongside the existing networkPolicy guidance, so
operators know to allow-list outbound dependencies when using default-deny
egress policies and can find the relevant configuration option easily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f2d3a8b-6524-4586-91ec-fb7855f9698a
📒 Files selected for processing (12)
charts/github-mcp-server/README.mdcharts/github-mcp-server/templates/NOTES.txtcharts/github-mcp-server/templates/_helpers.tplcharts/github-mcp-server/templates/deployment.yamlcharts/github-mcp-server/templates/ingress.yamlcharts/github-mcp-server/templates/networkpolicy.yamlcharts/github-mcp-server/templates/validate.yamlcharts/github-mcp-server/tests/networkpolicy_test.yamlcharts/github-mcp-server/tests/templates_test.yamlcharts/github-mcp-server/tests/validation_test.yamlcharts/github-mcp-server/values.schema.jsoncharts/github-mcp-server/values.yaml
✅ Files skipped from review due to trivial changes (2)
- charts/github-mcp-server/values.yaml
- charts/github-mcp-server/README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- charts/github-mcp-server/values.schema.json
- charts/github-mcp-server/templates/deployment.yaml
- charts/github-mcp-server/tests/templates_test.yaml
- charts/github-mcp-server/tests/networkpolicy_test.yaml
- charts/github-mcp-server/templates/ingress.yaml
- charts/github-mcp-server/tests/validation_test.yaml
- charts/github-mcp-server/templates/_helpers.tpl
- charts/github-mcp-server/templates/networkpolicy.yaml
mberlofa
force-pushed
the
fix/github-mcp-server-template-standards
branch
from
fa46a72 to
203e221
Compare
Contributor
Author
|
Addressed the still-valid CodeRabbit nitpicks from the later review bodies. What changed:
Validation:
The remaining items were posted in CodeRabbit review bodies/top-level summaries. Existing review threads were already resolved/confirmed, so there is no new unresolved thread ID to reply to or resolve. |
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
ingressClassNamerendering for empty valuesnetworkPolicy.extraEgresssupport with built-in DNS/HTTPS allowances, schema updates, and unit coveragegithub-mcp-server.validateand align NOTES with numbered HelmForge sectionsValidation
Site PR: helmforgedev/site#346
Issue: #633
Summary by CodeRabbit
networkPolicy.extraEgressto append custom egress rules after built-in DNS/HTTPS allowances.spec.ingressClassNamewhen the value is empty.podLabelsfrom overriding selector labels.