Skip to content

Conversation

@Guiorgy
Copy link
Contributor

@Guiorgy Guiorgy commented Nov 20, 2025

In the Dockerfile in the app-builder stage the .git directory is copied:

COPY .git .git

And then later in the same stage it is supposedly removed:

# Build the JS vendor code in the app-builder, and then remove the vendor source.
RUN export CPPFLAGS="-DPNG_ARM_NEON_OPT=0" && \
    npm install -g corepack && \
    ... # skipped for brevity, attention to below VVV
           /pgadmin4/.git

I say supposedly, because the .git directory is copied into the filesystem root directory (/), whereas, the rest of the project files are inside the pgadmin4 directory, and the later command expects the .git directory to be inside it. Clearly a mistake, it was supposed to be copied into /pgadmin4/.git.

Using docker build --target app-builder --tag "pgadmin4:app-builder-stage" . to only build and export this stage, and then dive pgadmin4:app-builder-stage to explore the layers, I confirmed that:

Screanshot of the output of dive

With that being said, is the .git directory even needed during the build process? If not, it would be better to completely omit the copy to save on having to copy more than 366 MB (the size of .git as of writing) into the Docker image each time, which takes some time.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed application deployment process to ensure proper file handling during build and installation, improving reliability and consistency across environments.

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

@coderabbitai
Copy link

coderabbitai bot commented Nov 20, 2025

Walkthrough

Modified the Dockerfile's COPY instruction to place the .git directory at /pgadmin4/.git instead of the root .git path, ensuring alignment with subsequent build steps that reference the pgadmin4-specific path.

Changes

Cohort / File(s) Change Summary
Docker build configuration
Dockerfile
Updated COPY instruction to target /pgadmin4/.git instead of .git at root, aligning with downstream cleanup and build step references

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

  • Verify the path change aligns with all downstream references to /pgadmin4/.git in build and cleanup steps
  • Confirm the .git directory is correctly positioned for any subsequent build or runtime operations

Pre-merge checks and finishing touches

✅ 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 accurately and concisely describes the main change: fixing the .git copy destination in the Dockerfile to target /pgadmin4/.git instead of the root.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent 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 482549b and f4bff51.

📒 Files selected for processing (1)
  • Dockerfile (1 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). (10)
  • 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, 18)
  • 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 (13)
  • GitHub Check: run-feature-tests-pg (18)
  • GitHub Check: run-feature-tests-pg (16)
  • GitHub Check: run-feature-tests-pg (15)
🔇 Additional comments (1)
Dockerfile (1)

34-34: Fix is correct, but consider if .git is needed at all.

The correction aligns the COPY destination with the removal command at line 65, fixing the original misplaced directory issue. However, the PR author raised a valid concern: is the .git directory actually used during the build process (lines 46–65), or is it copied only to be deleted?

If .git is not needed during the JS build steps, consider excluding it entirely via a .dockerignore file to avoid the ~366 MB copy overhead on every build. This would be more efficient than copying and then removing it.

Can you confirm whether the yarn/npm build steps (lines 49–55) require git metadata? If not, using .dockerignore to exclude .git would be a better optimization.


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

@Guiorgy
Copy link
Contributor Author

Guiorgy commented Nov 20, 2025

The only possible benefit to copying the .git directory in the build process, that I can see, is to get and store the git revision/hash, but even that has much better ways of handling, such as passing it from the host as a build argument. do we really need to be copying the whole .git directory into the image? It wasn't even being correctly copied until now anyway :D

PS. Gotta love flaky tests :P

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