-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Update Docker integration guide to prefer COPY over ADD for simple cases
#16883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Docker integration guide to prefer COPY over ADD for simple cases
#16883
Conversation
|
I'm fine with using |
docs/guides/integration/docker.md
Outdated
| # Change the working directory to the `app` directory | ||
| WORKDIR /app |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm?
uv/docs/guides/integration/docker.md
Lines 408 to 412 in e7af583
| # 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, "bind mounts" :)
There was a problem hiding this comment.
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
- "
WORKDIRstatement before first time it's used" vs - "
WORKDIRstatement at the start of the file" (potentially 'far away')
whatever you think is best <3
|
|
||
| # Sync the project into a new environment, asserting the lockfile is up to date | ||
| WORKDIR /app | ||
| RUN uv sync --locked |
There was a problem hiding this comment.
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
Summary
Docker best practices recommend to use
COPYwhen the additional functionality ofADDis not used.See:
Test Plan
Docs only change