Centralize CI PostgreSQL ref selection via postgres-build.conf#543
Conversation
📝 WalkthroughWalkthroughChangesThe PR centralizes PostgreSQL ref selection in configuration and a resolver script, adds resolver tests, and updates CI, Docker, and PostgreSQL 18 installcheck source acquisition to use configurable refs. PostgreSQL ref resolution
Build entrypoints
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 |
Up to standards ✅🟢 Issues
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/pg-stable-test.yml:
- Line 7: Update the comment in the workflow header to remove the duplicated
“builds spock per the README” phrase, while preserving the surrounding
description about running regression tests.
In `@tests/run-single-pg18-installcheck.sh`:
- Around line 251-261: Update clone_pg to resolve PG_REF before the
cached-source early return, then validate the existing checkout’s HEAD against
that resolved ref. Only skip cloning when the source is valid and matches the
desired revision; otherwise refresh or re-clone it so config changes and moved
branches are applied.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 59e83bf2-a3be-433e-8083-61939392641c
📒 Files selected for processing (6)
.github/workflows/pg-stable-test.ymltests/docker/Dockerfile-step-1.el9tests/postgres-build.conftests/resolve-pg-ref.shtests/resolve-pg-ref.test.shtests/run-single-pg18-installcheck.sh
abc9709 to
c004732
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/resolve-pg-ref.sh`:
- Line 55: Update the no-tag error message in the ref-resolution flow around the
`die` call so it does not include `PG_GIT_REMOTE` or any unredacted remote URL.
Use a generic failure message while preserving the existing error condition and
`${major}` context.
- Around line 49-54: Update the tag-resolution logic assigning ref to capture
git ls-remote output in a separate tags variable and immediately fail closed
when that command returns a nonzero status. Then feed tags through printf '%s\n'
before the existing sed, tr, sort, tail, and final tr filters, preserving normal
successful tag resolution.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 48ef912a-ddeb-4232-a5b0-d14a4cb6c44d
📒 Files selected for processing (6)
.github/workflows/pg-stable-test.ymltests/docker/Dockerfile-step-1.el9tests/postgres-build.conftests/resolve-pg-ref.shtests/resolve-pg-ref.test.shtests/run-single-pg18-installcheck.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/postgres-build.conf
- .github/workflows/pg-stable-test.yml
- tests/run-single-pg18-installcheck.sh
- tests/docker/Dockerfile-step-1.el9
- tests/resolve-pg-ref.test.sh
c004732 to
4a955df
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/resolve-pg-ref.test.sh (1)
29-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake tag and commit-pin success paths deterministic.
Any resolver failure for
tagis currently reported as a network skip, masking parser/selection regressions. Also,REL_16_9does not exercise the promised raw commit-SHA path. Stub a successfulgit ls-remoteresponse with multipleREL_17_*tags and assert the newest is selected; setpg16_refto a full hexadecimal commit ID and assert it is emitted verbatim.🤖 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 `@tests/resolve-pg-ref.test.sh` around lines 29 - 108, Update the test fixture and assertions in the resolver test: replace pg16_ref with a full hexadecimal commit SHA and keep asserting that the resolver emits it verbatim. Make the pg17 tag test deterministic by stubbing the resolver’s git ls-remote dependency with multiple REL_17_* tags, assert the newest tag is selected, and remove the network-dependent skip path so any resolver failure is reported as a test failure.
🤖 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 `@tests/docker/Dockerfile-step-1.el9`:
- Around line 141-145: Update the PostgreSQL checkout flow around PG_REF so it
supports raw commit SHAs as well as branches and tags: initialize the
destination repository, fetch the resolved PG_REF into it, and checkout
FETCH_HEAD instead of using git clone --branch. Preserve the existing PG_REF
default resolution and logging, including reporting the checked-out short commit
and resolved reference.
In `@tests/resolve-pg-ref.sh`:
- Around line 66-70: Update the wildcard branch of the mode case in
resolve-pg-ref.sh so unsupported reference modes are rejected instead of copied
into ref. Accept only values matching the documented pinned-commit format, emit
those verbatim, and return a failure for unknown values while preserving the
existing branch and tag handling.
---
Nitpick comments:
In `@tests/resolve-pg-ref.test.sh`:
- Around line 29-108: Update the test fixture and assertions in the resolver
test: replace pg16_ref with a full hexadecimal commit SHA and keep asserting
that the resolver emits it verbatim. Make the pg17 tag test deterministic by
stubbing the resolver’s git ls-remote dependency with multiple REL_17_* tags,
assert the newest tag is selected, and remove the network-dependent skip path so
any resolver failure is reported as a test failure.
🪄 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: CHILL
Plan: Pro Plus
Run ID: e3039079-a4e9-4376-ac89-ba51f8234ba7
📒 Files selected for processing (6)
.github/workflows/pg-stable-test.ymltests/docker/Dockerfile-step-1.el9tests/postgres-build.conftests/resolve-pg-ref.shtests/resolve-pg-ref.test.shtests/run-single-pg18-installcheck.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/postgres-build.conf
- .github/workflows/pg-stable-test.yml
- tests/run-single-pg18-installcheck.sh
Resolve the PG source ref (tag | branch | pin) per major from one config plus resolver script, used by all three build sites: Dockerfile-step-1.el9, pg-stable-test.yml, and run-single-pg18-installcheck.sh. Default 15-19 to tag: spock's patches track the release tags, so tags apply cleanly today while REL_*_STABLE tips have drifted. Flip a major to branch alongside a patch refresh. Dockerfile keeps an optional PG_REF build-arg so ref resolution can later move to the runner for layer caching without further Dockerfile changes.
4a955df to
89632c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/resolve-pg-ref.test.sh (1)
116-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake tag-mode verification deterministic.
Any resolver failure is currently reported as a network skip, and the regex does not verify newest-tag selection. Add a successful stubbed
git ls-remotecase (for example,REL_17_9andREL_17_10) that must resolve toREL_17_10; retain this as an optional network smoke 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 `@tests/resolve-pg-ref.test.sh` around lines 116 - 125, Make the tag-mode checks in the test script deterministic by adding a successful stubbed git ls-remote scenario with tags such as REL_17_9 and REL_17_10, asserting the resolver returns REL_17_10. Keep the existing live network invocation as an optional smoke test, but distinguish resolver failures from network unavailability instead of always skipping them.
🤖 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 `@tests/resolve-pg-ref.sh`:
- Around line 58-63: Update the tag-selection pipeline assigning ref so
release-candidate tags matching REL_${major}_RC* cannot be chosen when final
numeric release tags are available. Filter out RC/prerelease tags before sort -V
and tail, while preserving the existing underscore/dot normalization and
final-release selection behavior.
---
Nitpick comments:
In `@tests/resolve-pg-ref.test.sh`:
- Around line 116-125: Make the tag-mode checks in the test script deterministic
by adding a successful stubbed git ls-remote scenario with tags such as REL_17_9
and REL_17_10, asserting the resolver returns REL_17_10. Keep the existing live
network invocation as an optional smoke test, but distinguish resolver failures
from network unavailability instead of always skipping them.
🪄 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: CHILL
Plan: Pro Plus
Run ID: e41f2585-2541-49c6-a3fb-1617c316f911
📒 Files selected for processing (6)
.github/workflows/pg-stable-test.ymltests/docker/Dockerfile-step-1.el9tests/postgres-build.conftests/resolve-pg-ref.shtests/resolve-pg-ref.test.shtests/run-single-pg18-installcheck.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/pg-stable-test.yml
- tests/run-single-pg18-installcheck.sh
- tests/postgres-build.conf
Resolve the PG source ref (tag | branch | pin) per major from one config plus resolver script, used by all three build sites:
Dockerfile-step-1.el9,pg-stable-test.yml, andrun-single-pg18-installcheck.sh. Default 15-19 to "tag".The purpose is to build against the most recent release tag most of the time, but allow for building against HEAD of Postgres major stable branches as we near new releases (before they have been stamped) by modifying
tests/postgres-build.conf. The default values in that file are:To build against the head of branches, replace
tagwithbranchhere. As mentioned above, one can also pin to a certain commit/ref.Also, Dockerfile keeps an optional PG_REF build-arg so ref resolution can later move to the runner for layer caching without further Dockerfile changes. It also allows for flexible building locally.