Add --resolve-symlinks flag to build and push artifact commands#5724
Add --resolve-symlinks flag to build and push artifact commands#5724rohansood10 wants to merge 2 commits intofluxcd:mainfrom
Conversation
When building OCI artifacts from directories containing symlinks (e.g., symlink trees created by Nix), the symlinked files are silently skipped because the underlying archive logic only handles regular files and directories. This results in empty or incomplete artifacts. This change adds a --resolve-symlinks flag to both 'flux build artifact' and 'flux push artifact' commands. When set, symlinks are resolved by copying their target contents into a temporary directory before building the artifact. This approach: - Preserves backward compatibility (default behavior unchanged) - Works with symlinks pointing outside the source directory - Handles symlinked files and directories - Cleans up the temporary directory after the build completes Fixes fluxcd#5055 Signed-off-by: rohansood10 <rohansood10@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds a --resolve-symlinks flag to the flux build artifact and flux push artifact commands to support building OCI artifacts from directories containing symlinks, specifically addressing issue #5055 where Nix-generated symlink trees were not being properly packaged.
Changes:
- Added
--resolve-symlinksflag to both build and push artifact commands - Implemented
resolveSymlinksfunction that creates a temporary directory with symlink targets copied as regular files - Added helper function
copyFileto copy individual files - Added unit tests for the
resolveSymlinksfunction
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/flux/build_artifact.go | Added resolveSymlinks flag, implemented symlink resolution logic with resolveSymlinks and copyFile helper functions |
| cmd/flux/push_artifact.go | Added resolveSymlinks flag and integrated symlink resolution into the push command flow |
| cmd/flux/build_artifact_test.go | Added Test_resolveSymlinks unit test covering symlinked files, regular files, and symlinked directories |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Create a symlinked subdirectory | ||
| subDir := filepath.Join(srcDir, "subdir") | ||
| g.Expect(os.MkdirAll(subDir, 0o755)).To(Succeed()) | ||
| g.Expect(os.WriteFile(filepath.Join(subDir, "nested.yaml"), []byte("nested"), 0o644)).To(Succeed()) | ||
| g.Expect(os.Symlink(subDir, filepath.Join(symlinkDir, "linkeddir"))).To(Succeed()) |
There was a problem hiding this comment.
The test creates a symlinked subdirectory with a nested file, but doesn't verify that the nested file was properly copied to the resolved directory. Add an assertion to verify that the file inside the symlinked directory is accessible and contains the expected content.
cmd/flux/build_artifact.go
Outdated
| if realInfo.IsDir() { | ||
| return os.MkdirAll(dstPath, realInfo.Mode()) |
There was a problem hiding this comment.
When a symlink points to a directory, the code creates the directory but doesn't copy its contents. The filepath.Walk function doesn't descend into symlinked directories (it uses Lstat, not Stat), so files inside symlinked directories are not copied to the temporary directory. This needs to be fixed by recursively walking the resolved directory path when a symlink points to a directory.
| func copyFile(src, dst string) error { | ||
| in, err := os.Open(src) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer in.Close() | ||
|
|
||
| if err := os.MkdirAll(filepath.Dir(dst), 0o755); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| out, err := os.Create(dst) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer out.Close() | ||
|
|
||
| if _, err := io.Copy(out, in); err != nil { | ||
| return err | ||
| } | ||
| return out.Close() | ||
| } |
There was a problem hiding this comment.
The copyFile function doesn't preserve the original file's permissions. When copying files from the symlink targets, the permissions are reset to the default (0666 modified by umask) instead of preserving the source file's mode. Consider using os.Chmod to preserve the source file's permissions after copying.
Address review feedback: - Fix filepath.Walk not descending into symlinked directories by using os.ReadDir-based recursive traversal that follows symlinks - Preserve source file permissions in copyFile using os.OpenFile instead of os.Create - Add test assertions to verify nested files in symlinked directories are properly copied Signed-off-by: rohansood10 <rohansood10@users.noreply.github.com>
|
Addressed the review feedback in the latest push:
|
When building OCI artifacts from directories containing symlinks (e.g., symlink trees created by Nix), symlinked files are silently skipped. This adds a --resolve-symlinks flag to both build artifact and push artifact commands that resolves symlinks by copying their targets into a temp directory before archiving. Implements the approach suggested by stefanprodan in the issue discussion. Includes unit tests. Fixes #5055