internal/exec/go_getter_utils.go (2)

160-168: Eliminate potential duplication with existing logic.

This RegisterCustomGetters seems to mirror the RegisterCustomDetectors pattern. 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.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

internal/exec/go_getter_utils.go (1)

202-231: Enhance symlink handling with additional safeguards.

While the implementation is functional, consider these improvements:

1. Pass AtmosConfig to removeSymlinks instead of using an empty config
2. Add protection against circular symlinks
3. 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
     })
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

internal/exec/copy_glob.go (1)

168-255: Consider simplifying copyToTargetWithPatterns.

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.LogTrace

tests/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 .tf and .md files are included while .tfvars files are excluded for this source.

Would you like help creating a dedicated test that specifically checks the behavior of these globs?

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

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
 	})
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

internal/exec/copy_glob.go (1)

47-92: ⚠️ Potential issue

Fix LogTrace calls to match the updated function signature.

Based on the pipeline failures and past review comments, the LogTrace calls need to be updated.

Apply this fix to all LogTrace calls 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))

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
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

internal/exec/copy_glob.go (1)

55-55: ⚠️ Potential issue

Fix 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

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:

1. Pattern matching logic (lines 196-211)
2. File copying with exclusions (lines 212-247)
3. Fallback copying logic (lines 248-253)

This would improve readability and make the code easier to test.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

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 skipFunc to 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 := &copyWorker{
+        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))
+}

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

internal/exec/copy_glob.go (3)

95-138: Consider using filepath.WalkDir for better performance.

While the current implementation is correct, using filepath.WalkDir instead 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:

1. Target path preparation (lines 181-183)
2. Inclusion pattern handling (lines 196-247)
3. 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

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

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

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

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 "//".

tests/fixtures/scenarios/vendor/vendor.yaml (1)

55-65: Review of "test globs" component configuration

This 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.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

internal/exec/go_getter_utils.go (1)

98-104: ⚠️ Potential issue

Update URL handling to use parsedURL.Path

Based on the previous feedback, we should check for "//" in parsedURL.Path instead of d.source to avoid matching the scheme separator.

Apply this diff to fix the URL handling:

-if !strings.Contains(d.source, "//") {
+if !strings.Contains(parsedURL.Path, "//") {

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
     })
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

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 "//".

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)
+		}
+	}()

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

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.