Optimized Dockerfile #9378
Optimized Dockerfile #9378
Conversation
WalkthroughThe Dockerfile was updated to consolidate runtime dependency installation and library copies, add an explicit liblz4 symlink, combine Docker script copies, and centralize final runtime configuration (gunicorn install, user and directory setup, permissions, and entrypoint/runtime metadata). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
Dockerfile (2)
166-179: Verify apk update/upgrade impact on image reproducibility and build caching.The addition of
apk update && apk upgradeat lines 167 will always fetch the latest package versions, which can cause layer cache invalidation on every build even when source code hasn't changed. This defeats part of the caching benefit intended by moving dependencies to an earlier stage.While the PR objective mentions upgrading packages to keep the image up-to-date, consider:
- Whether this should be done selectively (upgrade only if explicitly requested)
- Whether pinning specific package versions would be better for reproducibility
- Whether
docker build --no-cache(mentioned in PR objectives) is the intended control mechanismIf the intent is to always get the latest patches for security, the current approach is correct but understand it will re-run this layer frequently.
204-217: Consolidation of final RUN step looks sound; verify wildcard in setcap command.The consolidation of configuration into a single RUN instruction (lines 204-217) is good for layer optimization. However, line 215 uses a wildcard pattern that may not match all Python versions reliably:
setcap CAP_NET_BIND_SERVICE=+eip /usr/local/bin/python3.[0-9][0-9]This pattern assumes Python 3.X version format. If the image contains Python 3.10 or 3.100+ in the future, this could fail silently or match unexpectedly. Consider:
- Explicitly discovering the Python version:
$(python3 -c "import sys; print(f'python{sys.version_info.major}.{sys.version_info.minor}')")- Or using a more flexible glob:
/usr/local/bin/python3*- setcap CAP_NET_BIND_SERVICE=+eip /usr/local/bin/python3.[0-9][0-9] && \ + setcap CAP_NET_BIND_SERVICE=+eip /usr/local/bin/python3.* && \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-16T16:38:56.851Z
Learning: In the pgadmin4 repository, when using the python:3-alpine Docker base image, libzstd is already included and does not need to be explicitly installed or copied from other build stages.
📚 Learning: 2025-11-16T16:38:56.851Z
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-16T16:38:56.851Z
Learning: In the pgadmin4 repository, when using the python:3-alpine Docker base image, libzstd is already included and does not need to be explicitly installed or copied from other build stages.
Applied to files:
Dockerfile
🪛 Hadolint (2.14.0)
Dockerfile
[error] 198-198: COPY with more than 2 arguments requires the last argument to end with /
(DL3021)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: run-python-tests-pg (windows-latest, 16)
- GitHub Check: run-python-tests-pg (ubuntu-22.04, 14)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (15)
🔇 Additional comments (1)
Dockerfile (1)
186-186: I'll examine the Dockerfile to understand the context and verify the concern about the source path.Let me examine the Dockerfile structure around line 186:
Perfect—I found the actual Dockerfile! The Dockerfile shows three separate COPY commands from pg18-builder: one for
/usr/local/lib/libpq.so.5.18, one for/usr/lib/libzstd.so.1.5.7, and one for/usr/lib/liblz4.so.1.10.0, all to/usr/lib/.Looking at this more carefully, the review comment's concern about the path being "destination-like" is incorrect. The path
/usr/lib/liblz4.so.1.10.0is actually the correct source location within the pg18-builder image (which is based on postgres:18-alpine). The COPY syntax is valid.However, the underlying issue is real: pgadmin4 users reported missing shared libraries libzstd.so.1 and liblz4.so.1 when running pg_dump with the latest Docker image version 7.7. This indicates these files are either not present in the pg18-builder stage or not being properly included in the final image.
Given the retrieved learning that libzstd is already included in python:3-alpine, and the confirmed production bug reports, the review comment needs significant correction.
Source paths for liblz4.so.1.10.0 and libzstd.so.1.5.7 are correct, but production runtime failures confirm the actual issue exists upstream.
The COPY commands correctly reference
/usr/lib/liblz4.so.1.10.0and/usr/lib/libzstd.so.1.5.7from pg18-builder—these are valid source paths within the postgres:18-alpine-based builder stage. The syntax is not a copy-paste error as suggested.However, pgadmin4 users have reported that liblz4.so.1 and libzstd.so.1 are missing at runtime in the Docker image version 7.7, which confirms a real problem exists. Rather than an issue with the COPY syntax or path notation, the concern should focus on:
- Whether pg18-builder actually contains these libraries after the build
- Whether the symbolic links created in the RUN statement (lines following the COPYs) are sufficient
The review comment should be revised to focus on verifying that pg18-builder properly includes these runtime dependencies after compilation, rather than questioning the source path notation itself.
Likely an incorrect or invalid review comment.
Guiorgy
force-pushed
the
optimize-dockerfile
branch
from
1a0dfd7 to
4ab2fe0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Dockerfile (2)
215-215: Potential issue: Python version glob pattern may be too restrictive.The glob pattern
python3.[0-9][0-9]only matches exactly two digits, which works for current versions (e.g.,python3.10,python3.12) but will fail for single-digit minor versions (e.g.,python3.9) or future triple-digit versions (e.g.,python3.100).Consider using a more flexible pattern:
- setcap CAP_NET_BIND_SERVICE=+eip /usr/local/bin/python3.[0-9][0-9] && \ + setcap CAP_NET_BIND_SERVICE=+eip /usr/local/bin/python3* && \Or, if more strict matching is preferred, use a
findcommand instead of a glob.
217-217: Fix sudoers entry typo: "pgadminr" should be "pgadmin".Line 217 references user
pgadminr(with extra 'r'), but the user created on line 207 ispgadmin. This typo means the sudoers rule on line 217 applies to a non-existent user.- echo "pgadminr ALL = NOPASSWD: /usr/sbin/postfix start" >> /etc/sudoers.d/postfix + echo "pgadmin ALL = NOPASSWD: /usr/sbin/postfix start" >> /etc/sudoers.d/postfix
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-16T16:38:56.851Z
Learning: In the pgadmin4 repository, when using the python:3-alpine Docker base image, libzstd is already included and does not need to be explicitly installed or copied from other build stages.
📚 Learning: 2025-11-16T16:38:56.851Z
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-16T16:38:56.851Z
Learning: In the pgadmin4 repository, when using the python:3-alpine Docker base image, libzstd is already included and does not need to be explicitly installed or copied from other build stages.
Applied to files:
Dockerfile
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: run-feature-tests-pg (14)
- GitHub Check: run-feature-tests-pg (18)
- GitHub Check: run-feature-tests-pg (17)
- GitHub Check: run-feature-tests-pg (13)
- GitHub Check: run-feature-tests-pg (16)
- GitHub Check: run-feature-tests-pg (15)
- GitHub Check: run-python-tests-pg (windows-latest, 15)
- GitHub Check: run-python-tests-pg (windows-latest, 16)
- GitHub Check: run-python-tests-pg (windows-latest, 17)
🔇 Additional comments (2)
Dockerfile (2)
166-179: Good consolidation of runtime dependencies for cache optimization.The early placement and cache cleanup align well with the PR objectives to improve build cache layers and keep dependencies up-to-date.
198-198: Good fix to the multi-source COPY destination.The destination now correctly ends with
/for multi-source COPY operations, resolving the past Hadolint DL3021 error. File consolidation reduces layers as intended.
This code was already present, and already discussed in the previous PR. No,
We want to keep it as simple as possible for maintainability.
I havent touched that part of the code, though it does indeed look weird. Here is from inside the latest docker container: $ sudo docker exec -it -u root PgAdmin4 sh
# cat /etc/sudoers.d/postfix
pgadmin ALL = NOPASSWD: /usr/sbin/postfix start
pgadminr ALL = NOPASSWD: /usr/sbin/postfix start
# cat /etc/passwd
root:x:0:0:root:/root:/bin/sh
bin:x:1:1:bin:/bin:/sbin/nologin
daemon:x:2:2:daemon:/sbin:/sbin/nologin
lp:x:4:7:lp:/var/spool/lpd:/sbin/nologin
sync:x:5:0:sync:/sbin:/bin/sync
shutdown:x:6:0:shutdown:/sbin:/sbin/shutdown
halt:x:7:0:halt:/sbin:/sbin/halt
mail:x:8:12:mail:/var/mail:/sbin/nologin
news:x:9:13:news:/usr/lib/news:/sbin/nologin
uucp:x:10:14:uucp:/var/spool/uucppublic:/sbin/nologin
cron:x:16:16:cron:/var/spool/cron:/sbin/nologin
ftp:x:21:21::/var/lib/ftp:/sbin/nologin
sshd:x:22:22:sshd:/dev/null:/sbin/nologin
games:x:35:35:games:/usr/games:/sbin/nologin
ntp:x:123:123:NTP:/var/empty:/sbin/nologin
guest:x:405:100:guest:/dev/null:/sbin/nologin
nobody:x:65534:65534:nobody:/:/sbin/nologin
postfix:x:100:101:postfix:/var/spool/postfix:/sbin/nologin
vmail:x:101:102:vmail:/var/mail/domains:/sbin/nologin
pgadmin:x:5050:0::/home/pgadmin:/sbin/nologinThe 2 lines are present in the |
|
Tip For best results, initiate chat on the files or code changes. @Guiorgy, I apologize for repeating the suggestion about the Python glob pattern. I've noted your feedback about why the current pattern Regarding the sudoers entry, you're right that this is pre-existing code not touched in this PR. The evidence you've provided clearly shows that the ✏️ Learnings added
🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. @Guiorgy, I apologize for repeating the suggestion about the Python glob pattern. I can see from my memory that this was already discussed and the pattern Regarding the sudoers entry with Your PR optimizations look good and are well-scoped to the Dockerfile improvements you intended to make. 🧠 Learnings used |
|
@coderabbitai I greped the codebase and found the referense to Essentially, as far as I understand, if the user running the container is not the default |
|
Tip For best results, initiate chat on the files or code changes. @Guiorgy, thank you for investigating and providing this explanation! That makes perfect sense now. The I've noted this information for future reviews to avoid incorrectly flagging it as a typo. ✏️ Learnings added
🧠 Learnings used |
1 participant
docker build --no-cache ...can be used.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.