From 055df2c8d2a8b6ed1ea1c0a3e3e4b31993a05e53 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 01:31:35 +0000 Subject: [PATCH 1/2] Make ci.yml's pg_dump major assertion able to pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The assertion added in this range failed on its first real outing — got=16 against server 18 — over an install that had plainly succeeded (`Setting up postgresql-client-18 (18.6-1.pgdg24.04+2)`). Installing the package is not what decides the answer: /usr/bin/pg_dump is a symlink to postgresql-common's pg_wrapper, and the wrapper's own header states the rule — it calls the client "with the version, cluster and default database specified in ~/.postgresqlrc or /etc/postgresql-common/user_clusters". That is cluster configuration, not "the newest client installed", so on a runner image carrying a PostgreSQL 16 cluster, adding a client package alongside it changes nothing about the dispatch. Verified rather than reasoned about: this container has the same shape as the runner (/usr/bin/pg_dump -> ../share/postgresql-common/pg_wrapper, the versioned tree at /usr/lib/postgresql/16/bin), and there the wrapper likewise reports 16 while prepending the versioned bin dir resolves pg_dump to it directly. So the versioned bin dir goes first on $GITHUB_PATH. That is also what the test needs: internal/admincli/backup.go execs `pg_dump` off PATH, with no versioned path of its own, so PATH resolution — not package presence — is the thing that decides whether backup_test.go's major-mismatch t.Skipf turns the only coverage of `fleet backup`/`fleet restore` off. Asserted in two places, because $GITHUB_PATH takes effect only from the next step: the install by absolute path in the step that performs it, and PATH resolution in a step of its own. The second one is the assertion that mirrors what the test actually invokes, so the two cannot drift apart again without going red. The CHANGELOG entry claiming the step "now asserts the major" is corrected in the same commit — the assertion was there and could not pass, which is the kind of half-true the honesty-in-docs invariant is about. Verified: actionlint clean over every workflow; go test ./scripts/ passes, including TestPostgresMajorAgreesAcrossCI, which requires every postgres major named anywhere in .github/workflows to agree. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_018eFXcF6BM7bxFVEQH67iuk --- .github/workflows/ci.yml | 35 +++++++++++++++++++++++++++++++++-- CHANGELOG.md | 13 ++++++++++++- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 97705e89..b7a958e6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -384,12 +384,43 @@ jobs: | sudo tee /etc/apt/sources.list.d/pgdg.list >/dev/null sudo apt-get update -qq sudo apt-get install -y -qq postgresql-client-18 + # Installing the package does NOT change what `pg_dump` resolves to, + # and that is what made this step's first outing fail with `got=16` + # over an install that had plainly succeeded. /usr/bin/pg_dump is a + # symlink to postgresql-common's pg_wrapper, and the wrapper's own + # header states the rule: it calls the client "with the version, + # cluster and default database specified in ~/.postgresqlrc or + # /etc/postgresql-common/user_clusters". That is cluster + # configuration, NOT "the newest client installed" — so on a runner + # image carrying a PostgreSQL 16 cluster, adding a 16-agnostic client + # package changes nothing about the dispatch. Put the versioned bin + # dir first on PATH instead, which is what the Go test needs anyway: + # it execs `pg_dump` off PATH (internal/admincli/backup.go), with no + # versioned path of its own. + echo "/usr/lib/postgresql/18/bin" >> "$GITHUB_PATH" # Assert the major, don't just print it: the test self-skips on a # mismatch, so an install that "succeeded" with the wrong major is - # exactly as invisible as one that failed. + # exactly as invisible as one that failed. Absolute path here because + # $GITHUB_PATH only takes effect in LATER steps — PATH resolution is + # asserted in the next step, where it has. + got="$(/usr/lib/postgresql/18/bin/pg_dump --version | grep -oE '[0-9]+' | head -1)" + [ "$got" = "18" ] || { + echo "::error::installed pg_dump major ${got} != server major 18 — the backup/restore round-trip test would self-skip." + exit 1 + } + + - name: Assert pg_dump on PATH is the server major + # A separate step because $GITHUB_PATH applies from the next step on, + # and PATH resolution is the half that actually broke: the package + # installed correctly and `pg_dump` still meant 16. This asserts the + # exact lookup the round-trip test performs, so the two cannot drift + # again without going red. + run: | + set -euo pipefail + command -v pg_dump got="$(pg_dump --version | grep -oE '[0-9]+' | head -1)" [ "$got" = "18" ] || { - echo "::error::pg_dump major ${got} != server major 18 — the backup/restore round-trip test would self-skip." + echo "::error::pg_dump on PATH is major ${got}, not server major 18 — the backup/restore round-trip test would self-skip." exit 1 } diff --git a/CHANGELOG.md b/CHANGELOG.md index ebef6ed9..4bd3167d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,7 +96,18 @@ prior versions are listed because none have shipped. was best-effort (`|| echo`), so an unreachable PGDG left client 16 in place and `backup_test.go`'s major-mismatch `t.Skipf` turned the *only* coverage of `fleet backup` / `fleet restore` off behind the single required check on - `main`; it now asserts the major. And the docs-only classifier initialised + `main`; it now asserts the major — and asserts it twice, because the first + version of that assertion could not pass: installing `postgresql-client-18` + does not change what `pg_dump` resolves to. `/usr/bin/pg_dump` is + postgresql-common's `pg_wrapper`, which dispatches on the version/cluster in + `~/.postgresqlrc` or `/etc/postgresql-common/user_clusters` rather than on the + newest client present, so on a runner carrying a PostgreSQL 16 cluster the + wrapper kept selecting 16 and the step failed with `got=16` over a successful + install. The versioned bin dir now goes first on `$GITHUB_PATH` — which is + what the round-trip test needs anyway, since it execs `pg_dump` off PATH — and + the install is asserted by absolute path in that step, PATH resolution in the + next one (`$GITHUB_PATH` only applies from the following step on). And the + docs-only classifier initialised `docs_only=true` and only ever cleared it inside its loop, so an **empty** diff classified as docs-only and skipped the suite — which `ci-gate` then waved through, because an empty diff is the absence of evidence, not evidence that From 7f507d34d703d08c5309d23733582efdeafc0030 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 02:01:48 +0000 Subject: [PATCH 2/2] Give the fast lane the Postgres client, and assert both lanes have it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit dev-ci.yml's Go job runs the same `go test ./...` against the same server-18 service as ci.yml's, and installed no matching client. The runner ships client 16, and TestBackupRestoreRoundTrip self-skips on a client/server major mismatch, so the ONLY coverage of `fleet backup` / `fleet restore` ran as a SKIP on every dev push — the green-but-vacuous shape this repo keeps writing post-mortems about. It had a second cost, which is why this is worth more than the coverage: ci.yml being the sole home of that step is exactly why a broken version of it could not surface until a dev→main promotion PR. A step that exists in one lane is a step the other lane cannot catch. So the install is now in both lanes, byte-identical, including the $GITHUB_PATH append the wrapper makes necessary and the two assertions (install by absolute path, PATH resolution in the following step, since $GITHUB_PATH only applies from the next step on). TestGoSuiteLanesInstallMatchingPgClient asserts both halves for both lanes. Both halves have failed in production, one per lane — dev-ci.yml with no install at all, ci.yml with an install whose assertion could not pass — and neither failure was visible in its own lane, so a comment saying "keep these in sync" is not enough. The test names the two files rather than inferring them: benchmark.yml, e2e-canary.yml and ci.yml's e2e-live job also declare a Postgres service but never run the Go suite, and pretending to derive "the lanes that run go test ./... with a service" from YAML would be a worse lie than naming them. Verified: - The new test fails on each failure mode and passes when restored — checked by mutation, not by reading it: dropping the $GITHUB_PATH line and dropping the client install each produce the intended error. - TestBackupRestoreRoundTrip PASSES (not skips) once the majors agree. Ran it for real against a local PostgreSQL 16 server with the matching client-16 on PATH: `--- PASS: TestBackupRestoreRoundTrip (0.31s)`. Before the ci.yml fix that preceded this, pg_dump resolved to 16 on BOTH lanes, so this test had not actually executed in CI on either. - actionlint clean over every workflow; gofmt, go vet and the full ./scripts/ suite clean. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_018eFXcF6BM7bxFVEQH67iuk --- .github/workflows/dev-ci.yml | 60 ++++++++++++++++++++++++++++++++++ CHANGELOG.md | 14 ++++++++ scripts/check_versions_test.go | 56 +++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+) diff --git a/.github/workflows/dev-ci.yml b/.github/workflows/dev-ci.yml index 34a623ac..fa62c564 100644 --- a/.github/workflows/dev-ci.yml +++ b/.github/workflows/dev-ci.yml @@ -93,6 +93,66 @@ jobs: go-version-file: go.mod cache: true + - name: Install postgresql-client-18 (so pg_dump matches the server major) + # This lane runs the same `go test ./...` as the full gate against the + # same server-18 service, so it needs the same client — and it did not + # have it. The runner ships client 16, and TestBackupRestoreRoundTrip + # (internal/admincli/backup_test.go) self-skips on a client/server major + # mismatch, so dev ran the ONLY coverage of `fleet backup` / + # `fleet restore` as a SKIP. Worse than the missing coverage: this step + # existing only in ci.yml is why a broken version of it (see below) + # could not be caught until a dev→main promotion PR, which is the one + # place this repo least wants to be debugging CI. + # + # Kept byte-identical to ci.yml's copy on purpose, and asserted as such + # by TestGoSuiteLanesInstallMatchingPgClient in scripts/. + run: | + set -euxo pipefail + sudo install -d /usr/share/postgresql-common/pgdg + sudo curl -fsSL https://www.postgresql.org/media/keys/ACCC4CF8.asc \ + -o /usr/share/postgresql-common/pgdg/apt.postgresql.org.asc + echo "deb [signed-by=/usr/share/postgresql-common/pgdg/apt.postgresql.org.asc] https://apt.postgresql.org/pub/repos/apt $(lsb_release -cs)-pgdg main" \ + | sudo tee /etc/apt/sources.list.d/pgdg.list >/dev/null + sudo apt-get update -qq + sudo apt-get install -y -qq postgresql-client-18 + # Installing the package does NOT change what `pg_dump` resolves to. + # /usr/bin/pg_dump is a symlink to postgresql-common's pg_wrapper, and + # the wrapper's own header states the rule: it calls the client "with + # the version, cluster and default database specified in + # ~/.postgresqlrc or /etc/postgresql-common/user_clusters". That is + # cluster configuration, NOT "the newest client installed" — so on a + # runner image carrying a PostgreSQL 16 cluster, adding a client + # package alongside it changes nothing about the dispatch. Put the + # versioned bin dir first on PATH instead, which is what the test + # needs anyway: it execs `pg_dump` off PATH + # (internal/admincli/backup.go), with no versioned path of its own. + echo "/usr/lib/postgresql/18/bin" >> "$GITHUB_PATH" + # Assert the major, don't just print it: the test self-skips on a + # mismatch, so an install that "succeeded" with the wrong major is + # exactly as invisible as one that failed. Absolute path here because + # $GITHUB_PATH only takes effect in LATER steps — PATH resolution is + # asserted in the next step, where it has. + got="$(/usr/lib/postgresql/18/bin/pg_dump --version | grep -oE '[0-9]+' | head -1)" + [ "$got" = "18" ] || { + echo "::error::installed pg_dump major ${got} != server major 18 — the backup/restore round-trip test would self-skip." + exit 1 + } + + - name: Assert pg_dump on PATH is the server major + # A separate step because $GITHUB_PATH applies from the next step on, + # and PATH resolution is the half that actually broke in ci.yml: the + # package installed correctly and `pg_dump` still meant 16. This asserts + # the exact lookup the round-trip test performs, so the two cannot drift + # again without going red. + run: | + set -euo pipefail + command -v pg_dump + got="$(pg_dump --version | grep -oE '[0-9]+' | head -1)" + [ "$got" = "18" ] || { + echo "::error::pg_dump on PATH is major ${got}, not server major 18 — the backup/restore round-trip test would self-skip." + exit 1 + } + - name: Create test databases run: | PGPASSWORD=fleet psql -h localhost -U fleet -d fleet -v ON_ERROR_STOP=1 \ diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bd3167d..7a77fb57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,20 @@ prior versions are listed because none have shipped. ### Added +- **The fast lane got the Postgres client the full gate had, and a test that + keeps both lanes honest.** `dev-ci.yml`'s Go job runs the same + `go test ./...` against the same server-18 service as `ci.yml`, but installed + no matching client — so `TestBackupRestoreRoundTrip` hit its + client/server-major `t.Skipf` and the **only** coverage of `fleet backup` / + `fleet restore` ran as a SKIP on every dev push. It also left `ci.yml` as the + sole home of that step, which is why a broken version of it could not surface + until a dev→main promotion PR. Both lanes now install + `postgresql-client-18`, put its versioned bin dir on `$GITHUB_PATH`, and + assert the major twice (install by absolute path, PATH resolution in the next + step). `TestGoSuiteLanesInstallMatchingPgClient` asserts both halves for both + lanes — the install and the `$GITHUB_PATH` append — because each has failed + once already, invisibly, one per lane. Measured, not assumed: the round-trip + test **passes** (not skips) once the majors agree. - **A workflow + shell lint gate (`actionlint`, `shellcheck`):** nothing checked the ~3.1k lines of workflow YAML that decide what every other gate runs, and nothing checked the ~6.2k lines of bash that *are* the deploy path diff --git a/scripts/check_versions_test.go b/scripts/check_versions_test.go index fa560a26..0869272f 100644 --- a/scripts/check_versions_test.go +++ b/scripts/check_versions_test.go @@ -236,6 +236,62 @@ func TestPostgresMajorAgreesAcrossCI(t *testing.T) { } } +// TestGoSuiteLanesInstallMatchingPgClient: the two lanes that run the full +// `go test ./...` suite against a Postgres service — ci.yml's `go` job and +// dev-ci.yml's — must each install a matching postgresql-client AND put that +// client's versioned bin dir on PATH. Both halves have failed in production, +// one per lane, and both failures were invisible: +// +// - dev-ci.yml had no client install at all. The runner ships client 16 +// against a server-18 service, and TestBackupRestoreRoundTrip self-skips on +// a major mismatch, so the ONLY coverage of `fleet backup` / `fleet restore` +// ran as a SKIP on every dev push. It also meant ci.yml's copy of this step +// was the only copy, so a broken version of it could not surface until a +// dev→main promotion PR. +// - ci.yml installed the client and asserted the major, and the assertion +// could not pass: /usr/bin/pg_dump is a symlink to postgresql-common's +// pg_wrapper, which dispatches on the version/cluster in ~/.postgresqlrc or +// /etc/postgresql-common/user_clusters rather than on the newest client +// installed. On a runner carrying a PostgreSQL 16 cluster the wrapper kept +// selecting 16 over a successful client-18 install. +// +// The service major itself is covered by TestPostgresMajorAgreesAcrossCI, which +// requires every postgres major named anywhere in .github/workflows to agree — +// so this test asserts only the two things that broke. +// +// Scoped to these two files by name rather than derived: benchmark.yml, +// e2e-canary.yml and ci.yml's own e2e-live job also declare a Postgres service +// but never run the Go suite, so requiring a pg_dump client of them would be +// noise, and pretending to infer "the lanes that run go test ./... with a +// service" from YAML would be a worse lie than naming them. +func TestGoSuiteLanesInstallMatchingPgClient(t *testing.T) { + root := repoRoot(t) + clientRe := regexp.MustCompile(`postgresql-client-(\d+)`) + + for _, wf := range []string{ + ".github/workflows/ci.yml", + ".github/workflows/dev-ci.yml", + } { + body := readFile(t, root, wf) + + client := clientRe.FindStringSubmatch(body) + if client == nil { + t.Errorf("%s runs `go test ./...` against a Postgres service but installs no postgresql-client-N: TestBackupRestoreRoundTrip self-skips on a client/server major mismatch, so `fleet backup` / `fleet restore` coverage silently drops to zero behind a green check", wf) + continue + } + major := client[1] + + // The package alone does not decide what `pg_dump` resolves to — the + // wrapper does. Only a $GITHUB_PATH append makes the versioned client + // the one every later step, and the Go test's exec.LookPath, resolves. + want := `echo "/usr/lib/postgresql/` + major + `/bin" >> "$GITHUB_PATH"` + if !strings.Contains(body, want) { + t.Errorf("%s installs postgresql-client-%s but never appends /usr/lib/postgresql/%s/bin to $GITHUB_PATH (want the line %s) — without it /usr/bin/pg_dump stays postgresql-common's pg_wrapper, which dispatches on the configured cluster rather than the newest client installed, and `pg_dump` keeps resolving to the runner's own major", + wf, major, major, want) + } + } +} + // goMinor pulls "1.27" out of a go.mod `go` directive or a `golang:1.27` image // tag, discarding any patch component. Comparing at major.minor is deliberate: // web/go.mod says in its own comment that pinning a PATCH there just created a