Skip to content

Conversation

@MatthijsKok
Copy link
Contributor

Summary

Docker best practices recommend to use COPY when the additional functionality of ADD is not used.

See:

Test Plan

Docs only change

@zanieb
Copy link
Member

zanieb commented Dec 2, 2025

I'm fine with using COPY but I'd rather not change the ordering of the working directory as well. I find it less clear to COPY . .

@zanieb zanieb added the documentation Improvements or additions to documentation label Dec 2, 2025
Comment on lines 165 to 166
# Change the working directory to the `app` directory
WORKDIR /app
Copy link
Member

Choose a reason for hiding this comment

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

Can you revert this change too please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first example was the only one that did not start with the WORKDIR, so I thought I'd align them.
But whatever you prefer :) I can see how this is simpler in a way.

Reverted.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I didn't notice that, I presume we need it earlier in the other ones because we're relying on it for the cache mounts. I'm not quite sure how I feel about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The cache mounts all target the absolute path /root/.cache/uv, so they are not impacted by WORKDIR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I always start my dockerfiles with a WORKDIR, so I moved it without too much thought haha.

It's not that important :) The current situation is perfectly fine

Copy link
Member

Choose a reason for hiding this comment

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

hm?

# Install dependencies
RUN --mount=type=cache,target=/root/.cache/uv \
--mount=type=bind,source=uv.lock,target=uv.lock \
--mount=type=bind,source=pyproject.toml,target=pyproject.toml \
uv sync --locked --no-install-project

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, "bind mounts" :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh. ok that makes a lot more sense!

I think the dilemma can be summed up as

  • "WORKDIR statement before first time it's used" vs
  • "WORKDIR statement at the start of the file" (potentially 'far away')

whatever you think is best <3

@zanieb zanieb enabled auto-merge (squash) December 3, 2025 15:03

# Sync the project into a new environment, asserting the lockfile is up to date
WORKDIR /app
RUN uv sync --locked
Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought: with only 1 statement making use of the WORKDIR you could even

RUN uv --directory=app sync --locked

@zanieb zanieb merged commit 539b736 into astral-sh:main Dec 3, 2025
101 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants