Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The Dockerfile changes introduce multi-arch build breakage (arm64) and likely regress major-upgrade/test expectations by removing older supported PostgreSQL versions and shifting the default PG version without coordinating CI/workflow updates.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates the Spilo postgres-appliance build defaults and documentation, apparently to target PostgreSQL 16 and to build via Docker Buildx with an explicit platform.
Changes:
- Switched README build instructions from
docker buildtodocker buildx build --platform linux/amd64 .... - Changed the Dockerfile default
PGVERSIONfrom 18 to 16 and adjusted the supported Debian PG versions list. - Added
--platform=linux/amd64to multipleFROMstages in the Dockerfile.
File summaries
| File | Description |
|---|---|
| README.rst | Updates local build instructions to use buildx with an explicit platform. |
| postgres-appliance/Dockerfile | Adjusts default PostgreSQL version and pins build stages to a specific platform. |
Review details
Suppressed comments (4)
postgres-appliance/Dockerfile:17
- Hard-coding
--platform=linux/amd64here forces the stage to amd64 even when building for arm64 (the publish workflow builds both). Use the target platform instead so multi-arch builds work.
FROM --platform=linux/amd64 $BASE_IMAGE as dependencies-builder
postgres-appliance/Dockerfile:30
- Hard-coding
--platform=linux/amd64here forces the stage to amd64 even when building for arm64 (the publish workflow builds both). Use the target platform instead so multi-arch builds work.
FROM --platform=linux/amd64 $BASE_IMAGE as builder-false
postgres-appliance/Dockerfile:81
- Hard-coding
--platform=linux/amd64here forces this stage to amd64 even when building for arm64 (the publish workflow builds both). Use the target platform instead so multi-arch builds work.
FROM --platform=linux/amd64 scratch as builder-true
postgres-appliance/Dockerfile:85
- Hard-coding
--platform=linux/amd64here forces the final stage to amd64 even when building for arm64 (the publish workflow builds both). Use the target platform instead so multi-arch builds work.
FROM --platform=linux/amd64 builder-${COMPRESS}
- Files reviewed: 2/2 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -1,11 +1,11 @@ | |||
| ARG BASE_IMAGE=ubuntu:22.04 | |||
| ARG PGVERSION=18 | |||
| ARG PGVERSION=16 | |||
|
|
||
|
|
||
| FROM ubuntu:18.04 as ubuntu-18 | ||
| FROM --platform=linux/amd64 ubuntu:18.04 as ubuntu-18 |
| $ cd postgres-appliance | ||
|
|
||
| $ docker build --tag $YOUR_TAG . | ||
| $ docker buildx build --platform linux/amd64 -t <IMAGE REGISTTRY>:<VERSION> . |
| ARG COMPRESS | ||
| ARG PGOLDVERSIONS="14 15 16 17" | ||
| ARG WITH_PERL=false | ||
|
|
||
| ARG DEB_PG_SUPPORTED_VERSIONS="$PGOLDVERSIONS $PGVERSION" | ||
| ARG DEB_PG_SUPPORTED_VERSIONS="$PGVERSION" |
There was a problem hiding this comment.
Wait is this going to be an issue when we want to bump to PG 17?
Dan-Pitt-VQ
left a comment
Dan-Pitt-VQ
left a comment
There was a problem hiding this comment.
copilots moans about hardcoded os are non relevant currently as we control and know the target OS which afaik isn't changing anytime soon
| ARG COMPRESS | ||
| ARG PGOLDVERSIONS="14 15 16 17" | ||
| ARG WITH_PERL=false | ||
|
|
||
| ARG DEB_PG_SUPPORTED_VERSIONS="$PGOLDVERSIONS $PGVERSION" | ||
| ARG DEB_PG_SUPPORTED_VERSIONS="$PGVERSION" |
There was a problem hiding this comment.
Wait is this going to be an issue when we want to bump to PG 17?
Tested and working