-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix caching to respect .dockerignore #854
Fix caching to respect .dockerignore #854
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I'll try to add unit tests and integration tests when I have the time |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@WanzenBug Thanks for your contribution. Would be up for adding tests? |
I've added unit tests to check that the |
44ad2c6
to
526ec29
Compare
@tejal29 I've added some unit tests |
Rebased |
268b948
to
9e0f466
Compare
docker ignore integration test failed
|
Previously kaniko would compute the cache key for any copy command by computing the combined hash of all files in a directory, even if they were listed as ignored. With this change, the cache key creation was updated to be aware of ignored files. Related issues: * GoogleContainerTools#594
Add unit tests that check the behaviour of CompositeCache on adding filesytem resources. It checks that * 2 identical directory trees produces the same hash * an extra file produces a different hash * an extra directry produces a different hash * an extra file that is excluded does not alter the hash * an extra directory that is excluded does not alter the hash
a6d8c88
to
782e491
Compare
Rebasing this again. Let us know if you need help finishing up this PR |
pkg/executor/composite_cache.go
Outdated
if err := filepath.Walk(p, func(path string, fi os.FileInfo, err error) error { | ||
if err != nil { | ||
return err | ||
} | ||
exclude := util.ExcludeFile(path, context) | ||
if fi.IsDir() && exclude { |
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.
This is still needed right so we can skip walking whitelisted directories?
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.
Actually this was onr if the bugs:
If the directory was ignored, but one the contained files was included via later rules in the ignore file, this would skip over this file completely.
The change now would still not include the directory in the cache key, but would include the contained file.
Description
Previously kaniko would compute the cache key for any copy command by computing
the combined hash of all files in a directory, even if they were listed
as ignored.
With this change, the cache key creation was updated to be aware of ignored
files.
Related issues:
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