Switch to pgxn-tools based testing#5
Conversation
e9c24de Fix pg_regress on versions > 12 (#5) c0af00f Improvements to HISTORY.asc 6e8f2a7 Allow use of sudo when installing an extension 705f1ec Don't run clean as part of make test 370fa8e Create test/sql during setup git-subtree-dir: pgxntool git-subtree-split: e9c24de986ddc85bbd1fb3149076888d075ce100
Ditch the old travis setup. Pulls in some pgxntool changes as well.
📝 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 |
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).This branch holds the original PR #1 content (the
728f815merge state). Intended to be squash-merged once CI is green.Note: CI on this branch surfaces a pre-existing
column "oid" specified more than onceerror atCREATE EXTENSION— the fix for that lives in the separatenew_featuresbranch (PR #2), so that needs sorting before/with this merge.