Switch to pgxn-tools based testing - #5
Conversation
📝 WalkthroughWalkthroughAdds GitHub Actions CI for PostgreSQL versions 17 through 10 using the Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant pgxn/pgxn-tools
participant PostgreSQL
GitHub Actions->>pgxn/pgxn-tools: Run make test with PGUSER=postgres
pgxn/pgxn-tools->>PostgreSQL: Execute tests for matrix version
PostgreSQL-->>pgxn/pgxn-tools: Return test results
pgxn/pgxn-tools-->>GitHub Actions: Return test status
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
1-18: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd a
permissionsblock to restrict default token permissions.The workflow has no
permissions:block, so theGITHUB_TOKENgets the repo's default permissions, which may include write access to contents, packages, etc. Since this job only runs tests, it should use read-only or empty permissions.🔒️ Proposed fix
name: CI on: [push, pull_request] +permissions: + contents: read jobs: test:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 1 - 18, Add a top-level permissions block to the CI workflow, before jobs, granting the GITHUB_TOKEN no permissions (or only the minimum read access required by actions/checkout). Keep the existing test job, PostgreSQL matrix, and steps unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 10-15: Pin the pgxn/pgxn-tools container to an immutable image
digest and pin actions/checkout to a full commit SHA instead of mutable tags. In
the checkout step, set persist-credentials to false.
In `@pgxntool/base.mk`:
- Around line 60-64: Fix the version-gated condition in the Makefile by changing
the malformed `ifeq` expression to use `$(call test, $(MAJORVER), -lt, 130)`,
matching the argument pattern used by the existing condition and the scaled
`MAJORVER` values. Keep the `REGRESS_OPTS += --load-language=plpgsql` assignment
unchanged.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-18: Add a top-level permissions block to the CI workflow, before
jobs, granting the GITHUB_TOKEN no permissions (or only the minimum read access
required by actions/checkout). Keep the existing test job, PostgreSQL matrix,
and steps unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f7a47839-f028-4d74-87c1-b3482fecc296
📒 Files selected for processing (9)
.github/workflows/ci.yml.gitignore.travis.ymlpg-travis-test.shpgxntool/HISTORY.ascpgxntool/base.mkpgxntool/setup.shsql/.object_reference.sql.swotest/dump/run.sh
💤 Files with no reviewable changes (2)
- .travis.yml
- pg-travis-test.sh
| endif | ||
|
|
||
| #DATA = $(wildcard sql/*--*.sql) | ||
| ifeq ($($call test, $(MAJORVER), -lt 13), yes) | ||
| REGRESS_OPTS += --load-language=plpgsql | ||
| endif |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix Makefile syntax error: --load-language=plpgsql is never added for any PostgreSQL version.
Line 62 has two defects that prevent the version-gated REGRESS_OPTS addition from ever executing:
-
$($calltypo — should be$(call. The extra$causes GNU Make to interpret$cas a single-char variable reference (undefined → empty), making the outer$(all test, …)resolve to an undefined variable → empty string. Theifeqalways evaluates to false. -
Missing comma and wrong comparison value —
-lt 13is passed as a single argument to thetestfunction (which expects 3 comma-separated args), and the value should be130not13becauseMAJORVERis already multiplied by 10 (line 51: PG 12 → 120, PG 13 → 130). Even with the$(callfix,test 120 -lt 13is false for PG 12.
Compare with the correct pattern on line 56: $(call test, $(MAJORVER), -ge, 91).
🐛 Proposed fix for line 62
-ifeq ($($call test, $(MAJORVER), -lt 13), yes)
+ifeq ($(call test, $(MAJORVER), -lt, 130), yes)
REGRESS_OPTS += --load-language=plpgsql
endif📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| endif | |
| #DATA = $(wildcard sql/*--*.sql) | |
| ifeq ($($call test, $(MAJORVER), -lt 13), yes) | |
| REGRESS_OPTS += --load-language=plpgsql | |
| endif | |
| endif | |
| ifeq ($(call test, $(MAJORVER), -lt, 130), yes) | |
| REGRESS_OPTS += --load-language=plpgsql | |
| endif |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pgxntool/base.mk` around lines 60 - 64, Fix the version-gated condition in
the Makefile by changing the malformed `ifeq` expression to use `$(call test,
$(MAJORVER), -lt, 130)`, matching the argument pattern used by the existing
condition and the scaled `MAJORVER` values. Keep the `REGRESS_OPTS +=
--load-language=plpgsql` assignment unchanged.
|
@claude please review this PR. |
|
Claude finished @jnasbyupgrade's task in 2m 51s —— View job Review: Switch to pgxn-tools based testing
SummaryThis is a clean re-application of the original pgxn-tools CI migration (matches the 1. Confirmed: pre-existing This reproduces on every PostgreSQL version tested (10–17), confirming this is the pre-existing bug the PR description says is fixed separately in the 2. Real bug (independent of the above): ifeq ($($call test, $(MAJORVER), -lt 13), yes)
REGRESS_OPTS += --load-language=plpgsql
endifTwo problems:
Correct form: ifeq ($(call test, $(MAJORVER), -lt, 130), yes)
REGRESS_OPTS += --load-language=plpgsql
endifThis means PG <13 regression runs are silently missing 3. Optional hardening (not blocking):
None of these affect correctness or the current failures; worth doing at some point but not a reason to hold this PR. 4. Minor/unrelated: the 5. Minor suggestion: the matrix has no Bottom lineThe CI plumbing itself (workflow file, dump script |
4fe08c0 to
4531d10
Compare
Rebuilt fresh on top of current master, containing only the actual CI/pgxn-tools migration (the SQL style linter integration and the cat_tools 0.3.0 dependency fix that had scope-crept into this branch are split out to #16 and #28 respectively). - Add .github/workflows/ci.yml: a `changes` job (docs-only gate + PG-major-matrix derivation from two constants), a `test` matrix job (container: pgxn/pgxn-tools, PostgreSQL 12-18), and an `all-checks-passed` aggregation gate for use as a single stable required status check. - Remove .travis.yml and pg-travis-test.sh, superseded by the above. - test/dump/run.sh: add -X to several psql invocations, disabling ~/.psqlrc so test runs are deterministic.
…ls 0.3.0 pgxn install --unstable cat_tools now resolves to cat_tools 0.3.0 directly (confirmed against the live PGXN index and by a clean CREATE EXTENSION cat_tools; both locally and, once pushed, in the actual object_reference CI run for Postgres-Extensions#5's slimmed CI-migration branch -- it went fully green without this Makefile change at all). The PGXN package index being stuck at the broken, 2017-era 0.2.1 release was true when this fix was first written, but isn't true anymore, so the git-clone-from-tag workaround has nothing left to work around. Keeping it would leave a Makefile comment describing a problem that no longer exists. The SQL/test fixes (renamed function call, new object_type enum members classified as unsupported) are unaffected -- those are needed regardless of how cat_tools 0.3.0 gets installed.
1 participant
Re-opens the pgxn-tools testing work (originally PR #1, which was merged as a merge commit rather than a squash; master has since been rolled back to the pre-merge state
d191ef1).Rescoped: this branch previously scope-crept into also carrying the SQL style linter integration (a duplicate of #16's content, pulled in by a merge to resolve a
ci.ymlconflict) and a cat_tools 0.3.0 dependency fix. Both have been split out:object_typeenum members classified as unsupported) is now Adapt to cat_tools 0.3.0: renamed function, two new object types #28.This branch is rebuilt fresh on top of current
mastercontaining only the actual CI/pgxn-tools migration:.github/workflows/ci.yml(new): achangesjob (docs-only gate + PG-major-matrix derivation from two constants,NEWEST=18/CURRENT_FLOOR=12), atestmatrix job (container: pgxn/pgxn-tools, PostgreSQL 12-18), and anall-checks-passedaggregation gate..travis.ymlandpg-travis-test.shdeleted (superseded by the above).test/dump/run.sh: added-Xflags to severalpsqlinvocations, disabling~/.psqlrcso test runs are deterministic.(
.gitignore's.claude/settings.local.jsonline from the original diff was dropped — currentmasteralready ignores it via the broader existing.claude/*.local.jsonpattern.)CI status
Live and green: this branch's own CI (all 7 PostgreSQL majors, 12-18, plus
all-checks-passed) passes cleanly as-is. Earlier drafts of this split assumed it would need #28 (cat_tools 0.3.0) to merge first, since PGXN's package index used to be stuck on a broken0.2.1cat_tools release — that's no longer the case, PGXN now serves0.3.0directly, somake test'spgxn install --unstable cat_toolsalready resolves correctly without any change here.Once #16 (SQL style linter) also merges, a small follow-up will be needed here to fold its
lintjob into thisci.yml(as the first, top-priority job ahead of thetestmatrix, per the reasoning previously written up when the two were briefly merged together). Not done in this PR — keeping this branch scoped to just the CI/pgxn-tools migration.#28 (cat_tools 0.3.0 adaptation) is unrelated to this PR going green, but is still a real, worthwhile correctness fix independent of CI — no required merge order between the two now.