Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
60 changes: 60 additions & 0 deletions .github/workflows/dev-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand Down
27 changes: 26 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
56 changes: 56 additions & 0 deletions scripts/check_versions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading