Skip to content

Conversation

@Guiorgy
Copy link
Contributor

@Guiorgy Guiorgy commented Dec 28, 2025

The following comment is wrong:

# Create the /pgadmin4 directory and copy the source into it. Explicitly
# remove the node_modules directory as we'll recreate a clean version, as well
# as various other files we don't want

It was left after a merge conflict resolution after merging #9377. As mentioned there, Explicitly remove the node_modules directory is wrong, since those files aren't even copied due to the use of the .dockerignore file.

Also, since I didn't get the answer, I'll try asking again, are we 100% sure we neet to copy the .git directory into the container during the build stage?

Edit: I found the use of .git inside web/package.json:

"bundle": "cross-env NODE_ENV=production NODE_OPTIONS=--max-old-space-size=3072 yarn run bundle:dev && yarn run git:hash",
"git:hash": "git log -1 --format=\"%H %as\" > commit_hash",

Still this is suboptimal. I'll see what I can do in another PR.

Edit: Rebased to fix the merge conflict after #9497

Summary by CodeRabbit

  • Chores
    • Simplified the Docker image build by consolidating multiple copy steps into a single operation, reducing Dockerfile complexity and streamlining what gets packaged into the image.

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

@coderabbitai
Copy link

coderabbitai bot commented Dec 28, 2025

Walkthrough

Consolidated two COPY instructions in the Dockerfile into a single COPY that brings both the .git directory and the web source into /pgadmin4/; WORKDIR remains /pgadmin4/web and no other build steps changed. (41 words)

Changes

Cohort / File(s) Summary
Docker build consolidation
Dockerfile
Replaced two separate COPY instructions with a single COPY that copies the .git directory and web source into /pgadmin4/; removed prior explicit file exclusion/commented notes.

Sequence Diagram(s)

(omitted — change is a localized Dockerfile update without multi-component control-flow)

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

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 PR title 'Corrected erroneous comment in Dockerfile' is partially related to the changeset but omits the equally significant consolidation of two COPY layers, which represents a meaningful optimization alongside the comment correction.
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.

@Guiorgy Guiorgy changed the title Removed erroneous comment and merged 2 copy layers in Dockerfile Corrected erroneous comment and merged 2 copy layers in Dockerfile Dec 28, 2025
@Guiorgy
Copy link
Contributor Author

Guiorgy commented Dec 28, 2025

@coderabbitai What do you mean "remove previous explicit file exclusion patterns", it was just a comment, the real exclusions remain in the unaltered .dockerignore file.

@coderabbitai
Copy link

coderabbitai bot commented Dec 28, 2025

Tip

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

@Guiorgy, you're absolutely right, and I apologize for the misleading wording in my summary.

I incorrectly described it as "removing the previous explicit and commented exclusion of certain files," which made it sound like the exclusion mechanism itself was being removed.

What actually happened is much simpler:

  • You removed an outdated comment that incorrectly suggested files were being explicitly excluded during the COPY operation
  • You merged two COPY layers for efficiency
  • The actual file exclusions remain properly configured in the .dockerignore file, which was not altered

The summary should have simply stated that you removed an erroneous comment and consolidated two COPY instructions. Thank you for the clarification!

@Guiorgy Guiorgy changed the title Corrected erroneous comment and merged 2 copy layers in Dockerfile Corrected erroneous comment in Dockerfile Jan 1, 2026
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: 0

🧹 Nitpick comments (1)
Dockerfile (1)

34-35: Consider clarifying the comment for accuracy.

The comment states "copy the source into it" referring to /pgadmin4, but the COPY instruction actually copies the web directory to /pgadmin4/web (a subdirectory). While /pgadmin4 is created as a parent directory, the phrasing could be more precise.

🔎 Suggested refinement
-# Create the /pgadmin4 directory and copy the source into it
+# Copy the web source into /pgadmin4/web
 COPY web /pgadmin4/web
📜 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 7a4b89a and f061278.

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

39-39: LGTM! Efficient use of bind mount for build-time dependency.

Using --mount=type=bind for the .git directory is an excellent approach. It provides access to git metadata during the build (needed by the git:hash script in web/package.json) without bloating the final image with the entire .git directory. This consolidation improves both layer efficiency and the final image size.

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.

1 participant