feat: allow symlink #571
feat: allow symlink #571
Conversation
Codecov Report
@@ Coverage Diff @@
## main #571 +/- ##
==========================================
- Coverage 50.72% 50.34% -0.38%
==========================================
Files 43 43
Lines 3316 3345 +29
==========================================
+ Hits 1682 1684 +2
- Misses 1424 1446 +22
- Partials 210 215 +5
Continue to review full report at Codecov.
|
|
I think the suggestion in #571 (comment) is still useful, we could add a symlink in the repo and exercise this new path in an e2e test |
pkg/commands/options/filestuff.go
Outdated
| if fi.IsDir() { | ||
| if path != paths && !fo.Recursive { | ||
| return filepath.SkipDir | ||
| } | ||
| if watcher != nil { | ||
| watcher.Add(path) | ||
| watcher.Add(symlink) |
There was a problem hiding this comment.
Let's just remove any watcher interaction, we're going to delete it all soon anyway in #585
There was a problem hiding this comment.
OFC, thank you 🙋🏻♂️
pkg/commands/options/filestuff.go
Outdated
| } else { | ||
| if symlink != "" { | ||
| files <- symlink | ||
| } |
There was a problem hiding this comment.
| } else { | |
| if symlink != "" { | |
| files <- symlink | |
| } | |
| } else if symlink != "" { | |
| files <- symlink | |
| } |
There was a problem hiding this comment.
thank you 🙋🏻♂️
| @@ -142,3 +155,10 @@ func EnumerateFiles(fo *FilenameOptions) chan string { | |||
| }() | |||
| return files | |||
| } | |||
|
|
|||
| func ResolveSymlink(path string, fi os.FileInfo) (symlink string, err error) { | |||
There was a problem hiding this comment.
Can this be replaced with https://pkg.go.dev/path/filepath#EvalSymlinks ?
There was a problem hiding this comment.
it didn't work when I switched to EvalSymlink, I don't know why 🤷🏻♂️
There was a problem hiding this comment.
I'd like to understand better why that's the case. This is getting fairly complicated, and I'd rather depend on stdlib methods as much as possible. If we still need to write significant code to handle symlinks, I'm a bit more inclined not to support symlinks for now.
test/symlink.yaml
Outdated
| @@ -0,0 +1 @@ | |||
| test/test.yaml | |||
There was a problem hiding this comment.
Can you add a test that checks that this symlink is followed if you ko apply -f test/symlink.yaml ?
There was a problem hiding this comment.
I tried something to test this but couldn't make it work but anyways I wanted to share the code with you to see what I missed or did wrong 🙋🏻♂️ maybe you can help me to fix this 🙏
There was a problem hiding this comment.
thanks a ton, @Dentrax, he helped me to fix this. ❤️
|
NIT: One small concern here is that we do not support the case in which symlink points itself: A workaround: https://github.com/fkautz/gitbom-go/pull/7/files (It would be overengineered to implement that in this project I think) |
|
kindly ping 🙋🏻♂️ |
|
This Pull Request is stale because it has been open for 90 days with |
Signed-off-by: Batuhan Apaydın <[email protected]>
Signed-off-by: Batuhan Apaydın [email protected]
Fixes #460
Is that what you thought? @imjasonh