-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add --parents opt-in flag for ADD/COPY (labs) #3001
Conversation
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.
Thanks for this! This looks like the right direction 🎉
I've left a few code comments inline, but I think the main thing to work on next will be a test suite, there's some edge cases we need to explore properly, like how this should interact with remote files, tar files, heredocs, etc, etc.
I'm particularly curious how this interacts with this from the dockerfile reference (trailing slash behavior is... hard):
If is any other kind of file, it is copied individually along with its metadata. In this case, if ends with a trailing slash /, it will be considered a directory and the contents of will be written at /base().
For example, cp
produces this error:
cp: with --parents, the destination must be a directory
Try 'cp --help' for more information.
So maybe we should explicitly fail like cp
does if the destination isn't a directory? Although, the behavior is already a little strange in buildkit.
Anyways, I think these issues should fall out as we build up a test suite. Nice work so far!
fyi @aaronlehmann |
Excellent 🎉 Thanks. |
How would you like |
URLs shouldn't be the determining factor here I don't think - the complexity comes with For a |
Looks like this needs a rebase |
Just came back from vacation yesterday, rebase, some fixes and test suite coming shortly. |
I think you may have rebased with an outdated local state (I see some unrelated commits made their way in); let me know if you need help with rebasing 👍 |
Yep, thanks. |
👋 @DYefimov just wanted to follow up and check to see you weren't blocked on anything? |
@jedevc output
|
Looks like the test suite is running the right way, that's how I run individual tests. I also just tried a test using The issue looks like a file that the test attempts to access doesn't exist at that path - could you maybe push what you have, I'm happy to take a look. |
Thanks. |
If you're getting stuck on the "mess you made with git", and need help / instructions how to resolve, let us know; happy to help |
Gave a try to new git tools and it didn't go as expected... Funny enough, I'm using git oneflow ideology for the most part - it is "based on rebasing", and now we got here :)
Of cause, my local state has no merge op in it. |
I tend to not worry too much about the state of the git history while I'm working in "draft mode" - at least while the code's final shape is unclear, it's quite frustrating to try and have a neat history. Once it's all done, I either:
The steps you've outlined look good to me, when rebasing you just to only |
ef278e3
to
f92391b
Compare
Thanks, followed your instructions and squashed/amended it. Apart from documentation, there are several issues left with
|
Have left a couple comments, when you're happy with the state of this, I think it's good to come out of draft 🎉
|
@DYefimov all good? Just wanted to checkup to see if there's anything else or other clarification you need? |
f92391b
to
b103572
Compare
Great! Integrated suggested changes, fixed some linting, and rebased just in case. Coming out of draft. What about the documentation? Guess it should be placed right after this one: https://github.com/moby/buildkit/blob/master/frontend/dockerfile/docs/reference.md#copy---link Regarding this:
Didn't exactly follow you on this one. Is it really that complicated of a change? Please, check the placeholder I left here: |
I noticed that there are some merge conflicts; I'd love for this to get merged soonish, so if you need someone to resolve them, I am willing to try and help :) |
2beb32a
to
ad1cf47
Compare
Looks like there's a compile issue in a test;
|
ad1cf47
to
6970249
Compare
Co-Authored-By: Dmitrii Efimov <5221640+DYefimov@users.noreply.github.com> Co-Authored-By: Justin Chadwell <me@jedevc.com> Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
6970249
to
dce0519
Compare
Not quite sure why I didn't merge when I approved this a couple weeks ago - but let's get this in 🎉 |
@jedevc Can this be used on a released Buildkit version already (when setting the right |
@levrik this should work with current versions; see docker/roadmap#107 (comment) for an example |
@thaJeztah Awesome. Thanks! |
@levrik you're welcome! And if you find any issues with the feature, please open tickets 👍. I don't know if there's a timeline already for including it in the "stable" syntax, but getting feedback helps those decisions (knowing that everything works as intended) |
@thaJeztah I just tried the feature and I have to say to everyone who has been involved: Thank you so much! Now it's just a |
This feature is working great for us! It will dramatically improve our ability to rely on layer caching in our Node monorepo. I hope this ends up in the stable syntax soon! |
Happy to hear the feature is working well 🥳 |
Awesome work! Thanks @DYefimov! Are there any plans to add support for combining the COPY --from=build --parents /packages/**/dist . |
@jmurzy wdym it is noop?
Looks correct to me. |
Thanks, @tonistiigi. It was an oversight on my part, but it seems to be working. The behavior was confusing because it copied files relative to the destination directory, even when the For example, the following command copied the files into /usr/src/app/packages/usr/src/app/packages/**/dist. COPY --parents --from=build /usr/src/app/packages/**/dist /usr/src/app/packages I managed to fix it with the following command: COPY --parents --from=build /usr/src/app/packages/**/dist / Cheers 🍻 |
Found this feature, tested it, it does exactly what I wanted 👍 COPY --link --parents **/go.mod **/go.sum . Can't wait for this to make it to stable. |
I see that Dockerfile 1.8 and 1.9 have been released, but this isn't yet out of labs. What is the process for that? Anything I can do to help get this feature "graduated"? |
parents
option to dockerfile COPY command moby#35639.parents_test.sh
:.parents_test.sh
output:Signed-off-by: Dmitrii Efimov 5221640+DYefimov@users.noreply.github.com