CI: fold PG12+ guard-proved update-to-current check into the test job, shrink extension-update-test to PG10-only - #75
Conversation
…t` job, shrink extension-update-test to PG10-only extension-update-test's PG12+ leg ran on the exact same PostgreSQL majors as the `test` job (supported_pg, 12-18), but as its own matrix job: its own runner, container boot, checkout, apt-get, and `make install`, paid again per major, for a check that can run as one more step inside a container the `test` job already has running, already checked out, and already has cat_tools installed on disk in (installcheck, a TEST_DEPS prerequisite, already ran as a side effect of that job's own verify-results call). Folded `bin/test_existing update-scenario cat_tools_update 0.2.2` in as an additional call in the `test` job's "Test on PostgreSQL" step instead. Verified before folding it in, not assumed: ran `make check-relkind-source && make verify-results && bin/test_existing update-scenario cat_tools_update 0.2.2` in the same shell/cluster session (mirroring the new CI step exactly) against a scratch cluster. Confirmed no database-name collision (pg_regress's own throwaway db is named independently from `cat_tools_update`), the dependency-guard proof fires (twice -- once right after CREATE EXTENSION, once again after the full suite run), the structural-diff check (bin/structural_diff, from PR Postgres-Extensions#55) fires and reports the updated database structurally identical to a fresh install, and the full suite passes -- exit 0 end to end. extension-update-test now runs PG10 only, with no matrix at all (single source of truth: needs.changes.outputs.legacy_pg, not a hardcoded "10") -- its entire remaining purpose is the pre-0.2.2 legacy-script checks, the only place those scripts still load. Removed the now-dead `if: matrix.pg != '10'` / `if: matrix.pg == '10'` guards throughout that job (nothing left to guard against once there's no other leg) and the "Update 0.2.2 -> current" step (moved above). The `changes` job's `update_pg` output/derivation is removed too -- it had exactly one consumer, and that consumer is gone. Restructured the top-of-file "Test strategy" comment around what actually matters to a reader: what runs on EVERY supported PostgreSQL major (the `test` job, now including the guard-proved update check, and `pg-tle-test` as a second, independently-isolated main-matrix job -- kept separate deliberately, since it specifically proves pg_tle deployment ISOLATION, not filesystem-install coincidence, unlike the fold above) versus SPECIAL CASES that apply to one specific scenario only (extension-update-test's PG10 legacy scripts, pg-upgrade-test's/pg-tle-upgrade-test's specific binary-pg_upgrade jump pairs, and pg-upgrade-stepwise's full climb). The pg-tle-test bullet leads with the goal (proving cat_tools works correctly via pg_tle) rather than isolation -- isolation is the precondition that makes that proof trustworthy, not the point of the job. No coverage lost: the PG12+ update-to-current check still runs on the exact same 7 majors it always did (moved, not removed), the PG10 legacy checks are byte-for-byte unchanged, and every other job is untouched.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude finished @jnasbyupgrade's task in 1m 45s —— View job Review: PR #75
CI on this PR has already completed successfully (run 30854798139). Summary: This is a clean, well-verified CI restructuring. I traced the code paths the description makes claims about, and they all hold up:
One minor, non-blocking observation: the new No coverage gaps, no dead code left behind, no correctness issues found. Nice work verifying this against a real cluster before folding it in rather than assuming it would work. |
Prompted directly by PR Postgres-Extensions#54/Postgres-Extensions#75 turning into hard-to-follow walls of text (a real contributor to a costly mid-PR mix-up), versus Postgres-Extensions#74's own description staying easy to follow despite covering real backstory. Length past the opening isn't the problem -- an unstructured mass of prose is.
…rategy comment The "Test strategy" summary listed job names in a manually space-padded "name -- description" column, which reads like a Makefile target list or a formatted spec table rather than a plain comment. Restructured each entry as a small heading (the job name alone, minimally indented) with its description as ordinary wrapped prose underneath -- same content, no alignment bookkeeping.
|
Claude finished @jnasbyupgrade's task in 1m 14s —— View job Review: PR #75 (updated after latest push)
Summary: The prior review already covered the substantive CI restructuring ( Checked the diff directly:
No functional risk, no coverage change, nothing to flag. This matches the PR description's own explanation for the commit. |
"GUARD-PROVED" was leading the description, making the verification mechanism (a planted dependency guard) read as the headline instead of what the check actually proves: cat_tools updates cleanly to the current version. Reworded both the Test strategy bullet and the test job's step comment so "update to current" is the label, with the dependency guard described as one supporting piece of how that's proven, not the point itself.
|
Claude finished @jnasbyupgrade's task in 1m 4s —— View job Review: PR #75 (updated after latest push)
Summary: Like the previous synchronize, this one is a pure comment reword — The change does exactly what the commit message says: it demotes "GUARD-PROVED" from the headline to a supporting detail, and leads with "UPDATE TO CURRENT" as the actual thing being proven. Checked:
CI on this commit passed (run 30858160392). No functional risk, no coverage change, nothing to flag. |
|
Closing without merging -- superseded by a split into two PRs, per the maintainer's request: this PR mixed the actual functional change (folding Split into:
Both are green and ready for review. Please merge #77 first (per its own description), then #78. |
1 participant
extension-update-test's PG12+ leg (the guard-proved update-to-current check: CREATE EXTENSION at 0.2.2, ALTER EXTENSION UPDATE to current, structural diff against a fresh install, then the full suite) used to run as its own separate matrix job. It's now folded into thetestjob's existing step instead, since that job already has a container/checkout/install running for the same PostgreSQL majors — a separate job was paying for all of that again for no added coverage.extension-update-testitself now only covers PG10's legacy pre-0.2.2 install/update-script checks, its one remaining reason to exist.What changed
testjob: added a step runningbin/test_existing update-scenario cat_tools_update 0.2.2right aftermake verify-results.extension-update-test: dropped its PG12+ matrix leg entirely. It now runs on a single fixed major (PG10, vianeeds.changes.outputs.legacy_pg— single source of truth, not a hardcoded'10') with nostrategy: matrix:at all, and the now-deadif: matrix.pg != '10'/== '10'guards are gone.changesjob: removed theupdate_pgoutput/derivation — its only consumer is gone.pg-tle-test's own blurb, which now leads with the goal — proving cat_tools works via pg_tle — rather than isolation mechanics) to reflect the new job responsibilities.How it was verified
Ran
make check-relkind-source && make verify-results && bin/test_existing update-scenario cat_tools_update 0.2.2in one shell/cluster session (mirroring the new CI step exactly) against a scratch cluster before folding it in. Confirmed:cat_tools_update)Background
This is a clean rebuild, on current
master, of the schema-independent CI-fold value from PR #54 (commits c12d0bd and 91277a5). PR #54 itself is closed without merging: its actual subject, aTEST_SCHEMAtest-harness dimension, turned out to be a non-starter — cat_tools' control file pinsrelocatable = false/schema = 'cat_tools', so the extension can never install into a custom schema, making schema-targeting tests meaningless. See PR #54's closing comment for the full explanation, and #76 for the corrected, narrower follow-up that spawned.Test plan
make lintclean