Repository navigation
CI: Make helm-docs.sh portable - #954
Open
marcleblanc2 wants to merge 5 commits into
Open
marcleblanc2 wants to merge 5 commits into
marcleblanc2 wants to merge 5 commits into
Conversation
…exist The script is run by developers, by Buildkite's helm-docs verify step, and (after sourcegraph/sourcegraph#16450) by the release worker, whose wolfi image only has busybox. It required bash for no reason it used: - #!/usr/bin/env bash and set -o pipefail: the script has no pipelines. - mktemp -d -t helm-docs.XXX: not POSIX, and every implementation reads it differently. GNU wants at least three X, busybox wants six (it fails with 'mktemp: : Invalid argument'), and BSD/macOS treats -t as a prefix and does not substitute the X at all. mktemp -d "${TMPDIR:-/tmp}/helm-docs.XXXXXX" behaves the same on all three. - ${0%/*}: fails with 'cd helm-docs.sh' when run as 'sh helm-docs.sh' from inside scripts/. dirname is POSIX and returns '.' in that case. - tar zf FILE -x MEMBER: old-style bunched options. tar -xzf FILE MEMBER is what GNU, BSD, and busybox tar all accept. Also remove the temp dir after extracting, and send the unsupported OS/arch messages to stderr. Verified: dash, macOS /bin/sh (bash 3.2), zsh, and busybox v1.36.1 sh (with PATH limited to busybox applets plus curl) all download once cold, run from cache warm, and work when invoked from inside scripts/. Regenerating the charts leaves git status clean. shellcheck -s sh and checkbashisms are clean. Amp-Thread-ID: https://ampcode.com/threads/T-01a0f388-6a21-7173-93ec-fdd98150d75d Co-authored-by: Amp <amp@ampcode.com>
Buildkite's helm-docs verify step and AGENTS.md each carried their own copy of "regenerate, then fail if the tree changed", both written as bash ([[ -z $(git status -s) ]]). --check moves that into the script: it runs helm-docs, then fails and lists the stale files if any charts/*/README.md differs from the index. It looks only at chart READMEs, so a developer's unrelated uncommitted edits do not fail it. The script now cd's to the repository root first. helm-docs scans the cwd for charts and reads .helmdocsignore from there, so run from anywhere else it found no charts, regenerated nothing, and --check would pass while the READMEs were stale. Verified with dash and macOS /bin/sh: --check passes on a clean tree, fails with exit 1 naming charts/sourcegraph/README.md after a values.yaml change, ignores a dirty values.yaml on its own, and behaves the same from /tmp and from inside scripts/. Arguments still pass through to helm-docs. Amp-Thread-ID: https://ampcode.com/threads/T-01a0f388-6a21-7173-93ec-fdd98150d75d Co-authored-by: Amp <amp@ampcode.com>
marcleblanc2
marked this pull request as draft
September 30, 2026 21:38
--check used git status to find READMEs helm-docs rewrote, which tied it to having git installed and a real checkout. Snapshot cksum of every charts/**/README.md before helm-docs runs and compare after; list the paths whose checksum line is missing from the snapshot. find, cksum, and grep -xF are POSIX and busybox applets, so --check now works anywhere the script does, including the release worker's tarball checkout. This also compares against the tree before the run rather than the index, so a developer who has regenerated but not yet committed a README gets a pass instead of a failure until they commit. In CI the tree is clean before the run, so the result is unchanged. Verified with dash and macOS /bin/sh, including a PATH containing no git: clean tree exits 0; one or several values.yaml changes exit 1 listing exactly the affected READMEs; a second run after regeneration exits 0; same results from /tmp. Amp-Thread-ID: https://ampcode.com/threads/T-01a0f388-6a21-7173-93ec-fdd98150d75d Co-authored-by: Amp <amp@ampcode.com>
The before/after checksum now runs on every invocation, not only under --check, and the paths helm-docs rewrote go to stdout one per line (helm-docs itself logs to stderr, so stdout was empty). The release worker in sourcegraph/sourcegraph can commit exactly those paths instead of keeping its own before/after snapshot of charts/**/README.md. --check is unchanged apart from the hint wording: it still exits 1 when the list is non-empty. Amp-Thread-ID: https://ampcode.com/threads/T-01a0f388-6a21-7173-93ec-fdd98150d75d Co-authored-by: Amp <amp@ampcode.com>
marcleblanc2
marked this pull request as ready for review
October 2, 2026 00:05
michaellzc
pushed a commit
that referenced
this pull request
Oct 21, 2026
Every release rewrites the image digests and registry in `values.yaml` and the chart version in `Chart.yaml`, and all three show up in the generated READMEs. That forced the release worker to regenerate the READMEs with its own copy of helm-docs. When that copy drifted from `scripts/helm-docs.sh`, `internal/release-v8.1.0` failed ":book: Verify helm-docs is up-to-date" and had to be fixed by hand. The regeneration also doesn't reach customers. The publish Action packages the chart from the first push to `promote/release-*`, before the docs commit lands, so the published `sourcegraph-8.0.0.tgz` README lists the internal registry as the default. This PR makes the READMEs independent of everything a release changes, so the release worker can drop its helm-docs step entirely (sourcegraph/sourcegraph#17056): - Every pinned image `defaultTag` gets `` # @default -- `<appVersion>@sha256:<digest>`, pinned by each release ``, and `sourcegraph.image.repository` gets `# @default -- Sourcegraph's public Docker Hub registry`, in the four charts the release updates. The two executor images also get the `# -- ` description helm-docs needs before it honors `@default`. The registry annotation deliberately avoids the literal registry string, because the release replaces that string everywhere in `values.yaml`, comments included. - The migrator README uses a `<chart-version>` placeholder instead of `{{ template "chart.version" . }}`. - A new CI step fails if any chart README renders a pinned digest or `index.docker.io/sourcegraph`. A new image added without an annotation then fails on its own PR rather than on the next release branch. It is a separate step so it doesn't conflict with #954, which rewrites the existing helm-docs step. - Published charts still list the exact values. Before `helm package`, the release and insiders publish workflows run the new `scripts/ci/render-published-readmes.sh`. It deletes the release-value `@default` annotations and reruns `./scripts/helm-docs.sh`, so each published README lists the same digests and registry as the `values.yaml` packaged with it. Nothing is committed. Because the docs are built from the commit being published, the README no longer depends on a later docs commit landing first, which was the cause of the wrong v8.0.0 README. The step is `continue-on-error`: if it fails (for example, the helm-docs download from GitHub fails), the chart still publishes with the committed placeholder README. The tradeoff is that READMEs viewed in this repository show placeholders instead of exact digests. The branch's `values.yaml` still has them, and the README in the published chart (`helm show readme`) shows them in full. The migrator's `helm upgrade` examples keep the `<chart-version>` placeholder in the published chart too. ## How this was tested - **Simulated release.** CI can't exercise a release, so I reproduced one on a scratch worktree. A temporary test in the sourcegraph repo ran the release worker's own `values.yaml` rewrite (`dev/images.UpdateHelmManifestContent`) over the four charts the release touches. It first rewrote them to the internal registry with new tags and digests (the internal phase), then back to the public registry (the promote phase). I bumped `Chart.yaml` `version`/`appVersion` with the same regex as `UpdateChartVersions`, and ran `./scripts/helm-docs.sh` after each phase. | | README lines changed after internal phase | after promote phase | |---|---|---| | `main` | 68 | 60 | | this branch | 0 | 0 | `values.yaml` and `Chart.yaml` changed as expected in both runs. Zero drift on this branch also shows that the release rewrite leaves the `# @default --` comments intact. - **Annotation coverage.** After regenerating, no chart README contains a 64-character `@sha256:` digest or `index.docker.io/sourcegraph`. A second `./scripts/helm-docs.sh` run leaves the tree clean. - **New CI guard.** I ran the new step's command against both trees. It exits 0 on this branch. On `main` it exits 1 and lists the offending lines, for example `sourcegraph.image.repository` in `charts/sourcegraph-executor/dind/README.md`. - **Publish rendering, end to end.** In a scratch worktree I rebuilt a promote branch the way a release would: internal phase, `Chart.yaml` bump, promote phase. Buildkite's helm-docs check stays clean on that tree. I then ran the publish job's steps with Helm v3.4.0, the version the workflows pin: the new script, then both `helm package` loops. I extracted each `.tgz` and compared its README with its packaged `values.yaml`. The expected values were taken from `values.yaml`, not from helm-docs output. | Scenario | Digests in packaged `values.yaml` | Missing from packaged README | Placeholders left | Registry row | |---|---|---|---|---| | Release publish (4 charts) | 24 / 1 / 1 / 1 | 0 | 0 | `"index.docker.io/sourcegraph"` | | Insiders publish (4 charts) | 24 / 1 / 1 / 1 | 0 | 0 | `"index.docker.io/sourcegraph"` | | Render step forced to fail | 24 / 1 / 1 / 1 | all | all | placeholder; packaging still succeeds | For every chart, in all three scenarios, `helm template` of the packaged chart matches the unmodified chart, after normalizing the migrator Job's random name suffix. The script only touches comments and READMEs. The `sourcegraph` chart has 25 annotated images but 24 distinct digests, because `codeIntelDB` and `pgsql` share one. `shellcheck` is clean, and `actionlint` reports the same three pre-existing `actions/checkout@v3` findings as on `main`. - **CI.** On the first commit, [Buildkite build 3049](https://buildkite.com/sourcegraph/deploy-sourcegraph-helm/builds/3049) passed, including ":book: Verify helm-docs is up-to-date" and the new ":book: Verify READMEs do not render release-managed values" step. "Lint and Test charts" also passed. The changes to `values.yaml` are comments only, so rendered manifests are unchanged. The publish workflows only run on pushes to `main`, `release/**` and `promote/release-**`, so this PR's CI can't exercise them. The end-to-end run above stands in for that. The first real check of the full flow will be the next release once both PRs are deployed. Merge this, and backport it to every `X.Y.x` branch that still gets patch releases (at least `8.0.x` and `8.1.x`), before the release worker change deploys. Patch releases branch from `X.Y.x`, and a branch without these annotations would fail the helm-docs check once the worker stops regenerating READMEs. Part of [PLAT-876](https://linear.app/sourcegraph/issue/PLAT-876/find-a-permanent-fix-for-helm-docs-release-failures).
michaellzc
pushed a commit
that referenced
this pull request
Oct 21, 2026
…#973) Backport c36d89e from #963 Every release rewrites the image digests and registry in `values.yaml` and the chart version in `Chart.yaml`, and all three show up in the generated READMEs. That forced the release worker to regenerate the READMEs with its own copy of helm-docs. When that copy drifted from `scripts/helm-docs.sh`, `internal/release-v8.1.0` failed ":book: Verify helm-docs is up-to-date" and had to be fixed by hand. The regeneration also doesn't reach customers. The publish Action packages the chart from the first push to `promote/release-*`, before the docs commit lands, so the published `sourcegraph-8.0.0.tgz` README lists the internal registry as the default. This PR makes the READMEs independent of everything a release changes, so the release worker can drop its helm-docs step entirely (sourcegraph/sourcegraph#17056): - Every pinned image `defaultTag` gets `` # @default -- `<appVersion>@sha256:<digest>`, pinned by each release ``, and `sourcegraph.image.repository` gets `# @default -- Sourcegraph's public Docker Hub registry`, in the four charts the release updates. The two executor images also get the `# -- ` description helm-docs needs before it honors `@default`. The registry annotation deliberately avoids the literal registry string, because the release replaces that string everywhere in `values.yaml`, comments included. - The migrator README uses a `<chart-version>` placeholder instead of `{{ template "chart.version" . }}`. - A new CI step fails if any chart README renders a pinned digest or `index.docker.io/sourcegraph`. A new image added without an annotation then fails on its own PR rather than on the next release branch. It is a separate step so it doesn't conflict with #954, which rewrites the existing helm-docs step. - Published charts still list the exact values. Before `helm package`, the release and insiders publish workflows run the new `scripts/ci/render-published-readmes.sh`. It deletes the release-value `@default` annotations and reruns `./scripts/helm-docs.sh`, so each published README lists the same digests and registry as the `values.yaml` packaged with it. Nothing is committed. Because the docs are built from the commit being published, the README no longer depends on a later docs commit landing first, which was the cause of the wrong v8.0.0 README. The step is `continue-on-error`: if it fails (for example, the helm-docs download from GitHub fails), the chart still publishes with the committed placeholder README. The tradeoff is that READMEs viewed in this repository show placeholders instead of exact digests. The branch's `values.yaml` still has them, and the README in the published chart (`helm show readme`) shows them in full. The migrator's `helm upgrade` examples keep the `<chart-version>` placeholder in the published chart too. ## How this was tested - **Simulated release.** CI can't exercise a release, so I reproduced one on a scratch worktree. A temporary test in the sourcegraph repo ran the release worker's own `values.yaml` rewrite (`dev/images.UpdateHelmManifestContent`) over the four charts the release touches. It first rewrote them to the internal registry with new tags and digests (the internal phase), then back to the public registry (the promote phase). I bumped `Chart.yaml` `version`/`appVersion` with the same regex as `UpdateChartVersions`, and ran `./scripts/helm-docs.sh` after each phase. | | README lines changed after internal phase | after promote phase | |---|---|---| | `main` | 68 | 60 | | this branch | 0 | 0 | `values.yaml` and `Chart.yaml` changed as expected in both runs. Zero drift on this branch also shows that the release rewrite leaves the `# @default --` comments intact. - **Annotation coverage.** After regenerating, no chart README contains a 64-character `@sha256:` digest or `index.docker.io/sourcegraph`. A second `./scripts/helm-docs.sh` run leaves the tree clean. - **New CI guard.** I ran the new step's command against both trees. It exits 0 on this branch. On `main` it exits 1 and lists the offending lines, for example `sourcegraph.image.repository` in `charts/sourcegraph-executor/dind/README.md`. - **Publish rendering, end to end.** In a scratch worktree I rebuilt a promote branch the way a release would: internal phase, `Chart.yaml` bump, promote phase. Buildkite's helm-docs check stays clean on that tree. I then ran the publish job's steps with Helm v3.4.0, the version the workflows pin: the new script, then both `helm package` loops. I extracted each `.tgz` and compared its README with its packaged `values.yaml`. The expected values were taken from `values.yaml`, not from helm-docs output. | Scenario | Digests in packaged `values.yaml` | Missing from packaged README | Placeholders left | Registry row | |---|---|---|---|---| | Release publish (4 charts) | 24 / 1 / 1 / 1 | 0 | 0 | `"index.docker.io/sourcegraph"` | | Insiders publish (4 charts) | 24 / 1 / 1 / 1 | 0 | 0 | `"index.docker.io/sourcegraph"` | | Render step forced to fail | 24 / 1 / 1 / 1 | all | all | placeholder; packaging still succeeds | For every chart, in all three scenarios, `helm template` of the packaged chart matches the unmodified chart, after normalizing the migrator Job's random name suffix. The script only touches comments and READMEs. The `sourcegraph` chart has 25 annotated images but 24 distinct digests, because `codeIntelDB` and `pgsql` share one. `shellcheck` is clean, and `actionlint` reports the same three pre-existing `actions/checkout@v3` findings as on `main`. - **CI.** On the first commit, [Buildkite build 3049](https://buildkite.com/sourcegraph/deploy-sourcegraph-helm/builds/3049) passed, including ":book: Verify helm-docs is up-to-date" and the new ":book: Verify READMEs do not render release-managed values" step. "Lint and Test charts" also passed. The changes to `values.yaml` are comments only, so rendered manifests are unchanged. The publish workflows only run on pushes to `main`, `release/**` and `promote/release-**`, so this PR's CI can't exercise them. The end-to-end run above stands in for that. The first real check of the full flow will be the next release once both PRs are deployed. Merge this, and backport it to every `X.Y.x` branch that still gets patch releases (at least `8.0.x` and `8.1.x`), before the release worker change deploys. Patch releases branch from `X.Y.x`, and a branch without these annotations would fail the helm-docs check once the worker stops regenerating READMEs. Part of [PLAT-876](https://linear.app/sourcegraph/issue/PLAT-876/find-a-permanent-fix-for-helm-docs-release-failures). Co-authored-by: Warren Gifford <warrenbruceg@gmail.com>
This branch has not been deployed
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
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.
Changes
Makes
scripts/helm-docs.shPOSIX portable, so it can be run the same, by:The helm-docs pinned version and cache location are not changed in this PR
Adds a
--checkarg for CI / agents&& [[ -z $(git status -s) ]]check inside the script, simplifying the "Verify helm-docs is up-to-date" Buildkite step andAGENTS.mdgit: the script checksumscharts/**/README.mdbefore and after helm-docs runs (find,cksum,grep -xF, all POSIX and busybox applets). It compares against the tree before the run, not the index, so a developer who has regenerated but not yet committed a README gets a pass. In CI the tree is clean before the run, so the result is the same as the oldgit statuscheckPrints the rewritten README paths on stdout
--check) prints the repository-relative path of each README helm-docs rewrote, one per line, on stdout. helm-docs itself logs to stderr, so stdout carried nothing beforecharts/**/README.md. This script is now the only implementation of both "which helm-docs version" and "which READMEs changed"--checkadds a hint on stderr and exits 1 when the list is non-emptyTest plan
dash, macOS/bin/sh(bash 3.2),zsh, and busybox v1.36.1sh(Linux x86_64,PATHlimited to busybox applets plus curl): each downloads once on a cold cache, runs from cache when warm, and works when invoked assh helm-docs.shfrom insidescripts/../scripts/helm-docs.shon this branch regenerates all four chart READMEs andgit statusstays clean.helm-docs.*directories left in$TMPDIRafter a cold run.--checkwith dash, macOS/bin/sh, and busybox v1.36.1shwith nogitonPATH: exit 0 on a clean tree; exit 1 namingcharts/sourcegraph/README.mdafter adding a key tocharts/sourcegraph/values.yaml; exit 0 with only an unrelated dirtyvalues.yaml; identical results when run from/tmpand from insidescripts/.--versionstill passes through to helm-docs./bin/sh): clean tree prints nothing on stdout; one stale chart prints onlycharts/sourcegraph/README.mdon stdout with exit 0; two stale charts under--checkprint both paths on stdout, the hint on stderr, exit 1.runHelmDocsScriptagainst agit archiveof this branch with one junkedvalues.yaml: returns exactlycharts/sourcegraph-executor/dind/README.md, the helm-docs log lines are in stderr, and a second run returns an empty list.shellcheck -s sh scripts/helm-docs.shandcheckbashisms scripts/helm-docs.share clean.