CI: fold PG12+ update-to-current check into the test job, shrink extension-update-test to PG10-only by jnasbyupgrade · Pull Request #78 · Postgres-Extensions/cat_tools

jnasbyupgrade · 2026-08-03T22:35:53Z

extension-update-test's PG12+ leg (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 the test job'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-test itself now only covers PG10's legacy pre-0.2.2 install/update-script checks, its one remaining reason to exist.

Depends on #77

This branch is stacked on #77 (comment-only reformat of the "Test strategy" summary), merged first so this PR's own diff stays small and doesn't fight with that rewrite. Until #77 merges, this PR's diff includes both commits; once #77 merges into master, this branch can be rebased and the diff will shrink to just the functional commit. Please merge #77 first.

What changed

test job: added a step running bin/test_existing update-scenario cat_tools_update 0.2.2 right after make verify-results.
extension-update-test: dropped its PG12+ matrix leg entirely. It now runs on a single fixed major (PG10, via needs.changes.outputs.legacy_pg — single source of truth, not a hardcoded '10') with no strategy: matrix: at all, and the now-dead if: matrix.pg != '10' / == '10' guards are gone.
changes job: removed the update_pg output/derivation — its only consumer is gone.
Only the minimal comment updates needed to describe this diff: the test and extension-update-test entries in the "Test strategy" summary (CI: reformat Test strategy comment, document pg-tle-test/pg-tle-upgrade-test #77's reformatted version), and the cross-reference in pg-tle-test's own comment that pointed at extension-update-test for the update path it no longer covers.
No coverage lost: the PG12+ check still runs on the exact same 7 majors it always did (moved, not removed); the PG10 legacy checks are byte-for-byte unchanged; every other job is untouched.

How it was verified

Ran make check-relkind-source && make verify-results && bin/test_existing update-scenario cat_tools_update 0.2.2 in one shell/cluster session (mirroring the new CI step exactly) against a scratch cluster before folding it in. Confirmed:

no database-name collision (pg_regress's own throwaway db is named independently from cat_tools_update)
the dependency-guard proof fires (once right after CREATE EXTENSION, once again after the full suite run)
the structural-diff check reports the updated database structurally identical to a fresh install
the full suite passes — exit 0 end to end

Background

This (plus #77) is a clean rebuild, on current master, of the schema-independent CI-fold value that used to be PR #75 — split at the maintainer's request into a docs-only PR (#77) and this functional-only one, since mixing them muddied review. #75 itself is now superseded/closed; see its closing comment. That work in turn traces back to PR #54 (closed without merging: its actual subject, a TEST_SCHEMA test-harness dimension, turned out to be a non-starter since cat_tools' control file pins relocatable = false / schema = 'cat_tools').

Test plan

Verified locally against a scratch cluster (see above)
make lint clean
CI green

…le-upgrade-test The "Test strategy" summary listed job names in a manually space-padded "name -- description" column, which read 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. Also documented pg-tle-test and pg-tle-upgrade-test, which this summary never mentioned even though both jobs already exist (added in PR Postgres-Extensions#47) -- and trimmed wordiness in the `test` job's own step comment. Comment-only: no job, matrix, or CI-behavior change.

…k 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. Only the minimal comment updates needed to describe this diff: the `test` and `extension-update-test` entries in the Test strategy summary (added previously in Postgres-Extensions#77, which this is based on), and the cross-references in `pg-tle-test`'s own comment that pointed at extension-update-test for the update path it no longer covers. 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.

coderabbitai · 2026-08-03T22:36:00Z

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c66649ef-dd53-4821-8ed5-7f5300a66cd9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-08-03T22:40:35Z

Claude finished @jnasbyupgrade's task in 3m 36s —— View job

Claude finished the review

Todo list

Gather context (read PR diff, CLAUDE.md, full .github/workflows/ci.yml)
Understand the request (code review of the CI workflow restructuring)
Review the diff for correctness (job gating, matrix→scalar conversion, output wiring, coverage parity)
Post review feedback

Review

I traced through the whole workflow (not just the diff) to check the restructuring is coherent. The core mechanics check out:

changes job: update_pg output/derivation is fully removed, and I confirmed no remaining reference to it anywhere else in the file (grep -n update_pg finds nothing).
test job: the new bin/test_existing update-scenario cat_tools_update 0.2.2 step runs after make verify-results in the same run: block. GitHub's default bash shell runs with -e, so if verify-results fails, the update-scenario command never executes — the per-major gating that mattered from the old design is preserved even though the step moved.
make verify-results depends on $(TEST_DEPS), which includes install (pgxntool/base.mk:374), so cat_tools is already installed by the time bin/test_existing update-scenario runs — no missing make install step was needed in test.
extension-update-test: strategy: matrix is gone, matrix.pg no longer appears anywhere in that job (confirmed via grep), and the now-dead if: matrix.pg != '10' / == '10' guards were correctly dropped along with the steps that only applied to the other leg.
The job's name: field uses ${{ needs.changes.outputs.legacy_pg }} directly (not env.LEGACY_PG) — correct, since the needs context (unlike job-level env) is valid in jobs.<id>.name.
Coverage math: old extension-update-test ran 8 legs (supported_pg 12–18 + legacy_pg 10). New split is 7 legs' worth of update-to-current checks folded into test's existing 7-major matrix, plus 1 fixed PG10 leg in extension-update-test — same 8 total, nothing dropped.
One subtle, intentional-looking behavior change worth being aware of (not a bug): previously, extension-update-test's PG12+ legs were gated on the entire test job succeeding (all matrix legs green) before any update check ran. Now that the update check is a step inside test's own per-major leg, a failure on PG18's fresh-install no longer blocks the update check on PG14 from running. That's a reasonable side effect of the fold, and consistent with how test's matrix already behaves per-major elsewhere in this workflow, but flagging it since it's a real (if minor) semantic shift from the old gating.

One nit: the "Derive the supported-PostgreSQL-major lists" step's comment block (.github/workflows/ci.yml:194-198, untouched by this diff) still says "the fresh-install test matrix, the extension-update-test matrix and the stepwise climb" — but this PR removes extension-update-test's strategy: matrix: entirely, so calling it "the extension-update-test matrix" is now stale. Minor, since the underlying point (single source of truth for PG versions) still holds, but worth a follow-up tweak since this PR is the one that made it stale.

Everything else — the changes/test/extension-update-test/all-checks-passed wiring, the reformatted "Test strategy" comment block, and the pg-tle-test cross-reference update — is internally consistent and matches the PR description. No functional issues found.

Each entry's job name sat alone on its own line, unindented -- reads like a Makefile target declaration rather than a reference to a CI job name, and this repo's own Makefile has real hyphenated targets (test-long, test-update, ...) a reader could easily conflate these with. Only one of these names (test) even has a corresponding make target; formatting the rest identically was actively misleading. Reworded each into an ordinary prose sentence ("The `test` job runs...") so the name reads as a backticked reference inside a sentence, not a declaration.

…scenario # Conflicts: # .github/workflows/ci.yml

github-actions · 2026-08-03T22:54:10Z

Claude finished @jnasbyupgrade's task in 2m 48s —— View job

Tasks

Gather context (PR diff, CLAUDE.md, full .github/workflows/ci.yml)
Review the workflow restructuring for correctness (job gating, matrix→scalar conversion, output wiring, coverage parity)
Post review feedback

Review

I re-verified the full workflow against the PR description (not just the diff). The mechanics are sound:

changes job: update_pg output is fully gone and no reference to it remains anywhere in the file (checked via grep for update_pg/matrix.pg).
extension-update-test: strategy: matrix is removed, matrix.pg doesn't appear anywhere in that job, and it correctly uses needs.changes.outputs.legacy_pg (single source of truth) instead of a hardcoded '10', including in the job-level name: — valid, since needs context is available at that scope.
test job: the new bin/test_existing update-scenario cat_tools_update 0.2.2 step runs in the same run: block right after make verify-results. Default bash -e means a verify-results failure stops the script before the update-scenario line runs, so per-major gating is preserved even though the check moved into the matrix leg. Confirmed bin/test_existing has an update-scenario subcommand (bin/test_existing:251,405), and make verify-results depends on $(TEST_DEPS) (includes install), so cat_tools is already installed before this step runs — no missing setup.
Coverage math checks out: old extension-update-test ran 8 legs (7 supported majors + PG10 legacy); new split is the same 7 update-to-current checks folded into test's matrix + 1 fixed PG10 leg — nothing dropped.
No merge-conflict markers or YAML structural issues from the docs-test-strategy-format merge commit.

Still-open nit from the earlier pass on this PR: the "Derive the supported-PostgreSQL-major lists" step's comment (.github/workflows/ci.yml:196) still reads "...the fresh-install test matrix, the extension-update-test matrix and the stepwise climb..." — but this PR removes extension-update-test's strategy: matrix: entirely, so "the extension-update-test matrix" is stale phrasing now. Same finding as before; doesn't look like it's been addressed yet. Fix this →

One additional observation (not a bug, worth being aware of): folding the update-to-current check into test's per-major step means a fresh-install failure and an update-path failure on the same PG major now surface under a single check name (e.g. "🐘 PostgreSQL 14"), where they previously had distinct job/check names. This is a reasonable tradeoff for avoiding a duplicate container/checkout, but it does mean the Checks tab alone can no longer distinguish "fresh install broke" from "update path broke" for a given major — you'd need to open the step logs. Likely acceptable given the PR's stated goal, just flagging the tradeoff explicitly.

Everything else — the changes/test/extension-update-test/all-checks-passed wiring, the reformatted "Test strategy" comment block, and the pg-tle-test cross-reference cleanup — is internally consistent and matches the PR description. No functional issues found.

…e-PG-lists comment extension-update-test no longer has a strategy: matrix (it's a single fixed PG10 leg now), so the "changes" job's own comment calling it a "matrix" alongside `test`/pg-upgrade-stepwise was stale. Also folded in the `pg-tle-test` mention this comment never had, matching the same job list `test`'s own consumers now cover.

github-actions · 2026-08-03T23:03:27Z

Claude finished @jnasbyupgrade's task in 2m 21s —— View job

Tasks

Gather context (PR diff, CLAUDE.md, full .github/workflows/ci.yml)
Review the workflow restructuring for correctness
Post review feedback

Review

This is the third pass on this PR (two earlier automated reviews already ran on prior pushes). The one nit those raised — stale "extension-update-test matrix" wording in the changes job's "Derive the supported-PostgreSQL-major lists" comment — has since been fixed in 720b1f2; I confirmed the comment now reads `test`/`pg-tle-test` matrix, `extension-update-test`'s single legacy major, and the stepwise climb (ci.yml:196-197).

Re-verified the mechanics from scratch against the current file, not just the diff:

update_pg output/derivation is fully gone from the changes job, and grepping the whole file for update_pg/matrix.pg turns up nothing left in extension-update-test — no dangling references.
test job (ci.yml:340-384): the new bin/test_existing update-scenario cat_tools_update 0.2.2 step runs in the same run: block right after make verify-results. Default bash -e means a verify-results failure stops the script before reaching the update-scenario line, preserving the old per-major gating even though the check moved into the matrix leg.
Confirmed make verify-results genuinely depends on $(TEST_DEPS) (pgxntool/base.mk:417, verify-results: $(TEST_DEPS), distinct from the recipe-only rule at line 385) — TEST_DEPS includes install/installcheck (base.mk:374), so cat_tools is already installed on disk before the new step runs. No missing setup.
Confirmed bin/test_existing has an update-scenario subcommand (bin/test_existing:251,405) matching the new step's invocation.
extension-update-test (ci.yml:711-772): strategy: matrix is gone entirely, it correctly derives its single major from needs.changes.outputs.legacy_pg (including in the job-level name:, where needs context — unlike job-level env — is valid), and the dead if: matrix.pg != '10' / == '10' guards are removed along with the steps that only applied to the other leg.
all-checks-passed's needs: list (ci.yml:1028) still matches the full set of defined jobs exactly (verified by extracting both lists) — nothing was orphaned by the restructuring.
Coverage math holds: old extension-update-test ran 8 legs (7 supported majors + PG10 legacy); new split is the same 7 update-to-current checks folded into test's existing matrix + 1 fixed PG10 leg — nothing dropped, nothing duplicated.

Everything is internally consistent and matches the PR description. No functional issues found — this looks ready to merge (pending #77 landing first, per the PR description, so the diff shrinks to just the functional commit).

jnasbyupgrade added 2 commits August 3, 2026 17:28

jnasbyupgrade added 2 commits August 3, 2026 17:44

Merge branch 'docs-test-strategy-format' into functional-fold-update-…

c96c18c

…scenario # Conflicts: # .github/workflows/ci.yml

jnasbyupgrade mentioned this pull request Aug 3, 2026

CI: fold PG12+ guard-proved update-to-current check into the test job, shrink extension-update-test to PG10-only #75

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

CI: fold PG12+ update-to-current check into the test job, shrink extension-update-test to PG10-only - #78

CI: fold PG12+ update-to-current check into the test job, shrink extension-update-test to PG10-only#78
jnasbyupgrade wants to merge 5 commits into
Postgres-Extensions:masterfrom
jnasbyupgrade:functional-fold-update-scenario

jnasbyupgrade commented Aug 3, 2026

Uh oh!

coderabbitai Bot commented Aug 3, 2026 •

edited

Loading

Review skipped

Uh oh!

github-actions Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 3, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

jnasbyupgrade commented Aug 3, 2026

Depends on #77

What changed

How it was verified

Background

Test plan

Uh oh!

coderabbitai Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude finished the review

Review

Uh oh!

github-actions Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tasks

Review

Uh oh!

github-actions Bot commented Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tasks

Review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Aug 3, 2026 •

edited

Loading

github-actions Bot commented Aug 3, 2026 •

edited

Loading

github-actions Bot commented Aug 3, 2026 •

edited

Loading

github-actions Bot commented Aug 3, 2026 •

edited

Loading