CI: priority lanes for rebase-cascade pushes, escalate on stack promotion - #50
CI: priority lanes for rebase-cascade pushes, escalate on stack promotion#50jnasbyupgrade wants to merge 16 commits into
Conversation
test/install/load.sql now selects fresh/update/existing via the
count_nulls.test_load_mode GUC (TEST_LOAD_SOURCE make var), matching
pgxntool/README.asc's documented U&U pattern:
- fresh: CREATE EXTENSION count_nulls (unchanged from phase 1/2).
- update: CREATE EXTENSION VERSION '0.9.6', then ALTER EXTENSION UPDATE -
committed, since test/install runs outside any per-test rolled-back
transaction.
- existing: asserts count_nulls is already installed and current,
touches nothing - for a real pg_upgrade run external to this
invocation (a later phase adds the CI job that drives this).
Added the extension-update-test CI job: just `make verify-results
TEST_LOAD_SOURCE=update`, no external script needed - unlike a real
pg_upgrade, an in-place update is pure SQL, so test/install handles the
whole fresh-vs-updated comparison within one pg_regress invocation.
Verified locally against PG17: fresh, update, and existing modes all pass
via make verify-results, including update x TEST_SCHEMA=Quoted crossed
together - zero expected-output changes needed for any combination,
confirming phase 2's schema-invariant design holds across load modes too
(load-bearing for a later phase that crosses U&U with schema in CI).
existing mode verified manually against a real out-of-band CREATE
EXTENSION + --use-existing run.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Each extension-update-test leg duplicated the fresh-install test job's own per-PG-version container/checkout setup just to additionally run the update-mode check - a load mode is just an input the same assertions run against, not a real environment difference (same reasoning TEST_SCHEMA already collapsed out of the CI matrix on). Relocate its two steps into the test job instead of giving them their own dedicated job and matrix.
\gset binds every column of a query's result row at once, so the raw GUC read and the two derived-boolean SELECTs can be one three-column query instead of three separate SELECT/\gset pairs. The DO block's validation still runs immediately after, unchanged in position and logic.
…rce PG list Independent of the U&U testing work itself, but best done now that multiple CI jobs exist and before the next phase adds the most expensive one (a real pg_upgrade job): - `changes` job: computes the actual per-push diff and skips test/ extension-update-test/pg-tle-test entirely on doc-only pushes, always triggering itself (no workflow-level paths-ignore, which would leave all-checks-passed stuck Pending on doc-only pushes in branch protection). - Derives the supported-PostgreSQL-major list from ONE set of constants (NEWEST/FLOOR) in that same job, consumed by both the `test` and `extension-update-test` matrices via fromJSON - they can't silently drift onto different lists, and a new major is a one-line change. - `all-checks-passed`: single stable required-status-check name, with a self-check that its own needs list can't silently omit a newly-added job. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The org-wide Actions runner queue backs up easily; a draft PR being actively iterated on doesn't need the full PG matrix or the heavy pg-tle-test job re-run on every push. Add a newest_pg scalar output (single source alongside supported_pg) and reduce the test job's matrix to just that value on a draft PR, while skipping pg-tle-test (and any later heavy job following the same needs:[changes]/if: docs_only pattern) outright. Non-draft PRs and push events (e.g. post-merge on master) are unaffected.
Adds the pg-upgrade-test CI job: install 0.9.6 on an old PostgreSQL major, plant + prove a dependency guard, binary pg_upgrade to a newer major, ALTER EXTENSION UPDATE the migrated objects, then run the suite against the real upgraded database in existing mode. bin/test_existing is much smaller than the equivalent script would have been pre-test/install: only prepare-old and run-suite are genuinely external-to-pg_regress concerns (a real pg_upgrade binary run isn't something pg_regress can invoke itself), plus a small `update` subcommand for the post-upgrade ALTER EXTENSION UPDATE step. There's no update-scenario subcommand at all - that entire scenario is just `make test-update` now (test/install/load.sql's own 'update' mode, added in phase 3), since an in-place update has no external step to drive. run_suite() gates on plain `make test`, not the old belt-and-suspenders `make test && make verify-results` - pgxntool 2.3.0 (this repo's phase 0) already made `make test` itself exit non-zero on regression failures. Not yet crossed with TEST_SCHEMA - that's the next phase, once both this job and extension-update-test can cross it together. Verified locally against PG17 (prepare-old -> update -> run-suite, without a real pg_upgrade - this container's clusters are persistent shared infra, so the actual binary pg_upgrade leg is left for CI's ephemeral containers, same reasoning as the pg-tle-test work). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… not after Reorders prepare-old -> update -> pg_upgrade -> run-suite (was prepare-old -> pg_upgrade -> update -> run-suite). The old order proved pg_upgrade could migrate 0.9.6's frozen objects, then updated afterward - not actionable, since that version already shipped. This job's whole point is proving pg_upgrade correctly migrates the objects count_nulls' CURRENT code creates, which requires updating BEFORE the binary upgrade runs. make install (into the old cluster) already happens earlier in the job, so the current version's update scripts are on disk in time for the moved step. Updates the job's step names/comments and bin/test_existing's own file-header sequence description to match the new order.
Propagates the draft-PR gating from phase3.5-ci-hygiene to the pg-upgrade-test job introduced by this branch: same needs:[changes]/ if: docs_only pattern as pg-tle-test, so it gets the same && github.event.pull_request.draft != true guard.
…/shell loops, not a matrix Redesign of the original approach (which crossed TEST_SCHEMA into both jobs' CI matrices) per the same reasoning as the `test` job's collapse: a schema name is just an input the same assertions run against, not a real environment difference. - extension-update-test: added `make test-update-schema-all` (Makefile), the same TEST_SCHEMA loop as test-schema-all but with TEST_LOAD_SOURCE=update. Job step calls it instead of crossing schema into the matrix. - pg-upgrade-test: no make-level loop is possible here (bin/test_existing's steps are shell, not `make test`), so instead prepares TWO databases - count_nulls_upgrade_none and count_nulls_upgrade_quoted, one per TEST_SCHEMA value - before the SINGLE pg_upgrade call, which migrates the whole cluster (every database in it) in one pass. This is strictly better than a doubled matrix would have been: it also halves the number of actual pg_upgrade binary invocations (the single most expensive operation in this job), not just container/checkout overhead. Verified locally against PG17: prepare-old -> update -> run-suite passes for both databases in the same cluster/session (no real pg_upgrade run, same reasoning as prior phases - this container's clusters are persistent shared infra); make test-update-schema-all passes both TEST_SCHEMA legs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…_update) Adds the one checklist item genuinely missing everywhere (cat_tools' own PR #46 only did this manually, uncommitted, per the design doc this whole series is based on): a script that installs fresh and 0.9.6-then-updated copies of count_nulls in their own scratch databases and diffs pg_get_functiondef/comments/ACLs for every object the extension owns (discovered live via pg_depend, not a hardcoded object list). Wired into extension-update-test as an automatic step, crossed with TEST_SCHEMA same as the rest of that job. Also scopes out extending pg-tle-test to the update path (documented inline, and filed as Postgres-Extensions/pgxntool#90): pgxntool 2.3.0's own fix for installcheck's ordering bug (#83) made installcheck unconditionally depend on install, which writes a real .control file to disk - defeating the entire point of proving a pg_tle deployment never touches the filesystem. There's currently no way to invoke the real pgTAP suite without a filesystem install happening first. The fresh-install pg_tle smoke test (already on master via #16) is unaffected, since it never calls installcheck. Verified locally against PG17: fresh/update x empty/Quoted all pass via make verify-results; bin/compare_fresh_vs_update reports identical definitions for both schema legs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ob name cat_tools#67 proposes generalizing this script's object-discovery-and- diff technique into a real cat_tools feature; note it as a natural follow-up. Also fix test/README.md's mention of the now-removed extension-update-test job (folded into test earlier in this stack).
Adapts the intent of #35 (branched before pg-upgrade-test's twin-database redesign, now stale) to the current job shape: prepare-old/update/run-suite already loop bin/test_existing over count_nulls_upgrade_none (schema "") and count_nulls_upgrade_quoted (schema Quoted) sharing one binary pg_upgrade call. Adds an EXISTING_DB argument to bin/compare_fresh_vs_update so it can structurally diff an already-populated database (the real pg_upgraded one) against a fresh install, instead of only ever creating its own scratch "updated" database - then loops that comparison over both databases/schemas after run-suite, same as extension-update-test already does for its own in-place update leg.
…comment The job it referred to was folded into test earlier in this stack; point at the test job's update leg instead.
…andboxing pgxntool 2.3.0's installcheck-ordering fix (pgxntool#83) made `installcheck` (and so `make test`) unconditionally depend on `install`, which writes a real .control file to disk - defeating the whole point of a pg_tle-only-deployment proof, and with no upstream fix yet (pgxntool#90). bin/test_existing's run_suite gains a TEST_EXISTING_DEPLOY=pgtle mode that instead redirects `make test`'s install step through a scratch DESTDIR (harmless here since a pg_tle-deployed database never needs those files), pre-seeding a stub pgtap.control at the DESTDIR-prefixed path pgxntool's `pgtap` prerequisite checks for - otherwise its DESTDIR-blind recipe (`pgxn install pgtap --sudo`) would trigger a real, unwanted reinstall against the scratch dir. Also fixes test/install/load.sql's existing-mode assertion, which cross-checks the installed version against pg_available_extensions.default_version - that view never sees pg_tle registrations (it only reads .control files off disk), so it would return NULL under a pg_tle deployment even though CREATE EXTENSION resolves correctly through pg_tle's own catalog. A new count_nulls.test_existing_deploy GUC (driven by the same TEST_EXISTING_DEPLOY var, Makefile-exported like TEST_SCHEMA/TEST_LOAD_SOURCE) selects pgtle.available_extensions() instead when deploy=pgtle. The pg-tle-test CI job now installs count_nulls at 0.9.6 into a second scratch database (inheriting the job's template1 pg_tle registration), plants + proves the dependency guard, ALTER EXTENSION UPDATEs to current, then runs the real pgTAP suite against it via the new mode - with bin/assert_fs_clean verify checked after every one of those steps, not just at the end, since that's what actually proves the DESTDIR sandboxing worked. Verified locally (no real pg_tle in this container): confirmed via mtime comparison that the real count_nulls.control and pgtap.control are untouched by a full prepare-old/update/run-suite(pgtle) cycle, that the scratch DESTDIR is cleaned up on both success and a forced regression failure (EXIT trap surviving `set -e`), and exercised the count_nulls.test_existing_deploy branch in load.sql against a stubbed pgtle.available_extensions() function. The genuine pg_tle-registered version needs a real CI run to confirm end to end. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tion gh stack rebase cascades a push to every PR above the one actually changed, at the SAME priority as real new commits - a stacked PR's own runner budget gets crowded out by rebase noise from pushes that introduce no new content. bin/patch_id_hash tells the two apart via a base-independent content hash; a synchronize push whose hash matches the last one observed for this PR routes to a small shared low-priority lane instead of the per-PR high-priority one, still running the full matrix either way (a rebase can break things, and merges wait on a clean run regardless) - just possibly later. A base retarget (edited, gh stack promoting this PR to the bottom of its stack) or a draft PR marked ready escalates to an immediate high-priority run bypassing whatever lane its last push landed in, unless a full run already exists for that exact SHA. ci.yml's pull_request trigger previously had no `edited`/`ready_for_review` in its types, so neither of those could fire at all before now. claude-code-review.yml gets the equivalent skip on pure rebase pushes (nothing new to review), restricted to `synchronize` so it never suppresses the review on ready_for_review/opened/reopened.
|
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 is reviewing this PR
|
|
Claude finished @jnasbyupgrade's task in 6m 56s —— View job Claude finished reviewing PR #50Todo list
Note: this PR's own commit ( Findings1. (Moderate) Same-SHA self-cancellation can poison This is different from (and does not invalidate) the routine case — a fresh push cancelling a run for the previous, now-stale SHA — which is harmless because branch protection only ever evaluates the current head SHA. It's specifically the same-SHA cancellation this PR newly introduces (via the 2. (Minor-moderate) 3. (Minor) Every What looks solid
As the PR body notes, the low-priority-lane path and the escalate path aren't exercised by CI on this PR itself — worth the manual check on a real stacked PR the author already flagged, and finding #1 above is exactly the kind of interaction that manual check might not surface unless the retarget happens to race an in-flight low-priority run. |
1 participant
gh stack rebase cascades a push to every PR above the one actually
changed, at the SAME priority as real new commits - a stacked PR's
own runner budget gets crowded out by rebase noise from pushes that
introduce no new content. bin/patch_id_hash tells the two apart via
a base-independent content hash; a synchronize push whose hash
matches the last one observed for this PR routes to a small shared
low-priority lane instead of the per-PR high-priority one, still
running the full matrix either way (a rebase can break things, and
merges wait on a clean run regardless) - just possibly later.
A base retarget (edited, gh stack promoting this PR to the bottom of
its stack) or a draft PR marked ready escalates to an immediate
high-priority run bypassing whatever lane its last push landed in,
unless a full run already exists for that exact SHA. ci.yml's
pull_request trigger previously had no edited/ready_for_review in
its types, so neither of those could fire at all before now.
claude-code-review.yml gets the equivalent skip on pure rebase pushes
(nothing new to review), restricted to synchronize so it never
suppresses the review on ready_for_review/opened/reopened.
Not exercised by CI on this PR itself: the low-priority-lane path
(needs two pushes with an unchanged patch-id - reachable by pushing
an empty commit as a follow-up) and the escalate path (needs a real
base retarget or draft->ready transition). Worth a manual check on
a real stacked PR before trusting this broadly.