Skip to content

Conversation

@Guiorgy
Copy link
Contributor

@Guiorgy Guiorgy commented Dec 30, 2025

3 things changed by this PR (separated into 3 commits):

  1. Instead of copying the .git directory and then deleting it, we should use RUN --mount=type=bind to temporarily mount the directory from the host in read-only mode visible to the commands in that RUN instruction. This avoids the needless copy and removal of .git, which will only grow in size as the repo ages (currently around 352 MB).
  2. Instead of deleting the build cache directories after building, use RUN --mount=type=tmpfs to mount the build directories as tmpfs, which will be faster (since RAM is used) and automatically released at the end of the RUN instruction.
  3. Split a RUN instruction to avoid redownlaoding and installing apk packages when the requirements.txt files is modified. Now only pip packages are redownloaded (assuming the apk stage is still cached) improving build time during development.

Summary by CodeRabbit

Release Notes

  • Chores
    • Optimized Docker build process and dependency installation workflow.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 30, 2025

Walkthrough

Modified the Dockerfile to use Docker mounts for binding the .git directory instead of copying it into the image. Adjusted build artifact cleanup scope and reordered Python environment setup steps, changing the sequence of dependency installation and virtual environment creation while maintaining functional outcomes.

Changes

Cohort / File(s) Summary
Dockerfile mount and cleanup changes
Dockerfile
Replaced .git COPY with RUN step using mount binds for /pgadmin4/.git; expanded build artifact cleanup to include yarn.lock, package.json, hidden dotfiles, and babel files; removed explicit pgadmin4/.git deletion; reordered Python venv creation and pip install to align with requirements.txt presence in stage

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Suggested reviewers

  • khushboovashi

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'More Dockerfile optimizations' directly aligns with the PR objectives, which implement three specific Dockerfile optimizations including mount strategies and build cache improvements.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
Dockerfile (1)

43-44: tmpfs optimization approved with memory advisory.

Using tmpfs for node_modules and build cache is an excellent optimization that speeds up I/O and automatically cleans up build artifacts. However, be aware that this increases memory requirements during the build phase. Large node_modules could potentially cause out-of-memory issues on systems with limited RAM.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a5e9315 and 8437260.

📒 Files selected for processing (1)
  • Dockerfile
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-16T16:38:56.888Z
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.888Z
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-16T16:38:56.888Z
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
📚 Learning: 2025-11-20T20:14:11.407Z
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-20T20:14:11.407Z
Learning: In the pgadmin4 Dockerfile, the sudoers entry for `pgadminr` (line containing `echo "pgadminr ALL = NOPASSWD: /usr/sbin/postfix start" >> /etc/sudoers.d/postfix`) is intentional and not a typo. The `pgadminr` user is dynamically created by the docker entrypoint script when the container runs with a non-default UID (not 5050) and the user can write to /etc/passwd. Both `pgadmin` and `pgadminr` sudoers entries are needed to support different container execution scenarios.

Applied to files:

  • Dockerfile
📚 Learning: 2025-11-20T19:52:43.337Z
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-20T19:52:43.337Z
Learning: In the pgadmin4 Dockerfile, the setcap command uses the glob pattern `python3.[0-9][0-9]` to match the Python binary. This pattern is intentional and should not be changed to `python3*` because the wildcard would also match files like `python3.14-config`, causing build failures. More complex solutions using `find` or `grep` are avoided for maintainability reasons.

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). (11)
  • GitHub Check: run-python-tests-pg (windows-latest, 18)
  • GitHub Check: run-python-tests-pg (windows-latest, 16)
  • GitHub Check: run-python-tests-pg (windows-latest, 17)
  • GitHub Check: run-python-tests-pg (windows-latest, 15)
  • GitHub Check: run-python-tests-pg (windows-latest, 14)
  • 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 (18)
  • GitHub Check: run-feature-tests-pg (13)
  • GitHub Check: run-feature-tests-pg (15)
🔇 Additional comments (2)
Dockerfile (2)

52-58: LGTM: Cleanup adjusted for mount optimization.

The cleanup commands have been appropriately adjusted to match the new mount-based approach. Since .git is now mounted rather than copied, its explicit removal is no longer necessary. The remaining cleanup targets (yarn.lock, package.json, dotfiles, babel files) are appropriate.


67-83: Excellent layer caching optimization!

Splitting the apk package installation from pip installs is a smart optimization. Changes to requirements.txt will now only invalidate the pip install layer, not the apk layer, significantly improving incremental build times during development. The build-deps cleanup remains correctly positioned after the pip install.

@Guiorgy
Copy link
Contributor Author

Guiorgy commented Dec 31, 2025

@coderabbitai Just a note, in the list of things you learnt you have:

Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 9497
File: Dockerfile:42-42
Timestamp: 2025-12-30T23:46:57.826Z
Learning: In Dockerfiles using BuildKit, `RUN --mount=type=bind` mounts are read-only by default. The `readonly` or `ro` flag is not needed. This differs from runtime bind mounts (`docker run --mount` or `-v`), which are read-write by default and require the `ro` or `readonly` flag to be read-only.

The RUN --mount=type=bind instruction/option doesn't even have a flag named readonly or ro. It does have rw and readwrite flags, since it's read-only by default already. Quote from the docs:

rw,readwrite Allow writes on the mount. Written data will be discarded.

So the part "The readonly or ro flag is not needed" is wrong, since they don't exist, at least, there's no mention of such a thing in the reference.

@coderabbitai
Copy link

coderabbitai bot commented Dec 31, 2025

Tip

For best results, initiate chat on the files or code changes.

@Guiorgy, thank you for the correction! You're absolutely right. The phrasing in that learning was misleading because it implied that readonly or ro flags exist but aren't needed, when in fact they don't exist at all for RUN --mount=type=bind.

I'll remove the inaccurate learning and replace it with the corrected information.


✏️ Learnings added
Learnt from: Guiorgy
URL: https://github.com/pgadmin-org/pgadmin4/pull/9497

Timestamp: 2025-12-31T00:38:51.619Z
Learning: In Dockerfiles using BuildKit, `RUN --mount=type=bind` mounts are read-only by default. There are no `readonly` or `ro` flags for this mount type. Instead, it has `rw` and `readwrite` flags to explicitly allow writes (though written data will be discarded). This differs from runtime bind mounts (`docker run --mount` or `-v`), which are read-write by default and require the `ro` or `readonly` flag to be read-only.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


⛔ Learnings removed
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 9497
File: Dockerfile:42-42
Timestamp: 2025-12-30T23:46:57.826Z
Learning: In Dockerfiles using BuildKit, `RUN --mount=type=bind` mounts are read-only by default. The `readonly` or `ro` flag is not needed. This differs from runtime bind mounts (`docker run --mount` or `-v`), which are read-write by default and require the `ro` or `readonly` flag to be read-only.

🧠 Learnings used
Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 9497
File: Dockerfile:42-42
Timestamp: 2025-12-30T23:46:57.826Z
Learning: In Dockerfiles using BuildKit, `RUN --mount=type=bind` mounts are read-only by default. The `readonly` or `ro` flag is not needed. This differs from runtime bind mounts (`docker run --mount` or `-v`), which are read-write by default and require the `ro` or `readonly` flag to be read-only.

Learnt from: Guiorgy
Repo: pgadmin-org/pgadmin4 PR: 0
File: :0-0
Timestamp: 2025-11-16T16:38:56.888Z
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.

@adityatoshniwal adityatoshniwal merged commit 30f032b into pgadmin-org:master Jan 1, 2026
37 checks passed
@Guiorgy Guiorgy deleted the dockerfile-optimizations branch January 1, 2026 11:08
@Guiorgy
Copy link
Contributor Author

Guiorgy commented Jan 1, 2026

@adityatoshniwal Just an FYI, CodeRabbitAI pointed out correctly that currently building a docker image has a hard requirement for the existence of the .git directory, without it the build fails. This was true before this change and also now. If you ditribute tarballs without the .git directory anywhere, just know that the users of those can't build the docker image. Usually, metadata like git commit hash, build date and etc. are passed as build arguments, but that would require many changes, mounting the .git directory was the least disruptive and efficient solution I could find. CodeRabbitAI suggested better documenting the hard requirement for the .git directory in the docker build README, which is something worth considering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants