Fix Vendoring Issues with Globs and Symlinks #984
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request enhance the GitHub repository fetching mechanism within the Atmos vendoring process. A new field, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/go_getter_utils.go (2)
160-168: Eliminate potential duplication with existing logic.This
RegisterCustomGettersseems to mirror theRegisterCustomDetectorspattern. Consider centralizing shared logic or converting both into a unified registration function to reduce redundancy.
207-220: Consider configurable symlink removal.Automatically stripping symlinks is helpful, but give the user control to skip or confirm. This might be vital for some use cases or compliance requirements.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/go_getter_utils.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/go_getter_utils.go (3)
102-109: Check for potential unintended path expansion.Appending
//.whenever the path ends with.gitor has only three parts might impact cases where the user wants to download a subdirectory. Confirm that this logic is aligned with user expectations.
188-199: Good approach for overriding the default getters.Overriding
'git'with a custom implementation is a practical solution. Be sure to keep an eye on future expansions (likes3,gcs) if needed.
222-236: Validate error handling on symlink removal.If removing a symlink fails due to permission errors or other reasons, you might want more detailed logging or fallback behavior. Evaluate whether to continue, warn, or halt.
|
Please add a test for this type of vendoring. It can be in the vendoring scenario we already have. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/go_getter_utils.go (1)
202-231: Enhance symlink handling with additional safeguards.While the implementation is functional, consider these improvements:
- Pass AtmosConfig to removeSymlinks instead of using an empty config
- Add protection against circular symlinks
- Log removed symlink details for audit purposes
Here's a suggested enhancement:
type CustomGitGetter struct { getter.GitGetter + AtmosConfig schema.AtmosConfiguration } func (c *CustomGitGetter) Get(dst string, url *url.URL) error { if err := c.GitGetter.Get(dst, url); err != nil { return err } - return removeSymlinks(dst) + return removeSymlinks(dst, c.AtmosConfig) } -func removeSymlinks(root string) error { +func removeSymlinks(root string, config schema.AtmosConfiguration) error { + visited := make(map[string]bool) return filepath.Walk(root, func(path string, info os.FileInfo, err error) error { if err != nil { return err } if info.Mode()&os.ModeSymlink != 0 { + // Check for circular symlinks + realPath, err := filepath.EvalSymlinks(path) + if err != nil { + return fmt.Errorf("failed to evaluate symlink %s: %w", path, err) + } + if visited[realPath] { + return fmt.Errorf("circular symlink detected: %s", path) + } + visited[realPath] = true + + // Log symlink details before removal + target, _ := os.Readlink(path) - u.LogWarning(schema.AtmosConfiguration{}, fmt.Sprintf("Removing symlink: %s", path)) + u.LogWarning(config, fmt.Sprintf("Removing symlink: %s -> %s", path, target)) return os.Remove(path) } return nil }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/go_getter_utils.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/go_getter_utils.go (4)
73-73: LGTM! Field addition enhances URL handling capabilities.The addition of the
sourcefield to store the original URL is a clean enhancement.
151-157: LGTM! Clean function signature update.The modification properly propagates the source URL to the detector while maintaining the existing detector registration logic.
171-194: LGTM! Well-documented changes with secure defaults.The changes are well-documented and the getters map provides a comprehensive set of protocols. Good security practice to have S3 and GCS getters disabled by default.
103-109: LGTM! Clean implementation of top-level repository detection.The logic correctly handles bare repository URLs by appending "//." when appropriate. The debug logging provides good visibility into the decision-making process.
Let's verify the URL transformation behavior:
✅ Verification successful
URL handling logic is correctly implemented and follows go-getter conventions! 💪
The code properly handles repository URLs by:
- Detecting bare repository URLs (without subdirectories)
- Adding "//." suffix only when needed (for .git URLs or standard org/repo formats)
- Maintaining compatibility with go-getter's URL format requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Test various GitHub URL patterns to ensure correct handling echo "Testing URL transformations..." # Create a test file cat << 'EOF' > test.go package main import ( "fmt" "os" u "github.com/cloudposse/atmos/pkg/utils" ) func main() { detector := &CustomGitHubDetector{source: os.Args[1]} result, ok, err := detector.Detect(os.Args[1], "") fmt.Printf("Input: %s\nOutput: %s\nOK: %v\nErr: %v\n", os.Args[1], result, ok, err) } EOF # Test cases urls=( "github.com/org/repo.git" "github.com/org/repo" "github.com/org/repo/subdir" ) for url in "${urls[@]}"; do echo "Testing URL: $url" go run test.go "$url" echo "---" doneLength of output: 6203
Script:
#!/bin/bash # Find the URL handling implementation echo "Searching for URL handling implementation..." ast-grep --pattern 'type $_ struct { source string $$$ }' echo -e "\nSearching for Detect method implementation..." ast-grep --pattern 'func ($_ $_) Detect($_, $_) ($_, $_, error) { $$$ }' echo -e "\nSearching for URL parsing logic..." rg -A 5 'strings\.Contains.*"//"' --type goLength of output: 998
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/exec/copy_glob.go (1)
168-255: Consider simplifyingcopyToTargetWithPatterns.The function logic is clear, but the length and multiple branches make it complex. Splitting out smaller helper functions (e.g., for handling the included vs excluded paths) might improve maintainability and ease of unit testing.
🧰 Tools
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 184-184:
too many arguments in call to u.LogTrace🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 184-184:
too many arguments in call to u.LogTracetests/fixtures/scenarios/vendor/vendor.yaml (1)
54-64: Add a test case to validate the newly introduced globs.The expanded vendor configuration is helpful for controlling which files are included or excluded. Remember to add or update an integration test verifying that only
.tfand.mdfiles are included while.tfvarsfiles are excluded for this source.Would you like help creating a dedicated test that specifically checks the behavior of these globs?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/exec/copy_glob.go(1 hunks)internal/exec/vendor_model.go(1 hunks)tests/fixtures/scenarios/vendor/vendor.yaml(1 hunks)tests/test-cases/demo-stacks.yaml(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Build (macos-latest, macos)
internal/exec/copy_glob.go
[failure] 55-55:
too many arguments in call to u.LogTrace
[failure] 64-64:
too many arguments in call to u.LogTrace
[failure] 67-67:
too many arguments in call to u.LogTrace
[failure] 78-78:
too many arguments in call to u.LogTrace
[failure] 81-81:
too many arguments in call to u.LogTrace
[failure] 87-87:
too many arguments in call to u.LogTrace
[failure] 119-119:
too many arguments in call to u.LogTrace
[failure] 157-157:
too many arguments in call to u.LogTrace
[failure] 162-162:
too many arguments in call to u.LogTrace
[failure] 184-184:
too many arguments in call to u.LogTrace
🪛 GitHub Check: Build (ubuntu-latest, linux)
internal/exec/copy_glob.go
[failure] 55-55:
too many arguments in call to u.LogTrace
[failure] 64-64:
too many arguments in call to u.LogTrace
[failure] 67-67:
too many arguments in call to u.LogTrace
[failure] 78-78:
too many arguments in call to u.LogTrace
[failure] 81-81:
too many arguments in call to u.LogTrace
[failure] 87-87:
too many arguments in call to u.LogTrace
[failure] 119-119:
too many arguments in call to u.LogTrace
[failure] 157-157:
too many arguments in call to u.LogTrace
[failure] 162-162:
too many arguments in call to u.LogTrace
[failure] 184-184:
too many arguments in call to u.LogTrace
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/vendor_model.go (1)
310-310: Switching tocopyToTargetWithPatternslooks good.This change aligns with the new glob-handling logic. The final code is consistent with the PR’s objective to support inclusion and exclusion patterns.
tests/test-cases/demo-stacks.yaml (1)
196-204: Enhanced Vendoring File Checks AddedThe new file existence entries in the "atmos_vendor_pull" test bolster the validation of the vendoring process. They ensure that the expected files from both the demo-library and demo-stacks components are present after cloning and post-symlink removal. This aligns well with the PR objectives addressing glob functionality and symlink handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exec/go_getter_utils.go (2)
202-215: Consider enhancing error context.While the implementation is solid, consider wrapping the errors with additional context to aid in debugging:
func (c *CustomGitGetter) Get(dst string, url *url.URL) error { // Normal clone if err := c.GitGetter.Get(dst, url); err != nil { - return err + return fmt.Errorf("git clone failed for %s: %w", url, err) } // Remove symlinks - return removeSymlinks(dst) + if err := removeSymlinks(dst); err != nil { + return fmt.Errorf("failed to remove symlinks in %s: %w", dst, err) + } + return nil }
219-231: Consider adding path validation and detailed logging.The symlink removal implementation could be enhanced with additional safety checks and logging:
func removeSymlinks(root string) error { + // Validate root path + if _, err := os.Stat(root); err != nil { + return fmt.Errorf("invalid root path %s: %w", root, err) + } + return filepath.Walk(root, func(path string, info os.FileInfo, err error) error { if err != nil { return err } if info.Mode()&os.ModeSymlink != 0 { - u.LogWarning(schema.AtmosConfiguration{}, fmt.Sprintf("Removing symlink: %s", path)) + target, err := os.Readlink(path) + if err != nil { + return fmt.Errorf("failed to read symlink %s: %w", path, err) + } + u.LogWarning(schema.AtmosConfiguration{}, fmt.Sprintf("Removing symlink: %s -> %s", path, target)) // It's a symlink, remove it - return os.Remove(path) + if err := os.Remove(path); err != nil { + return fmt.Errorf("failed to remove symlink %s: %w", path, err) + } } return nil }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/go_getter_utils.go(4 hunks)internal/exec/vendor_model.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/vendor_model.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/go_getter_utils.go (2)
71-74: LGTM! Smart handling of top-level repository cloning.The addition of the
sourcefield and the logic to append "//." for top-level repositories is a solid improvement. This ensures proper cloning behavior when no subdirectory is specified.Also applies to: 103-109
151-155: LGTM! Well-documented parameter addition.The addition of the
sourceparameter and the detailed comments explaining its purpose make the code's intent clear and maintainable.Also applies to: 171-175
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/exec/copy_glob.go (1)
47-92:⚠️ Potential issueFix LogTrace calls to match the updated function signature.
Based on the pipeline failures and past review comments, the
LogTracecalls need to be updated.Apply this fix to all
LogTracecalls in the function:- u.LogTrace(atmosConfig, fmt.Sprintf("Error computing relative path for %q: %v", srcPath, err)) + u.LogTrace(fmt.Sprintf("Error computing relative path for %q: %v", srcPath, err))
🧹 Nitpick comments (4)
internal/exec/copy_glob.go (3)
16-45: Consider handling errors from deferred Close() calls.While the implementation is solid, it's good practice to handle errors from
Close()operations, especially for the destination file where data corruption could occur.Here's how you could improve the error handling:
func copyFile(atmosConfig schema.AtmosConfiguration, src, dst string) error { sourceFile, err := os.Open(src) if err != nil { return fmt.Errorf("opening source file %q: %w", src, err) } - defer sourceFile.Close() + defer func() { + if cerr := sourceFile.Close(); cerr != nil && err == nil { + err = fmt.Errorf("closing source file %q: %w", src, cerr) + } + }() if err := os.MkdirAll(filepath.Dir(dst), os.ModePerm); err != nil { return fmt.Errorf("creating destination directory for %q: %w", dst, err) } destinationFile, err := os.Create(dst) if err != nil { return fmt.Errorf("creating destination file %q: %w", dst, err) } - defer destinationFile.Close() + defer func() { + if cerr := destinationFile.Close(); cerr != nil && err == nil { + err = fmt.Errorf("closing destination file %q: %w", dst, cerr) + } + }()
139-166: Consider extracting pattern suffixes as constants.The pattern matching logic is well-implemented, but the magic strings could be made more maintainable.
Consider this improvement:
+const ( + singleLevelGlob = "/*" + recursiveGlob = "/**" +) + func getMatchesForPattern(atmosConfig schema.AtmosConfiguration, sourceDir, pattern string) ([]string, error) { fullPattern := filepath.Join(sourceDir, pattern) matches, err := u.GetGlobMatches(fullPattern) if err != nil { return nil, fmt.Errorf("error getting glob matches for %q: %w", fullPattern, err) } if len(matches) == 0 { - if strings.HasSuffix(pattern, "/*") { - recursivePattern := strings.TrimSuffix(pattern, "/*") + "/**" + if strings.HasSuffix(pattern, singleLevelGlob) { + recursivePattern := strings.TrimSuffix(pattern, singleLevelGlob) + recursiveGlob
168-255: Consider pre-allocating the map with an estimated size.The implementation is solid, but the map allocation could be optimized when processing inclusion patterns.
Consider this optimization:
if len(s.IncludedPaths) > 0 { - filesToCopy := make(map[string]struct{}) + // Pre-allocate map with estimated size based on number of patterns + filesToCopy := make(map[string]struct{}, len(s.IncludedPaths)*10)This pre-allocation helps avoid map resizing during population, potentially improving performance when dealing with many files.
internal/exec/go_getter_utils.go (1)
202-233: Consider adding progress logging for symlink removal.The symlink removal implementation is solid, but for better observability, consider adding debug logging for the number of symlinks removed.
Here's a suggested enhancement:
func removeSymlinks(root string) error { + removedCount := 0 err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { if err != nil { return err } if info.Mode()&os.ModeSymlink != 0 { - return os.Remove(path) + if err := os.Remove(path); err != nil { + return err + } + removedCount++ return nil } return nil }) + if removedCount > 0 { + u.LogDebug(fmt.Sprintf("Removed %d symlinks from %s", removedCount, root)) + } return err }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/copy_glob.go(1 hunks)internal/exec/go_getter_utils.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (5)
internal/exec/copy_glob.go (2)
3-13: LGTM! Well-organized imports with clear purpose.The imports are appropriately chosen for file operations and include an optimized copy library with a clear explanatory comment.
94-137: LGTM! Excellent symlink handling implementation.The function properly implements the PR objective of removing symlinks by explicitly skipping them during the copy process. The error handling is comprehensive and the recursive logic is sound.
internal/exec/go_getter_utils.go (3)
71-74: LGTM! Clean struct extension.The addition of the
sourcefield toCustomGitHubDetectoris a well-structured change that maintains backward compatibility.
171-194: LGTM! Well-documented client configuration.The client configuration is clear and the comments explaining the source parameter's importance are helpful.
219-233: Consider adding test coverage for symlink removal.The symlink removal functionality would benefit from unit tests to verify its behavior.
Would you like me to generate unit tests for the
removeSymlinksfunction to ensure it handles various scenarios correctly?
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/exec/copy_glob.go (1)
55-55:⚠️ Potential issueFix logger usage to use charmbracelet logger correctly.
The current logger usage is causing compilation errors. Based on past review comments, we should use the charmbracelet logger directly.
Apply this fix:
- l.LogDebug("Error computing relative path", 'srcPath', srcPath, 'err', err) + l.LogDebug("Error computing relative path", "srcPath", srcPath, "err", err)🧰 Tools
🪛 golangci-lint (1.62.2)
55-55: illegal rune literal
(typecheck)
🪛 GitHub Check: Build (macos-latest, macos)
[failure] 55-55:
undefined: l
[failure] 55-55:
more than one character in rune literal🪛 GitHub Check: Build (ubuntu-latest, linux)
[failure] 55-55:
undefined: l
[failure] 55-55:
more than one character in rune literal🪛 GitHub Check: autofix
[failure] 55-55:
illegal rune literal
[failure] 55-55:
illegal rune literal🪛 GitHub Actions: autofix.ci
[error] 55-55: illegal rune literal
🧹 Nitpick comments (3)
internal/exec/copy_glob.go (3)
15-45: Consider enhancing error wrapping for better debugging.The error handling is thorough, but we could make it more consistent with Go 1.13+ error wrapping conventions.
Consider this improvement:
- return fmt.Errorf("opening source file %q: %w", src, err) + return fmt.Errorf("failed to open source file %q: %w", src, err) - return fmt.Errorf("creating destination directory for %q: %w", dst, err) + return fmt.Errorf("failed to create destination directory for %q: %w", dst, err) - return fmt.Errorf("creating destination file %q: %w", dst, err) + return fmt.Errorf("failed to create destination file %q: %w", dst, err) - return fmt.Errorf("copying content from %q to %q: %w", src, dst, err) + return fmt.Errorf("failed to copy content from %q to %q: %w", src, dst, err) - return fmt.Errorf("getting file info for %q: %w", src, err) + return fmt.Errorf("failed to get file info for %q: %w", src, err) - return fmt.Errorf("setting permissions on %q: %w", dst, err) + return fmt.Errorf("failed to set permissions on %q: %w", dst, err)
139-166: Consider extracting pattern suffix as a constant.The pattern suffix "/*" is used multiple times and could be made more maintainable.
Consider this improvement:
+const ( + singleLevelGlobSuffix = "/*" + recursiveGlobSuffix = "/**" +) func getMatchesForPattern(atmosConfig schema.AtmosConfiguration, sourceDir, pattern string) ([]string, error) { fullPattern := filepath.Join(sourceDir, pattern) matches, err := u.GetGlobMatches(fullPattern) if err != nil { return nil, fmt.Errorf("error getting glob matches for %q: %w", fullPattern, err) } if len(matches) == 0 { - if strings.HasSuffix(pattern, "/*") { - recursivePattern := strings.TrimSuffix(pattern, "/*") + "/**" + if strings.HasSuffix(pattern, singleLevelGlobSuffix) { + recursivePattern := strings.TrimSuffix(pattern, singleLevelGlobSuffix) + recursiveGlobSuffix
168-255: Consider breaking down the pattern matching logic into smaller functions.The function is well-implemented but could be more maintainable if split into smaller, focused functions.
Consider extracting these parts into separate functions:
- Pattern matching logic (lines 196-211)
- File copying with exclusions (lines 212-247)
- Fallback copying logic (lines 248-253)
This would improve readability and make the code easier to test.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/copy_glob.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
internal/exec/copy_glob.go
55-55: illegal rune literal
(typecheck)
🪛 GitHub Check: Build (macos-latest, macos)
internal/exec/copy_glob.go
[failure] 55-55:
undefined: l
[failure] 55-55:
more than one character in rune literal
🪛 GitHub Check: Build (ubuntu-latest, linux)
internal/exec/copy_glob.go
[failure] 55-55:
undefined: l
[failure] 55-55:
more than one character in rune literal
🪛 GitHub Check: autofix
internal/exec/copy_glob.go
[failure] 55-55:
illegal rune literal
[failure] 55-55:
illegal rune literal
🪛 GitHub Actions: autofix.ci
internal/exec/copy_glob.go
[error] 55-55: illegal rune literal
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/copy_glob.go (2)
1-13: Well-structured imports and package organization!Good choice using the optimized
otiai10/copylibrary for unfiltered copying operations.
94-137: Solid implementation of recursive directory copying!Good job handling symlinks and maintaining proper error propagation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
internal/exec/copy_glob.go (5)
16-46: Consider handling errors from deferred Close() calls.While the implementation is solid, it's good practice to handle errors from
Close()operations, especially for write operations.func copyFile(src, dst string) error { sourceFile, err := os.Open(src) if err != nil { return fmt.Errorf("opening source file %q: %w", src, err) } - defer sourceFile.Close() + defer func() { + if cerr := sourceFile.Close(); cerr != nil { + l.Debug("Error closing source file", "path", src, "error", cerr) + } + }() if err := os.MkdirAll(filepath.Dir(dst), os.ModePerm); err != nil { return fmt.Errorf("creating destination directory for %q: %w", dst, err) } destinationFile, err := os.Create(dst) if err != nil { return fmt.Errorf("creating destination file %q: %w", dst, err) } - defer destinationFile.Close() + defer func() { + if cerr := destinationFile.Close(); cerr != nil { + l.Debug("Error closing destination file", "path", dst, "error", cerr) + } + }()
48-93: Consider pre-compiling patterns for better performance.For better performance when processing multiple files, consider pre-compiling the patterns once and reusing them.
+// compiledPattern holds a pre-compiled pattern and its original string +type compiledPattern struct { + pattern string + regexp *regexp.Regexp +} + +// compilePatterns pre-compiles glob patterns to regular expressions +func compilePatterns(patterns []string) ([]*compiledPattern, error) { + compiled := make([]*compiledPattern, 0, len(patterns)) + for _, p := range patterns { + re, err := u.GlobToRegexp(p) + if err != nil { + return nil, fmt.Errorf("compiling pattern %q: %w", p, err) + } + compiled = append(compiled, &compiledPattern{pattern: p, regexp: re}) + } + return compiled, nil +}Then modify the
skipFuncto accept pre-compiled patterns instead of string slices.
95-138: Consider parallel processing for large directories.For better performance when copying large directories, consider implementing parallel processing using goroutines and a worker pool.
+// copyWorker represents a worker that copies files in parallel +type copyWorker struct { + jobs chan copyJob + results chan error + wg sync.WaitGroup +} + +// copyJob represents a single copy operation +type copyJob struct { + src string + dst string +} + func copyDirRecursive(atmosConfig schema.AtmosConfiguration, srcDir, dstDir, baseDir string, excluded, included []string) error { + // Create a worker pool for parallel copying + numWorkers := runtime.GOMAXPROCS(0) + worker := ©Worker{ + jobs: make(chan copyJob, numWorkers), + results: make(chan error, numWorkers), + } + + // Start workers + for i := 0; i < numWorkers; i++ { + go worker.start() + }
140-167: Consider caching pattern matches for repeated operations.For scenarios where the same patterns are used multiple times, consider implementing a cache to store pattern matches.
+var ( + matchCache sync.Map + matchCacheTTL = 5 * time.Minute +) + +type cacheEntry struct { + matches []string + expires time.Time +} + func getMatchesForPattern(sourceDir, pattern string) ([]string, error) { + cacheKey := sourceDir + ":" + pattern + if entry, ok := matchCache.Load(cacheKey); ok { + if e := entry.(cacheEntry); time.Now().Before(e.expires) { + return e.matches, nil + } + matchCache.Delete(cacheKey) + }
169-256: Consider adding progress reporting for large copy operations.For better user experience during large copy operations, consider implementing progress reporting.
+type progressTracker struct { + totalFiles int64 + processedFiles int64 + mu sync.Mutex +} + +func (p *progressTracker) update() { + p.mu.Lock() + p.processedFiles++ + progress := float64(p.processedFiles) / float64(p.totalFiles) * 100 + p.mu.Unlock() + l.Info("Copy progress", "completed", fmt.Sprintf("%.1f%%", progress)) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/copy_glob.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/exec/copy_glob.go (3)
95-138: Consider using filepath.WalkDir for better performance.While the current implementation is correct, using
filepath.WalkDirinstead of manual recursion could improve performance, especially for deep directory structures.Here's how you could refactor it:
func copyDirRecursive(srcDir, dstDir, baseDir string, excluded, included []string) error { - entries, err := os.ReadDir(srcDir) - if err != nil { - return fmt.Errorf("reading directory %q: %w", srcDir, err) - } - for _, entry := range entries { - srcPath := filepath.Join(srcDir, entry.Name()) + return filepath.WalkDir(srcDir, func(srcPath string, d fs.DirEntry, err error) error { + if err != nil { + return err + } dstPath := filepath.Join(dstDir, filepath.Rel(srcDir, srcPath)) - info, err := entry.Info() + info, err := d.Info() // ... rest of the function remains the same - } - return nil + }) }
140-167: Consider caching the pattern suffix check.The pattern suffix check could be moved outside the empty matches check to avoid unnecessary string operations.
Here's a minor optimization:
func getMatchesForPattern(sourceDir, pattern string) ([]string, error) { + isStarPattern := strings.HasSuffix(pattern, "/*") fullPattern := filepath.Join(sourceDir, pattern) matches, err := u.GetGlobMatches(fullPattern) if err != nil { return nil, fmt.Errorf("error getting glob matches for %q: %w", fullPattern, err) } - if len(matches) == 0 { - if strings.HasSuffix(pattern, "/*") { + if len(matches) == 0 && isStarPattern {
169-255: Consider splitting the function for better maintainability.The function is handling multiple responsibilities and could be split into smaller, more focused functions to improve readability and maintainability.
Consider extracting these parts into separate functions:
- Target path preparation (lines 181-183)
- Inclusion pattern handling (lines 196-247)
- Exclusion pattern handling (lines 249-253)
Example extraction:
+func prepareTargetPath(targetPath string, sourceIsLocalFile bool, uri string) string { + if sourceIsLocalFile && filepath.Ext(targetPath) == "" { + return filepath.Join(targetPath, SanitizeFileName(uri)) + } + return targetPath +} +func handleInclusionPatterns(sourceDir, targetPath string, s *schema.AtmosVendorSource) error { // Move lines 196-247 here +} func copyToTargetWithPatterns(...) error { - if sourceIsLocalFile && filepath.Ext(targetPath) == "" { - targetPath = filepath.Join(targetPath, SanitizeFileName(uri)) - } + targetPath = prepareTargetPath(targetPath, sourceIsLocalFile, uri) // ... rest of the function
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/copy_glob.go(1 hunks)internal/exec/vendor_model.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/vendor_model.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (3)
internal/exec/copy_glob.go (3)
1-14: LGTM! Well-organized imports with clear purpose.The imports are cleanly structured and the comment explaining the use of the optimized copy library is helpful.
16-46: LGTM! Robust file copying implementation.The function demonstrates excellent error handling, proper resource management with defers, and preserves file permissions. The detailed error messages will aid in debugging.
48-93: Update logging to use charmbracelet logger directly.As per previous review feedback, replace the logging calls with direct charmbracelet logger usage.
Apply this pattern throughout the function:
-l.Debug("Error computing relative path", "srcPath", srcPath, "error", err) +l.Debug("Error computing relative path", "srcPath", srcPath, "err", err)
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/go_getter_utils.go (1)
212-214: Improve function comment clarity.The comment suggests the function is for the destination directory, but the function name doesn't reflect this context.
Apply this diff to improve the comment:
-// removeSymlinks walks the directory and removes any symlinks -// it encounters. +// removeSymlinks walks the directory and removes any symlinks it encounters
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exec/go_getter_utils.go(4 hunks)tests/test-cases/demo-stacks.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test-cases/demo-stacks.yaml
🧰 Additional context used
📓 Learnings (1)
internal/exec/go_getter_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#984
File: internal/exec/go_getter_utils.go:103-109
Timestamp: 2025-02-03T15:51:48.035Z
Learning: When checking for subdirectories in GitHub URLs, use `parsedURL.Path` to check for "//" instead of the entire URL, as the scheme portion (e.g., "https://") will always contain "//".
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (4)
internal/exec/go_getter_utils.go (4)
67-69: LGTM! Well-structured field addition.The
sourcefield addition toCustomGitHubDetectoris clean and follows Go conventions.
146-170: LGTM! Clear documentation and implementation.The changes to
RegisterCustomDetectorsare well-documented and correctly implement the requirement to pass the original source URL.
197-211: LGTM! Clean implementation of custom git getter.The
CustomGitGetterimplementation correctly extends the baseGitGetterand adds symlink removal functionality with proper error handling.
98-104:⚠️ Potential issueFix URL path handling to avoid matching scheme separator.
The current implementation checks for "//" in the entire URL (
d.source), which will incorrectly match the scheme separator (e.g., "https://"). Instead, check for "//" specifically in the path portion.Apply this diff to fix the URL path handling:
-if !strings.Contains(d.source, "//") { +if !strings.Contains(parsedURL.Path, "//") {Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/fixtures/scenarios/vendor/vendor.yaml (1)
55-65: Review of "test globs" component configurationThis new component cleanly demonstrates advanced glob patterns. The included paths pattern using brace expansion (
**/{demo-library,demo-stacks}/**/*.{tf,md}) is well-formed to target both file extensions in the intended directories, and the excluded paths appropriately filter out unwanted files. For maintainability, consider adding a brief inline comment explaining the rationale behind these patterns for future maintainers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/fixtures/scenarios/vendor/vendor.yaml(1 hunks)tests/test-cases/demo-stacks.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test-cases/demo-stacks.yaml
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (1)
tests/fixtures/scenarios/vendor/vendor.yaml (1)
66-74: Review of "test globs without double stars upfront" component configurationThis component effectively tests the scenario where the glob pattern does not start with double stars, which directly addresses osterman’s previous request. The included paths pattern (
/weather/*.md) should validate that the glob matching correctly handles leading slashes. It would be good to confirm that the leading slash is interpreted as intended (relative vs. absolute) by your glob processing logic.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/exec/go_getter_utils.go (1)
98-104:⚠️ Potential issueUpdate URL handling to use
parsedURL.PathBased on the previous feedback, we should check for "//" in
parsedURL.Pathinstead ofd.sourceto avoid matching the scheme separator.Apply this diff to fix the URL handling:
-if !strings.Contains(d.source, "//") { +if !strings.Contains(parsedURL.Path, "//") {
🧹 Nitpick comments (1)
internal/exec/go_getter_utils.go (1)
219-235: Enhance error handling in symlink removal.While the implementation is solid, we should provide more context in error messages to help with debugging.
Consider this enhanced implementation:
func removeSymlinks(root string) error { return filepath.Walk(root, func(path string, info os.FileInfo, err error) error { if err != nil { - return err + return fmt.Errorf("failed to access path %s: %w", path, err) } if info.Mode()&os.ModeSymlink != 0 { - return os.Remove(path) + if err := os.Remove(path); err != nil { + return fmt.Errorf("failed to remove symlink at %s: %w", path, err) + } } return nil }) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/go_getter_utils.go(4 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/go_getter_utils.go (1)
Learnt from: osterman
PR: cloudposse/atmos#984
File: internal/exec/go_getter_utils.go:103-109
Timestamp: 2025-02-03T15:51:48.035Z
Learning: When checking for subdirectories in GitHub URLs, use `parsedURL.Path` to check for "//" instead of the entire URL, as the scheme portion (e.g., "https://") will always contain "//".
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: [mock-macos] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] tests/fixtures/scenarios/complete
- GitHub Check: [mock-windows] examples/demo-vendoring
- GitHub Check: [mock-windows] examples/demo-context
- GitHub Check: [mock-windows] examples/demo-component-versions
- GitHub Check: Acceptance Tests (macos-latest, macos)
- GitHub Check: Acceptance Tests (windows-latest, windows)
- GitHub Check: Acceptance Tests (ubuntu-latest, linux)
- GitHub Check: [localstack] demo-localstack
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/go_getter_utils.go (2)
139-144: LGTM! Good addition of clone depth control.Setting a default clone depth of 1 is a performance optimization that prevents downloading the entire git history when not needed.
173-177: LGTM! Clear and helpful comment.The comment effectively explains why we need to pass the original source URL to the detector.
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/exec/copy_glob.go (2)
1-14: Add package documentation.Consider adding a package-level documentation comment to describe the purpose and functionality of this package.
Add this documentation above the package declaration:
+// Package exec provides functionality for copying files and directories with pattern matching support. +// It handles file permissions, symlinks, and supports inclusion/exclusion patterns for selective copying. package exec
17-46: Optimize error handling in copyFile.The function is well-implemented, but we can improve the error handling by checking the result of
Close()operations.Consider this optimization:
sourceFile, err := os.Open(src) if err != nil { return fmt.Errorf("opening source file %q: %w", src, err) } - defer sourceFile.Close() + defer func() { + if cerr := sourceFile.Close(); cerr != nil && err == nil { + err = fmt.Errorf("closing source file %q: %w", src, cerr) + } + }()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/copy_glob.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/exec/copy_glob.go (1)
Learnt from: Listener430
PR: cloudposse/atmos#984
File: internal/exec/copy_glob.go:0-0
Timestamp: 2025-02-06T13:38:07.216Z
Learning: The `u.LogTrace` function in the `cloudposse/atmos` repository accepts `atmosConfig` as its first parameter, followed by the message string.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
internal/exec/copy_glob.go (2)
56-56: Well-implemented logging pattern!Excellent use of structured logging with charmbracelet/log. The debug messages are informative and will be helpful for troubleshooting.
Also applies to: 68-69, 72-73, 96-98, 100-101
48-111: Consider revising error handling strategy in shouldSkipEntry.The function silently continues on pattern matching errors, which might hide issues with malformed patterns.
Consider either:
- Propagating pattern matching errors to allow the caller to decide how to handle them
- Adding more detailed logging about why patterns were skipped
Let's verify if there are any malformed patterns in the codebase:
| if err := os.MkdirAll(targetPath, os.ModePerm); err != nil { | ||
| return fmt.Errorf("creating target directory %q: %w", targetPath, err) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Handle potential race condition in directory creation.
The MkdirAll followed by operations on the directory could be subject to a race condition if multiple processes are involved.
Consider adding file locking or checking for concurrent access issues. Let's check if there are any existing synchronization mechanisms in the codebase:
🏁 Script executed:
#!/bin/bash
# Search for file locking mechanisms
rg -A 5 'flock|Lock|Mutex|Synchronize'Length of output: 28258
Handle race condition in directory creation in internal/exec/copy_glob.go
After reviewing the codebase, it’s clear that although other parts of the repository (for example, in pkg/config/cache.go) leverage file locks (using gofrs/flock) or internal mutexes to coordinate concurrent file operations, the directory creation in copy_glob.go is done solely via os.MkdirAll without any explicit locking mechanism. This could lead to potential race conditions in environments where multiple processes or goroutines attempt to create or operate on the same directory concurrently.
• The os.MkdirAll call is inherently idempotent, but subsequent operations on the newly created directory might be affected by concurrent access.
• Unlike other modules that incorporate file locking, no synchronization guards the MkdirAll in copy_glob.go.
Consider introducing a locking mechanism (such as using a file-based lock or an internal mutex) around directory creation to ensure safe concurrent access.
what
Done:
why
Fix vednoring issues
references
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements