Snapshot only specific files for COPY #319
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before GoogleContainerTools#289 was merged, when copying over directories for COPY kaniko would get a list of all files at the destination specified and add them to the list of files to be snapshotted. If the destination was root it would add all files. This worked because the snapshotter made sure the file had been changed before adding it to the layer. After GoogleContainerTools#289, we changed the logic to add all files snapshotted to a layer without checking if the files had been changed. This created the bug in got all the files at root and added them to the layer without checking if they had been changed. This change should fix this bug. Now, the CopyDir function returns a list of files it copied over and only those files are added to the list of files to be snapshotted. Should fix GoogleContainerTools#314
bobcatfish
approved these changes
Aug 27, 2018
pkg/util/fs_util.go
Outdated
| @@ -155,21 +155,24 @@ func ChildDirInWhitelist(path, directory string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| func unTar(r io.Reader, dest string) error { | |||
| // unTar returns a list of files extracted from the tar archive | |||
There was a problem hiding this comment.
would be good to including in this docstring what the params r and dest are for (https://blog.golang.org/godoc-documenting-go-code) e.g. unTar returns a list of files that have been extracted from the tar archive at r to the path at dest
There was a problem hiding this comment.
for sure, just added it!
|
/bark |
|
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
I'm sorry that that dog looks so suspicious |
I changed UnpackLocalTarArchive to return a list of files that were extracted, so that the list of snapshotted files for ADD is more accurate. Previously, we used to add all files in the extracted dir to be snapshotted, but this could result in preexisting files being snapshotted again.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before #289 was merged, when copying over directories for COPY kaniko
would get a list of all files at the destination specified and add them
to the list of files to be snapshotted. If the destination was root it
would add all files. This worked because the snapshotter made sure the
file had been changed before adding it to the layer.
After #289, we changed the logic to add all files snapshotted to a layer
without checking if the files had been changed. This created the bug in which kaniko
got all the files at root and added them to the layer without checking
if they had been changed.
This change should fix this bug. Now, the CopyDir function returns a
list of files it copied over and only those files are added to the list
of files to be snapshotted.
I also added this change to the
UnpackLocalTarArchivefunction in ADD, so that we only add specific files extracted from local tar archives to the list of snapshotted files.Should fix #314
Once #317 is implemented, this change will be tested by integration tests. Until then, I locally tested the Dockerfile in the bug and it seems to be working now.