Fix issue #704 #973
Fix issue #704 #973
Conversation
* add util.GetFSFromLayers * GetFSFromImage delegates to GetFSFromLayers * add FSOpts and FSConfig for GetFSFromLayers * add tests for GetFSFromLayers * add gomock for test support * add mock_v1 for layers
Update caching run and copy commands to use the new GetFSFromLayers method and include the whiteout option so that whiteout files are extracted and included in extractedFiles
for some reason hack/boilerplate.sh failed after using `addlicense` so I copy/pasted the license header from another file
| } | ||
| } | ||
|
|
||
| func ExtractFunc(extractFunc ExtractFunction) FSOpt { |
There was a problem hiding this comment.
I couldn't wrap my head around ExtractFunc taking an ExtractFunction type with parameter name extractFunc. I don't have a recommendation either since my go skills are rather poor so just fyi.
There was a problem hiding this comment.
WDYT about func ExtractFuncOpt(f ExtractFunction) FSOpt
There was a problem hiding this comment.
ah I now get it was well, might just be me not getting it the other night, never seen this pattern before. I think keeping as is is fine
|
|
||
| func IncludeWhiteout() FSOpt { | ||
| return func(opts *FSConfig) { | ||
| opts.includeWhiteout = true |
There was a problem hiding this comment.
this is always true. Do you see any future this being configured via command line? i am wondering if we could make the code simpler and avoid this config.
There was a problem hiding this comment.
I don't see it ever being configured via command line since a single process will call FSFromLayers with different options.
This is always true because the zero value is false. So by not including this option it is set to false.
I might be misunderstanding
There was a problem hiding this comment.
I guess I could see an argument for passing the value of IncludeWhitespace as an arg to the function so it could be switched from true to false but that seems like something that could be changed later if the need arisea
There was a problem hiding this comment.
actually i meant the opposite. Lets remove opts.includeWhiteout totally
There was a problem hiding this comment.
I don't understand. When GetFSFromImage calls GetFSFromLayers includeWhiteout should be false. When CachingCopyCommand or CachingRunCommand calls GetFSFromLayers includeWhiteout should be true.
so includeWhiteout has to be configurable in some regards. Are you objecting to having it be configurable or this specific configuration pattern?
Line 105 in 9609335
Line 210 in 9609335
Line 162 in 9609335
There was a problem hiding this comment.
Ah, i think it did not occur to me includeWhiteout was false due to the opts .. FSOpt array.
That explains.
return GetFSFromLayers(root, layers, ExtractFunc(extract))
}
func GetFSFromLayers(root string, layers []v1.Layer, opts ...FSOpt) ([]string, error) {
Fixes #704
Description
Previously whiteout files were not extracted from cached layers and not included in the list of extracted files. This meant that if a file was deleted in a cached layer it would "reappear" in the final image as it was missing the whiteout file.
This change adds a new method
GetFSFromLayersto whichGetFSFromImagenow delegates.GetFSFromLayersaccepts a series of options, one of whichIncludeWhiteoutwill cause whiteout files to be extracted and returned in the list of extracted files.Additionally, this change updates
CachingRunCommandandCachingCopyCommandto useGetFSFromLayersand pass the option to include the whiteout files. This results in the whiteout files being extracted from cached layers and the deleted file properly removed from the final image.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.