Promote dev → main: the fast lane's Postgres client, and a test that keeps both lanes honest - #1256
Merged
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018eFXcF6BM7bxFVEQH67iuk
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018eFXcF6BM7bxFVEQH67iuk
…both lanes keep it (#1255) Closes the two loose ends from promotion #1253. ci.yml's pg_dump PATH fix is cherry-picked onto dev, so dev's ci.yml is once again byte-identical to main's. dev-ci.yml gains the same Postgres-client lane. Its Go job runs the same `go test ./...` against the same server-18 service as the full gate and had no matching client, so TestBackupRestoreRoundTrip hit its major-mismatch 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 promotion PR. TestGoSuiteLanesInstallMatchingPgClient asserts both halves — the install and the $GITHUB_PATH append — for both lanes. Each half has failed once in production, one per lane, neither visibly in its own lane. Verified by mutation (both failure modes produce their intended error) and by running TestBackupRestoreRoundTrip for real against a local PostgreSQL server with matching majors: PASS, not SKIP.
…leet-promote-pw9idd
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
2 participants
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.
What changed, and why
Promotion of
dev→maincarrying #1255: three files, +130 lines, no production code.dev-ci.ymlgets the Postgres-client lane the full gate already had. Its Go job runs the samego test ./...against the samepostgres:18service asci.yml's and installed no matching client. The runner ships client 16, andTestBackupRestoreRoundTripself-skips on a client/server major mismatch, so the only coverage offleet backup/fleet restoreran as a SKIP on every dev push. The second cost is the one that prompted this:ci.ymlbeing the sole home of that step is exactly why a broken version of it could not surface until promotion Promote dev → main: Kubernetes as a first-class deployment, the CI-and-scanner overhaul, the enterprise security audit #1253. A step that lives in one lane is a step the other lane cannot catch.TestGoSuiteLanesInstallMatchingPgClientasserts, for both lanes, that each installs apostgresql-client-Nand appends/usr/lib/postgresql/N/binto$GITHUB_PATH. Both halves have failed in production, one per lane —dev-ci.ymlwith no install at all,ci.ymlwith an install whose assertion could not pass — and neither was visible in its own lane, so a "keep these in sync" comment is not enough. The service major is already covered byTestPostgresMajorAgreesAcrossCI, so this asserts only the two things that broke.ci.ymldoes not appear in this diff, and that is the intended outcome: #1255 cherry-picked its pg_dump fix ontodev, producing content identical to whatmainalready had, so the two branches no longer diverge there.How you verified it
Dev gatewas green on #1255 before it merged — 15/15, including the Go job at 6.3 min. That job is the direct evidence for the new steps: it carriesAssert pg_dump on PATH is the server major, so it could not have gone green with PATH resolution still broken on the runner.The merge was clean — no conflicts. Worth noting explicitly, because promotion #1253 hit twenty: #1253 was merged as a merge commit rather than squashed, so the ancestry link survived and this promotion cost nothing to assemble. The merged tree is byte-identical to
origin/dev.From #1255, carried forward:
$GITHUB_PATHappend and dropping the client install each produce their intended failure, and restoring givesok. A test that passes unconditionally is the exact failure mode it exists to prevent.TestBackupRestoreRoundTrippasses rather than skips once the majors agree — run for real against a local PostgreSQL 16 server with the matching client first on PATH (--- PASS (0.31s)). Before theci.ymlfix,pg_dumpresolved to 16 on both lanes, so that test had not actually executed in CI on either. This un-skips into working coverage, not into a new red.On this merged tree:
actionlintclean over every workflow, and the full./scripts/suite green — including the new test and the two neighbours it could have disturbed.Scope and deviations
A promotion of one PR. No production code, no invariant moves, no new ADR needed.
One deliberate non-change: the new test names
ci.ymlanddev-ci.ymlrather than deriving the lane set.benchmark.yml,e2e-canary.ymlandci.yml's owne2e-livejob also declare a Postgres service but never run the Go suite, so demanding a pg_dump client of them would be noise — and pretending to infer "the lanes that rungo test ./...against a service" out of YAML would be a worse lie than naming two files.Merge this as a merge commit, not a squash. #1248 was squashed and #1253 paid for it with twenty add/add conflicts; #1253 was merged properly and this PR assembled without a single one.
CHANGELOG.mdupdated — entry landed with Give the fast lane the Postgres client the full gate had, and assert both lanes keep it #1255 ondevdocs/<FEATURE>.md) added, if this ships a feature — n/a, ships no featureGenerated by Claude Code