ci: add helm-template-smoke job to catch chart-render parse errors#533
Closed
bussyjd wants to merge 1 commit into
Closed
ci: add helm-template-smoke job to catch chart-render parse errors#533bussyjd wants to merge 1 commit into
bussyjd wants to merge 1 commit into
Conversation
PR #527 fixed an unescaped {{ $labels }} in a PrometheusRule annotation that broke `helm upgrade base` on every `obol stack up`. The bug shipped to integration testing because go test ./... doesn't exercise Helm rendering. This job pipes the embedded base chart through `helm template` on every PR; parse errors fail the build before merge. - Runs against ./internal/embed/infrastructure/base - Uses helm v3.20.1 (matches obolup.sh pinned version) - Also runs `helm lint` for chart-structure issues - Substitutes {{OLLAMA_HOST_IP}}/{{CLUSTER_ID}} stubs in a temp copy of the chart (mirroring what `obol stack init` does via internal/defaults/defaults.go::InfrastructureReplacements) - Future: pair with a helmfile-lint job for state-value tests If we ever land a chart-template change that this doesn't catch, expand the helm-template invocation with --set values mimicking what `obol stack up` provides.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+17
to
+83
| name: helm template embedded chart | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 | ||
|
|
||
| - name: Set up Helm | ||
| uses: azure/setup-helm@1a275c3b69536ee54be43f2070a358922e12c8d4 # v4.3.1 | ||
| with: | ||
| version: v3.20.1 # match obolup.sh pinned version | ||
|
|
||
| - name: helm template ./base | ||
| run: | | ||
| # Render the embedded `base` chart and fail on Go-template parse | ||
| # errors. Catches bugs like the unescaped `{{ $labels }}` in | ||
| # PrometheusRule annotations that broke `helm upgrade base` on | ||
| # every `obol stack up` (see PR #527). `go test ./...` does not | ||
| # exercise Helm rendering, so this is the only pre-merge gate | ||
| # for chart parse errors. | ||
| # | ||
| # The base chart contains `{{PLACEHOLDER}}` strings (e.g. | ||
| # `{{OLLAMA_HOST_IP}}`, `{{CLUSTER_ID}}`) that are substituted | ||
| # by `internal/defaults/defaults.go::InfrastructureReplacements` | ||
| # before helmfile runs. Helm's Go-template parser would treat | ||
| # them as actions and fail, so we substitute stub values into | ||
| # a working copy first — mirroring what `obol stack init` does. | ||
| set -euo pipefail | ||
| workdir="$(mktemp -d)" | ||
| cp -R internal/embed/infrastructure/base "$workdir/base" | ||
| # Mirror internal/defaults InfrastructureReplacements with CI stubs. | ||
| find "$workdir/base" -type f -name '*.yaml' -print0 \ | ||
| | xargs -0 sed -i \ | ||
| -e 's/{{OLLAMA_HOST_IP}}/127.0.0.1/g' \ | ||
| -e 's/{{OLLAMA_HOST}}/localhost/g' \ | ||
| -e 's/{{CLUSTER_ID}}/ci-helm-smoke/g' | ||
| # Match values passed by helmfile.yaml `releases[base]`. | ||
| helm template base "$workdir/base" \ | ||
| --set dataDir=/data \ | ||
| --set network=mainnet \ | ||
| > /dev/null | ||
|
|
||
| - name: helm template ./cloudflared | ||
| run: | | ||
| # The cloudflared chart has no placeholder substitution and uses | ||
| # default values from values.yaml. | ||
| set -euo pipefail | ||
| helm template cloudflared internal/embed/infrastructure/cloudflared \ | ||
| > /dev/null | ||
|
|
||
| - name: helm lint ./base | ||
| run: | | ||
| set -euo pipefail | ||
| workdir="$(mktemp -d)" | ||
| cp -R internal/embed/infrastructure/base "$workdir/base" | ||
| find "$workdir/base" -type f -name '*.yaml' -print0 \ | ||
| | xargs -0 sed -i \ | ||
| -e 's/{{OLLAMA_HOST_IP}}/127.0.0.1/g' \ | ||
| -e 's/{{OLLAMA_HOST}}/localhost/g' \ | ||
| -e 's/{{CLUSTER_ID}}/ci-helm-smoke/g' | ||
| helm lint "$workdir/base" \ | ||
| --set dataDir=/data \ | ||
| --set network=mainnet | ||
|
|
||
| - name: helm lint ./cloudflared | ||
| run: | | ||
| set -euo pipefail | ||
| helm lint internal/embed/infrastructure/cloudflared |
6 tasks
Collaborator
Author
|
Superseded by bundle PR #536 — closing in favor of the consolidated merge target. Original branch and history preserved. |
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
2 participants
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
basechart throughhelm templateandhelm linton every PR touchinginternal/embed/infrastructure/**{{ \$labels }}in a PrometheusRule annotation fixed in fix(prometheus-rules): escape PromQL $labels for Helm rendering #527) thatgo test ./...cannot detect because unit tests don't exercise Helm renderingv3.20.1(matches the version pinned inobolup.sh)Why
PR #527 fixed an unescaped
{{ \$labels.X }}in a PrometheusRule annotation that madehelm upgrade base ./basefail on everyobol stack up:The bug shipped to integration testing because nothing in CI exercises
helm templateagainstinternal/embed/infrastructure/base. This PR closes that gap.What the job does (and doesn't) catch
Catches:
base/templates/*.yamlhelm lint)cloudflared/templates/*.yamlDoesn't catch (left for future jobs):
values/*.yaml.gotmpl(needshelmfile lint)helm/chart-testing-actionin the existinglint-test.yamlworkflow already handles top-level charts; this job complements it for the embedded chartImplementation notes
The
basechart contains{{OLLAMA_HOST_IP}},{{OLLAMA_HOST}}, and{{CLUSTER_ID}}placeholders that are not Helm template actions — they're substituted byinternal/defaults/defaults.go::InfrastructureReplacementsbeforehelmfile syncruns (seeobol stack init). Plainhelm templatechokes on them as undefined-variable actions, so the job copies the chart to a temp dir andsed-substitutes CI stubs first, mirroring the runtime flow.Test plan
{{ \$labels.X }}PrometheusRule annotation —helm templatefailed with the sameundefined variable "\$labels"error the bug originally produced