Fix the copier:get operation to properly gather symlink information#6759
Conversation
dosubot
Bot
added
the
size:M
nalind
left a comment
nalind
left a comment
There was a problem hiding this comment.
A question about a change to a WalkDir callback, and I don't think that the additions to the tests are examining everything that we want them to.
| isOriginallySymlink := false | ||
| expectedLinkTarget := "" | ||
| for _, origHdr := range testArchive.headers { | ||
| if origHdr.Name == hdr.Name && origHdr.Typeflag == tar.TypeSymlink { |
There was a problem hiding this comment.
This looks like it would skip over items whose paths are changed for some reason, either because they were explicitly renamed due to a renames setting, or because they were copied from one subdirectory to another.
There are a number of the test cases where the expected set of items includes the contents of a directory that was specified as the source for the copy operation, where the contents of the directory, but not the directory itself, are expected to be copied to the destination.
It might be simpler to add a new field to the test cases which maps from the members of the items slice to the corresponding expected Linkname field values in the headers with matching Name values. This loop body could then populate a new map using the Name and Linkname values for every header it sees with type TypeSymlink, and then those maps could be compared when expectedContents and actualContents are compared.
There was a problem hiding this comment.
Good shout. I've split the commits in two now:
First commit will contain the expansion of the copier_test.go to validate the symlink content as of main branch
Second commit applies the fix and fixes the test for easier reviewing of what is changing
BenjaminSchubert
force-pushed
the
bschubert/fix-symlink-copy
branch
2 times, most recently
from
981b85a to
0c18548
Compare
nalind
left a comment
nalind
left a comment
There was a problem hiding this comment.
LGTM with a nit, and the two commits might as well be squashed since they touch some of the same test cases.
| if info.Mode()&os.ModeType == os.ModeSymlink { | ||
| target, err := os.Readlink(path) | ||
| if err != nil { | ||
| return "", fmt.Errorf("readlink(%q): %w", path, err) |
There was a problem hiding this comment.
Nit: the standard library wraps the error with the path in a PathError result, and this would add the path again.
There was a problem hiding this comment.
Sure thing, done. This is a pattern that's present multiple time in the same file, should I clean that up in a separate PR?
Previously, the copier was always dereferencing symlinks, even if `noDerefSymlinks` was set. This passed unnoticed as the test was not validating the symlink value. This augments the test to validate the symlink target, and ensures the copier sets the information completely. Also expand the tests in the copier to validate the symlinks destinations too Previously, this would only be validating the source, but never the destination Signed-off-by: Benjamin Schubert <bschubert15@bloomberg.net>
BenjaminSchubert
force-pushed
the
bschubert/fix-symlink-copy
branch
from
0c18548 to
88d6873
Compare
dosubot
Bot
added
size:S
|
@nalind This is now squashed and ready to go from my side. Thanks for the reviews! |
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
LGTM, I think CI failures are flakes. (I don't have permission to rerun them.)
|
PTAL @containers/buildah-maintainers |
4 participants
What type of PR is this?
What this PR does / why we need it:
Previously, the copier was always dereferencing symlinks, even if
noDerefSymlinkswas set. This passed unnoticed as the test was not validating the symlink value.This augments the test to validate the symlink target, and ensures the copier sets the information completely.
How to verify it
Remove the fix, run the copier tests, notice that there is no more symlinks in them
Which issue(s) this PR fixes:
None (I can create one if needed, this was discovered while working on #6737 )
Special notes for your reviewer:
Does this PR introduce a user-facing change?
I am unsure here, I don't think so?