fix: fix COPY command error due to missing but ignored files #2812
Conversation
QuanZhang-William
changed the title
COPY command error due to missing but ignored files
| } | ||
|
|
||
| err = cmd.ExecuteCommand(cfg, dockerfile.NewBuildArgs([]string{})) | ||
| if err != nil { |
There was a problem hiding this comment.
Could you help me to understand what's the error as described in #1598 could have happened without this fix?
There was a problem hiding this comment.
Hi @JeromeJu. IIUC #1598 describes a racing condition between 2 process. Say process A 1) finds all the files to be copied; and 2) copies the files one by one. If process B kicks in and deletes some file between 1) and 2), it will throw an error. However, if the files to be copied are supposed to be ignored, it should not fail since the files will not be copied anyways. That's all swapping the 2 function calls solve the problem.
Regarding the tests, I didn't find a way to mock the race condition in unit test, so I just added a test case with ignored files. Please let me know if you have better way to test it 😁
There was a problem hiding this comment.
One way of doing this is via "monkey patching" (done elsewhere in the code) where we can mock out RemainingFiles or os.Lstat to delete an excluded file after/before processing and use this mock for a repro test. I have done this here - https://gist.github.com/aaron-prindle/7f7fd1f43dc5c3fe4e9b14f69ecdd9e7
I don't think it is necessary for the code in this case though, I have manually tested using the above test @ HEAD vs @this-PR and this does fix the issue (as expected). Looks good!
Fixes GoogleContainerTools#1598. This commit puts `context.ExcludesFile` before `os.Lstat` to avoid the `COPY` command error due to missing but ignored files.
QuanZhang-William
force-pushed
the
exclude-file-before-copy
branch
from
880d694
to
91562d0
Compare
| @@ -461,6 +462,60 @@ func TestCopyCommand_ExecuteCommand_Extended(t *testing.T) { | |||
| } | |||
| }) | |||
|
|
|||
| t.Run("copy dir to another dir - with ignored files", func(t *testing.T) { | |||
There was a problem hiding this comment.
Thanks for adding this!
Description
Fixes #1598.
This commit puts
context.ExcludesFilebeforeos.Lstatto avoid theCOPYcommand error due to missing but ignored files.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Reviewer Notes
Release Notes
Describe any changes here so maintainer can include it in the release notes, or delete this block.