Add --resolve-symlinks flag to build and push artifact commands

[rohansood10]

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

[Copilot]

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-symlinks flag to both build and push artifact commands
• Implemented resolveSymlinks function that creates a temporary directory with symlink targets copied as regular files
• Added helper function copyFile to copy individual files
• Added unit tests for the resolveSymlinks function

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.

[rohansood10]

Addressed the review feedback in the latest push:

1. Symlinked directory traversal: Replaced filepath.Walk with a recursive copyDir function using os.ReadDir that properly follows symlinks into directories. filepath.Walk uses Lstat internally and doesn't descend into symlinked directories — the new approach resolves symlinks and recursively copies directory contents.

2. File permission preservation: Updated copyFile to use os.OpenFile with the source file's mode instead of os.Create (which defaults to 0666), ensuring permissions are preserved.

3. Test coverage: Added assertions to verify that files inside symlinked directories are properly copied and are regular files (not symlinks).

[rohansood10]

Friendly ping — any chance to review this when you get a moment? Happy to make changes if needed.

[stefanprodan]

copyDir follows symlinks recursively with no cycle detection. If a symlink creates a circular reference (e.g., link -> ../parent), filepath.EvalSymlinks resolves the intermediate symlink but the resolved real directory can contain further symlinks pointing back up, causing infinite recursion and a stack overflow.

This is particularly relevant for the Nix use case, where symlink trees can be complex. A visited map tracking resolved real paths (by device+inode or absolute path) should be threaded through the recursion:

func copyDir(srcDir, dstDir string, visited map[string]bool) error {
    real, _ := filepath.EvalSymlinks(srcDir)
    abs, _ := filepath.Abs(real)
    if visited[abs] {
        return nil // break the cycle
    }
    visited[abs] = true
    // ... rest of the function
}

[stefanprodan]

When resolveSymlinks receives a single file path, it returns a temp directory containing the copied file. The caller then sets path = resolved, so oci.Build now receives a directory instead of a file. While the archive content ends up functionally the same (both cases produce just the base filename in the tarball), the semantics change — and it could diverge if oci.Build behavior is ever updated.

A simpler and more correct approach for the single-file case would be to return the path to the resolved file itself (within the temp dir), not the temp dir:

// Instead of: return tmpDir, nil
return dst, nil

[rohansood10]

Hi @stefanprodan @hiddeco,

Friendly ping on this PR! It's been 8 days since the last update.

This PR adds the --resolve-symlinks flag to the flux build artifact and flux push artifact commands, which enables proper handling of symbolic links when creating OCI artifacts. This is particularly useful for:

• Kustomization overlays that reference base configurations via symlinks
• Shared configuration patterns where multiple apps reference common configs
• GitOps workflows that use symlinks to avoid duplication

The implementation:

• Adds the flag to both build and push commands
• Properly resolves symlinks during artifact creation
• Includes comprehensive tests
• Updates documentation

All CI checks are passing. Would appreciate a review when you get a chance. Happy to address any feedback!

Thanks!

[stefanprodan]

@rohansood10 did you read my last review??

[rohansood10]

Hi @stefanprodan, yes I did! I addressed all your review feedback in the commits from 8 days ago:

1. ✅ Fixed symlinked directory traversal by replacing filepath.Walk with a recursive copyDir function
2. ✅ Fixed file permission preservation using os.OpenFile with source file mode
3. ✅ Added test coverage for files inside symlinked directories

The implementation now properly handles all the edge cases you mentioned. All tests are passing. Is there any additional feedback I should address?

[stefanprodan]

You really don't see the last review I made around symlinks recursively with no cycle detection and the single file path?

[rohansood10]

@stefanprodan Apologies for missing those — I see the two review comments now. Both are addressed in the latest push:

1. Symlink cycle detection (comment): Added a visited map (keyed by absolute resolved path) threaded through copyDir recursion. If a directory has already been visited, we return early to break the cycle. Added Test_resolveSymlinks_cycle that creates a dir/cycle -> dir symlink and verifies no infinite recursion or deeply nested paths.

2. Single file path semantics (comment): resolveSymlinks now returns the resolved file path (not the temp directory) for the single-file case, preserving file semantics for callers. The function signature was updated to return a separate cleanup path so the temp directory is still properly cleaned up. Added Test_resolveSymlinks_singleFile to verify the returned path is a file.

[stefanprodan]

@rohansood10 please squash all commits into one, signoff and force push. You also need to rebase your fork.

[rohansood10]

@stefanprodan done - squashed into a single commit, signed off, and rebased onto main. Ready for another look when you get a chance!

[stefanprodan]

you need to sync your fork with upstream and rebase! take a look at what you pushed in this PR...

[rohansood10]

@stefanprodan done - synced the fork and rebased with a single clean commit. Sorry about the messy state earlier.