fix(hoppscotch): align template standards#675
Open
mberlofa wants to merge 2 commits into
Open
Conversation
github-actions
Bot
added
area:docs
area:schema
area:templates
area:tests
area:values
chart:hoppscotch
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 (14)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughAdds ChangesHoppscotch chart extraEgress and extraManifests support
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 | 87.62626% |
✅ 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
Because this hook manifest now lives in the chart-root tests/ directory instead of templates/tests/, Helm no longer renders it into the release manifest; I verified the previous chart emitted # Source: hoppscotch/templates/tests/connection-test.yaml, while the current chart emits no test-connection hook. In any installed release, helm test will therefore have no connection test to run, so this should stay under templates/tests/ and any helm-unittest coverage should be added as a separate *_test.yaml suite.
ℹ️ 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/hoppscotch/templates/extra-manifests.yaml (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
tplfor templated extra manifests.Entries under
extraManifestsare rendered as static YAML only; users can't reference.Release,.Values, or chart helpers inside their manifests. Wrapping withtplis the common pattern for this feature and would let consumers parameterize their extra manifests (e.g., namespace, release name) without hardcoding values.♻️ Suggested refactor
{{- range .Values.extraManifests }} --- -{{ toYaml . }} +{{ tpl (toYaml .) $ }} {{- end }}Not required for current CI usage (fixtures use fully static manifests), so this is optional.
🤖 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/hoppscotch/templates/extra-manifests.yaml` around lines 1 - 5, The extra manifests in extra-manifests.yaml are rendered only with toYaml, so templated values like .Release or .Values cannot be resolved. Update the rendering inside the extraManifests range to pass each item through tpl before outputting it, so consumers can parameterize manifests using chart context while preserving the existing static-manifest behavior. Use the extraManifests loop and the toYaml rendering as the locating symbols.
🤖 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/hoppscotch/templates/extra-manifests.yaml`:
- Around line 1-5: The extra manifests in extra-manifests.yaml are rendered only
with toYaml, so templated values like .Release or .Values cannot be resolved.
Update the rendering inside the extraManifests range to pass each item through
tpl before outputting it, so consumers can parameterize manifests using chart
context while preserving the existing static-manifest behavior. Use the
extraManifests loop and the toYaml rendering as the locating symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d1943b55-ce06-44ff-96f9-11402e86ae8b
📒 Files selected for processing (14)
charts/hoppscotch/README.mdcharts/hoppscotch/ci/dual-stack.yamlcharts/hoppscotch/ci/external-db.yamlcharts/hoppscotch/ci/external-secrets.yamlcharts/hoppscotch/templates/extra-manifests.yamlcharts/hoppscotch/templates/networkpolicy.yamlcharts/hoppscotch/tests/connection-test.yamlcharts/hoppscotch/tests/extra-manifests-values.yamlcharts/hoppscotch/tests/extra_manifests_test.yamlcharts/hoppscotch/tests/networkpolicy-extra-egress-values.yamlcharts/hoppscotch/tests/networkpolicy-legacy-and-extra-egress-values.yamlcharts/hoppscotch/tests/networkpolicy_test.yamlcharts/hoppscotch/values.schema.jsoncharts/hoppscotch/values.yaml
💤 Files with no reviewable changes (1)
- charts/hoppscotch/ci/dual-stack.yaml
mberlofa
force-pushed
the
fix/hoppscotch-template-standards
branch
from
03bbd71 to
fc5f429
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
charts/hoppscotch/tests/connection_test.yaml (1)
9-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider broadening assertion coverage.
The suite only validates kind, name, and the
helm.sh/hookannotation. The producer template also setshelm.sh/hook-delete-policy, awgethealth-check command, and container image — asserting on these would catch regressions in the hook's actual behavior, not just its shape.♻️ Suggested additional assertions
- equal: path: metadata.annotations["helm.sh/hook"] value: test + - equal: + path: metadata.annotations["helm.sh/hook-delete-policy"] + value: before-hook-creation,hook-succeeded + - matchRegex: + path: spec.containers[0].command[2] + pattern: /backend/health🤖 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/hoppscotch/tests/connection_test.yaml` around lines 9 - 18, The Helm test hook check in connection_test.yaml is too narrow and only verifies kind, name, and helm.sh/hook. Expand the test assertions in the existing it block to also cover the hook-delete-policy annotation, the wget health-check command, and the container image so the connection hook’s actual behavior is validated via the same template-generated Pod.charts/hoppscotch/templates/extra-manifests.yaml (1)
1-5: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
tpl-rendering entries for templating support.Currently
extraManifestsentries are only YAML-dumped verbatim, so users can't reference.Release.Namespace,.Chart.Name, or other chart values inside their manifests — a common convention for this kind of passthrough (e.g. Bitnami'sextraDeploy).♻️ Proposed refactor to support templating
{{/* SPDX-License-Identifier: Apache-2.0 */}} {{- range .Values.extraManifests }} --- -{{ toYaml . }} +{{ tpl (toYaml .) $ }} {{- end }}🤖 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/hoppscotch/templates/extra-manifests.yaml` around lines 1 - 5, `extraManifests` is currently emitted with a plain `toYaml` dump in the `range .Values.extraManifests` block, so chart values and release metadata cannot be interpolated. Update the `extra-manifests.yaml` template to run each entry through `tpl` before YAML rendering, using the chart context so fields like `.Release.Namespace` and `.Chart.Name` can be resolved. Keep the existing `extraManifests` loop and ensure the rendered result still emits valid YAML documents.
🤖 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/hoppscotch/templates/extra-manifests.yaml`:
- Around line 1-5: `extraManifests` is currently emitted with a plain `toYaml`
dump in the `range .Values.extraManifests` block, so chart values and release
metadata cannot be interpolated. Update the `extra-manifests.yaml` template to
run each entry through `tpl` before YAML rendering, using the chart context so
fields like `.Release.Namespace` and `.Chart.Name` can be resolved. Keep the
existing `extraManifests` loop and ensure the rendered result still emits valid
YAML documents.
In `@charts/hoppscotch/tests/connection_test.yaml`:
- Around line 9-18: The Helm test hook check in connection_test.yaml is too
narrow and only verifies kind, name, and helm.sh/hook. Expand the test
assertions in the existing it block to also cover the hook-delete-policy
annotation, the wget health-check command, and the container image so the
connection hook’s actual behavior is validated via the same template-generated
Pod.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 706313dd-c91d-4c3b-b79c-608a4a5c9e3c
📒 Files selected for processing (14)
charts/hoppscotch/README.mdcharts/hoppscotch/ci/dual-stack.yamlcharts/hoppscotch/ci/external-db.yamlcharts/hoppscotch/ci/external-secrets.yamlcharts/hoppscotch/templates/extra-manifests.yamlcharts/hoppscotch/templates/networkpolicy.yamlcharts/hoppscotch/tests/connection_test.yamlcharts/hoppscotch/tests/extra-manifests-values.yamlcharts/hoppscotch/tests/extra_manifests_test.yamlcharts/hoppscotch/tests/networkpolicy-extra-egress-values.yamlcharts/hoppscotch/tests/networkpolicy-legacy-and-extra-egress-values.yamlcharts/hoppscotch/tests/networkpolicy_test.yamlcharts/hoppscotch/values.schema.jsoncharts/hoppscotch/values.yaml
💤 Files with no reviewable changes (1)
- charts/hoppscotch/ci/dual-stack.yaml
✅ Files skipped from review due to trivial changes (4)
- charts/hoppscotch/tests/networkpolicy-legacy-and-extra-egress-values.yaml
- charts/hoppscotch/tests/extra_manifests_test.yaml
- charts/hoppscotch/tests/extra-manifests-values.yaml
- charts/hoppscotch/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- charts/hoppscotch/tests/networkpolicy-extra-egress-values.yaml
- charts/hoppscotch/templates/networkpolicy.yaml
- charts/hoppscotch/tests/networkpolicy_test.yaml
- charts/hoppscotch/ci/external-db.yaml
- charts/hoppscotch/values.schema.json
- charts/hoppscotch/values.yaml
mberlofa
force-pushed
the
fix/hoppscotch-template-standards
branch
from
fc5f429 to
c67b0a4
Compare
Contributor
Author
|
Addressed the CodeRabbit review-summary items for Hoppscotch. What changed:
Validation:
These findings were present in CodeRabbit review summaries rather than unresolved review threads, so there are no thread IDs 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
templates/tests/connection-test.yamlsohelm testrenders and executes it, with unittest coverage for the hook pod.networkPolicy.extraEgresswhile preserving legacynetworkPolicy.egress.extraManifestsand unit coverage for additional rendered manifests.Related
Validation
helm template test charts/hoppscotch | rg -n "helm.sh/hook|test-connection|connection-test"(hook rendered fromtemplates/tests/connection-test.yaml)helm unittest charts/hoppscotch(81 tests, 14 suites)make template-standards-check CHART=hoppscotchnode scripts/charts/validate-chart.js --chart hoppscotch --no-k3dmake validate-chart CHART=hoppscotch TIMEOUT=900(FULLY VALIDATED, 18 layers)make site-sync-check CHART=hoppscotchmake release-check REPO=chartsmake attribution-check REPO=chartsSite Validation
npm run lintnpm run format:checknpm run buildSummary by CodeRabbit
extraManifeststo render additional Kubernetes resources during the Helm release (with templating support).networkPolicy.extraEgressto append extra egress rules to generated NetworkPolicies.values.schema.jsonforextraManifestsandextraEgress.