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/.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 ebef6ed9..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 @@ -96,7 +110,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 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