Add Key() to LayeredMap and Snapshotter #337
Conversation
This will return a string representaiton of the current filesystem to be used with caching. Whenever a file is explictly added (via ADD or COPY), it will be stored in "added" in the LayeredMap. The file will map to a hash created by CacheHasher (which doesn't take into account mtime, since that will be different with every build, making the cache useless) Key() will returns a sha of the added files which will be used in determining the overall cache key for a command.
| // Key returns a hash for added files | ||
| func (l *LayeredMap) Key() (string, error) { | ||
| c := bytes.NewBuffer([]byte{}) | ||
| enc := json.NewEncoder(c) |
There was a problem hiding this comment.
It's not clear to me why we're json encoding anything. Is the idea to get a stable hash for a map[string]string?
I'm not sure if the sorting of a map when encoding it will be stable...
There was a problem hiding this comment.
Yah, the idea was to get a stable hash, I thought it would work because of this from the docs on json.Marshal:, which is used by the encoder
The map keys are sorted and used as JSON object keys
My plan was to get a hash for the config file in the same way.
| @@ -85,11 +102,18 @@ func (l *LayeredMap) MaybeAddWhiteout(s string) (bool, error) { | |||
|
|
|||
| // Add will add the specified file s to the layered map. | |||
| func (l *LayeredMap) Add(s string) error { | |||
There was a problem hiding this comment.
is the name of the file or contents?
There was a problem hiding this comment.
s is the name of the file
There was a problem hiding this comment.
sorry, is that what you were asking?
There was a problem hiding this comment.
yes. i was just wondering if we shd be hashing the file contents as well.
There was a problem hiding this comment.
we call the hasher function on line 106:
newV, err := l.hasher(s)
and that function will read the file contents, modtime, and some other things as well
There was a problem hiding this comment.
hmm.. After reading the PR description again, i think the cacheHasher does it.
pkg/snapshot/layered_map_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("error getting key for map 2: %v", err) | ||
| } | ||
| if test.equal && k1 != k2 { |
There was a problem hiding this comment.
You could avoid t.Fatalf by adding this 1 check instead of checking them separately.
if test.equal != (k1 == k2) {
t.Errof("keys expected to be same: %t. Got %t", test.equal, k1==k2)
}
That way, the other test cases still keep running
There was a problem hiding this comment.
thanks, just changed it!
I think t.Fatalf will only kill the current test in this case, since it's running within t.Run
| // SHA256 returns the shasum of the contents of r | ||
| func SHA256(r io.Reader) (string, error) { | ||
| hasher := sha256.New() | ||
| _, err := io.Copy(hasher, r) |
There was a problem hiding this comment.
return error if its not nil.
the next line might throw an exception.
pkg/snapshot/layered_map.go
Outdated
| whiteouts []map[string]string | ||
| added []map[string]string | ||
| hasher func(string) (string, error) | ||
| cacheHasher func(string) (string, error) |
There was a problem hiding this comment.
What is the difference between hasher and cacheHasher?
There was a problem hiding this comment.
nvm, read the description again. Can you add comments here describing hasher and cacheHasher?
There was a problem hiding this comment.
cacheHasher doesn't include file mtime in the hash, since that would be different for every build and we need a stable key for caching
There was a problem hiding this comment.
added the comment, there's also more detailed descriptions in util.go where the hash functions live
This will return a string representaiton of the current filesystem to be
used with caching.
Whenever a file is explictly added (via ADD or COPY), it will be stored
in "added" in the LayeredMap. The file will map to a hash created by
CacheHasher (which doesn't take into account mtime, since that will be
different with every build, making the cache useless)
Key() will returns a sha of the added files which will be used in
determining the overall cache key for a command.