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
changed the title
marcleblanc2
changed the title
marcleblanc2
changed the title
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
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
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.
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.