Revert "Replace path.Join with filepath.Join (#856)" #887
Conversation
This reverts commit 2051592.
📝 WalkthroughWalkthroughThe pull request introduces a systematic replacement of Changes
Suggested labels
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 11
🧹 Nitpick comments (31)
internal/exec/workflow.go (1)
7-7: Potential Windows compatibility concern
Replacing "filepath" with "path" may break cross-platform usage if the code relies on OS-specific separators.cmd/docs.go (2)
7-7: Consider OS-specific path handling
Importing "path" might cause separator mismatches on Windows.
62-62: Cross-platform path caution
Using "path.Join" can lead to breakage if Windows file paths are involved.pkg/generate/terraform_generate_varfiles_test.go (1)
31-31: Potential Windows path issue in tests
"filePattern" may not match expected directory structures on Windows.internal/exec/terraform_generate_backend.go (1)
78-78: Double-check file path handling
Switching to "path.Join" could disrupt Windows support unless that's not a concern.internal/exec/oci_utils.go (2)
11-11: Evaluate cross-platform requirements for OCI workflows
Using "path" may ignore Windows path delimiters.
94-94: Layer extraction path caution
"filepath.Join" is recommended if layer paths must be valid on Windows.pkg/vender/component_vendor_test.go (4)
5-5: Consider using 'filepath' if targeting OS-specific paths.
You’re importing "path", which treats paths as URL-like. If these are filesystem-specific paths, replacing "path" with "filepath" might be safer for cross-platform compatibility (especially on Windows).
32-37: Potential filesystem compatibility concern.
All these path joins use "path.Join(...)" rather than "filepath.Join(...)", which may not respect Windows path separators. If you’re dealing with local paths, consider "filepath.Join".
48-64: Same OS compatibility concern.
Again, these lines rely on "path.Join(...)". Only use "path" if paths are URL-like or platform-agnostic.
67-85: Confirm file deletion logic with OS-specific semantics.
The "os.Remove(...)" and "os.RemoveAll(...)" calls will succeed, but if the broader codebase requires OS-consistent path handling, switching these joins from "path" to "filepath" is recommended.pkg/vender/vendor_config_test.go (5)
7-7: Check whether local or URL paths.
Importing "path" will treat paths as URL-like. For local paths, "filepath" is generally a more reliable cross-platform choice.
31-31: Platform-agnostic path usage.
"componentPath := path.Join(...)" might behave unexpectedly on Windows if the code expects local filesystem semantics.
87-87: Candidate for 'filepath.Join'.
Since this references a file on disk, "filepath.Join" is suggested for operating-system consistency.
105-105: Nonexistent file test logic.
Using "path.Join" here can pass the incorrect path on certain OSes. If the test is intended for all OSes, switching to "filepath.Join" is safer.
129-129: Path join inside local filesystem.
This can cause confusion if the code eventually runs on Windows with backslashes in the path.pkg/utils/file_utils.go (1)
72-72: Cross-platform join check.
At line 72, "path.Join" might cause issues on Windows. If these are pure filesystem paths, switch to "filepath.Join".internal/exec/terraform_utils.go (3)
8-8: Mixed path imports.
You’re importing both "path" and "filepath". Consolidate your approach: generally "filepath" is standard for local file manipulation.
91-91: Adjust for local file usage.
"backendFileName := path.Join(...)" is likely operating-system dependent. If these are truly local filesystem paths, prefer "filepath.Join".
124-124: Maintain cross-platform compatibility.
Using "path.Join" for the "providers_override.tf.json" path can lead to unexpected results on Windows.internal/exec/stack_utils.go (1)
167-167: Ensure consistency in path handling for Terraform components
Double-check whether the component paths are expected to be OS-agnostic or purely slash-based paths. If they are local file system paths, revert to “filepath.Join” for local compatibility.internal/exec/workflow_utils.go (2)
6-6: Use “filepath” instead of “path” for local paths
For multi-platform support, consider reverting to “filepath” imports for handling local directories.
150-150: Reevaluate “path.Join” for workflows
If these workflow definitions point to local file paths, revert to “filepath.Join” to avoid OS path separator issues.internal/exec/terraform_generate_backends.go (1)
294-294: Revert to “filepath.Join” for local file creation
Backends are typically stored in local directories. Using “filepath.Join” will ensure correctness on different operating systems.pkg/config/config.go (3)
136-136: Check if this path needs OS-aware handling.
Replacing filepath.Join with path.Join may introduce unexpected behavior on Windows.
165-165: Assess cross-platform implications.
While simpler, path.Join may fail if components contain backslashes on Windows.
192-192: Check uniform usage of path joining logic.
Having both path and filepath in the same file can be confusing.internal/exec/describe_affected_utils.go (2)
1185-1187: Assess if path.Join is valid for the 'component' argument.
On Windows, a backslash can creep in and cause mismatch.
1244-1244: Ensure path.Join is correct for module calls.
If these paths are always local file references, consider reverting to filepath.Join.internal/exec/stack_processor_utils.go (1)
375-375: Consider fallback to filepath for template lookups.
Local-based lookups often rely on OS-specific separators.pkg/utils/glob_utils.go (1)
39-39: Traverse results carefully on Windows.
Forward slash separators might break local references in certain edge cases.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
cmd/cmd_utils.go(2 hunks)cmd/docs.go(3 hunks)internal/exec/atlantis_generate_repo_config.go(2 hunks)internal/exec/aws_eks_update_kubeconfig.go(2 hunks)internal/exec/describe_affected_utils.go(4 hunks)internal/exec/helmfile.go(3 hunks)internal/exec/oci_utils.go(3 hunks)internal/exec/stack_processor_utils.go(3 hunks)internal/exec/stack_utils.go(2 hunks)internal/exec/terraform_generate_backend.go(2 hunks)internal/exec/terraform_generate_backends.go(3 hunks)internal/exec/terraform_generate_varfiles.go(2 hunks)internal/exec/terraform_utils.go(3 hunks)internal/exec/validate_component.go(3 hunks)internal/exec/validate_stacks.go(4 hunks)internal/exec/vendor_component_utils.go(3 hunks)internal/exec/vendor_utils.go(2 hunks)internal/exec/workflow.go(2 hunks)internal/exec/workflow_utils.go(3 hunks)pkg/config/config.go(8 hunks)pkg/generate/terraform_generate_varfiles_test.go(2 hunks)pkg/utils/file_utils.go(2 hunks)pkg/utils/glob_utils.go(2 hunks)pkg/vender/component_vendor_test.go(3 hunks)pkg/vender/vendor_config_test.go(6 hunks)
✅ Files skipped from review due to trivial changes (8)
- internal/exec/atlantis_generate_repo_config.go
- cmd/cmd_utils.go
- internal/exec/validate_component.go
- internal/exec/aws_eks_update_kubeconfig.go
- internal/exec/terraform_generate_varfiles.go
- internal/exec/vendor_utils.go
- internal/exec/helmfile.go
- internal/exec/validate_stacks.go
🔇 Additional comments (16)
internal/exec/workflow.go (1)
83-83: Recommend verifying path correctness on all platforms
If Windows usage is expected, consider reverting to "filepath.Join" to avoid separator issues.
pkg/generate/terraform_generate_varfiles_test.go (1)
5-5: Check for platform consistency
Importing "path" can lead to inconsistent file patterns on Windows.
internal/exec/terraform_generate_backend.go (1)
5-5: Platform-aware path usage
"filepath" is typically used for local file system paths. Confirm if "path" suffices.
✅ Verification successful
Let me verify how the path package is being used in this context.
This will help us understand:
- How the path package is being used in the context
- Whether filepath is already present
- The specific path operations being performed
Using path package is correct in this context
The path package is being used to join path components for generating a backend configuration file path that will be used in Terraform configuration. Since this appears to be for generating paths that will be used within Terraform's configuration (which uses forward slashes regardless of the platform), using the path package is actually the correct choice here instead of filepath. The path package always uses forward slashes which is exactly what we want for Terraform configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the usage of path package in the file
rg -A 5 -B 5 'path\.' internal/exec/terraform_generate_backend.go
# Check if filepath package is also imported or used
rg 'filepath' internal/exec/terraform_generate_backend.go
# Look at the complete context of path usage
ast-grep --pattern 'path.$_($_)'
Length of output: 485
internal/exec/stack_utils.go (2)
5-5: Consider reverting to “filepath” for file paths on local systems
“path” is typically for URL paths; for improved cross-platform compatibility involving file system paths, “filepath” is more appropriate.
169-169: Confirm Helmfile component path usage
Similar to the comments above, confirm that the Helmfile base path is truly OS-agnostic before finalizing the use of “path.Join.”
✅ Verification successful
Let me verify the usage of path joining in the codebase to ensure OS compatibility.
This will help us understand:
- Which path joining method is consistently used
- Whether the imports indicate awareness of OS-specific path handling
Let me check more thoroughly by examining the imports and overall path handling patterns in the file.
Path handling is correctly implemented
The code is using the standard path package (imported as "path") which is designed specifically for slash-separated paths and is OS-agnostic. The implementation consistently uses path.Join throughout the file for both Terraform and Helmfile component paths, making it safe for cross-platform usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for path joining usage patterns in the codebase
rg -A 2 "path\.Join|filepath\.Join" internal/exec/stack_utils.go
# Look for any imports of path/filepath packages
rg "^import.*\"path\".*|^import.*\"path/filepath\".*" internal/exec/stack_utils.go
Length of output: 429
Script:
#!/bin/bash
# Get the full content of the file to analyze imports and path handling
cat internal/exec/stack_utils.go
# Also check for any tests that might reveal path handling expectations
fd stack_utils_test.go --exec cat {}
Length of output: 8080
internal/exec/terraform_generate_backends.go (1)
164-164: Check the base path usage
Consider using “filepath.Join” if the path references a folder in a local environment so that Windows path separators are respected.
internal/exec/vendor_component_utils.go (1)
9-9: Avoid mixing “path” for local file paths
When dealing with real files in operating systems, “filepath” is safer.
pkg/config/config.go (5)
296-296: Maintain consistent path joining for Terraform base path.
Ensure environment-specific paths don’t conflict with path.Join usage.
[approve]
304-304: Same note applies to helmfileBasePath.
Double-check if path.Join fits all usage patterns.
✅ Verification successful
Let me search for how helmfileBasePath is being used in the codebase to verify if path.Join is appropriate for all usage patterns.
path.Join usage for helmfileBasePath is appropriate and verified
The code analysis shows that helmfileBasePath is used correctly in a path joining operation. It's immediately converted to an absolute path using filepath.Abs(), which is the proper way to handle filesystem paths. The usage pattern is consistent with the codebase's path handling requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for usages of helmfileBasePath
rg "helmfileBasePath" -A 3 -B 3
# Also search for any path manipulation patterns related to helmfile
rg "helmfile.*Path" -A 3 -B 3
Length of output: 14511
274-274: Watch out for Windows path separators.
Using path.Join here could break if the BasePath and Stacks.BasePath are mixed with backslashes.
8-8: Consider platform-specific nuances when using path.Join.
For OS-agnostic file paths (including Windows), filepath.Join might be more reliable.
151-151: Confirm consistency across all configuration paths.
This usage of path.Join can lead to cross-platform issues if called from Windows-based scripts.
internal/exec/describe_affected_utils.go (2)
1223-1223: Check local module references for Windows.
Using path.Join may result in broken references on certain systems.
435-437: Double-check cross-platform paths for stack elements.
Replacing filepath with path might be risky if these are local filesystem paths rather than URL paths.
✅ Verification successful
Let me gather more information about the path handling in this codebase.
This will help us understand:
- The current path handling approach in the file
- Whether filepath package is imported and used elsewhere
- If there's any OS-specific path handling logic
- The import statements to confirm which package is being used
Let me check the imports more thoroughly to understand the complete context.
The path handling is correct as implemented.
The code correctly uses both path and filepath packages based on their intended purposes:
path.Joinis used for constructing logical paths that are part of the Atmos configuration (BasePath, Components paths)filepathis used for actual filesystem operations (Abs, Rel) when dealing with the local filesystem
The codebase shows a consistent pattern where:
- Configuration paths use the
pathpackage for cross-platform consistency - Actual filesystem operations use
filepathfor OS-specific path handling - Both packages are properly imported and used according to Go's best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check for other usages of path vs filepath in the codebase
rg "path\." -A 2 internal/exec/describe_affected_utils.go
# Look for any platform-specific path handling
rg "filepath\." -A 2 internal/exec/describe_affected_utils.go
# Check imports to see which package is being used
rg "^import.*path" internal/exec/describe_affected_utils.go
rg "^import.*filepath" internal/exec/describe_affected_utils.go
# Look for any OS-specific path handling patterns
ast-grep --pattern 'if runtime.GOOS == $_'
Length of output: 3741
Script:
#!/bin/bash
# Check the full import block
rg "^import \(" -A 20 internal/exec/describe_affected_utils.go
# Check if this file has any OS-specific imports
rg "runtime|os" internal/exec/describe_affected_utils.go
# Look for any path manipulation utility functions
ast-grep --pattern 'func $_(path $_) $_'
Length of output: 10452
internal/exec/stack_processor_utils.go (1)
2183-2183: Confirm that baseComponent now uses path.Join.
If the component directory resides on Windows, validate that path.Join won't break references.
[approve]
pkg/utils/glob_utils.go (1)
6-6: Mixing path and OS-based file lookups can lead to confusion.
Check if intended usage is for file system or URL paths.
✅ Verification successful
Let me gather more information about the usage of the path package in the codebase to verify this concern.
This will help us understand:
- The full context of the glob_utils.go file
- How the path package is being used
- If there's any mixing with filepath package that could cause confusion
After analyzing the code, I can now provide a definitive response about the usage of the path package in glob_utils.go.
The path package usage here is correct and intentional.
The code is using path package specifically for path manipulation in a platform-agnostic way, while also providing a clear PathMatch function that serves as a drop-in replacement for filepath.Match() with explicit documentation about path separators. The code shows a clear understanding of when to use each:
path.Joinis used for clean path joining in a platform-agnostic wayfilepathfunctions are properly used when dealing with the actual filesystem
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's see the content of the file to understand the context
cat pkg/utils/glob_utils.go
# Then, let's search for usage of path-related functions across the codebase
rg "path\." -A 2
# Also check for filepath package usage to see if there's mixing
rg "filepath\." -A 2
Length of output: 63017
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/exec/vendor_utils.go (1)
Line range hint
1-645: Consider implementing a platform-agnostic path handling strategyGiven the history of path-related issues (mentioned in PR #856) and the addition of Windows-specific handling, consider:
- Creating a centralized path handling utility
- Documenting the path handling strategy
- Adding integration tests for cross-platform scenarios
This would help prevent future path-related issues and make the codebase more maintainable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exec/vendor_utils.go(4 hunks)
🔇 Additional comments (2)
internal/exec/vendor_utils.go (2)
6-8: Both path and filepath packages are imported
This could lead to confusion as both packages are used for path manipulation. The PR description mentions reverting from filepath.Join to path.Join, but both imports remain.
Let's check the usage patterns:
✅ Verification successful
Both path and filepath packages are used appropriately
Based on the usage patterns found in the codebase:
path.Joinis primarily used for handling URL-style paths and configuration paths that need to be platform-independentfilepath.Joinis correctly used for filesystem operations where OS-specific path separators are required
The coexistence of both packages is intentional and follows Go's best practices:
pathfor virtual/URL paths (platform-independent)filepathfor actual filesystem paths (platform-specific)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage patterns of path.Join and filepath.Join
echo "Checking path.Join usage:"
rg "path\.Join\(" -A 1
echo -e "\nChecking filepath.Join usage:"
rg "filepath\.Join\(" -A 1
Length of output: 24000
556-558: Ensure consistent path handling across platforms
The usage of sanitizeFileName here is good, but we should ensure consistent path handling across all platforms.
Let's check for similar patterns that might need the same treatment:
✅ Verification successful
Let me check for any instances where file paths are constructed without proper sanitization. I'll search for direct file path construction patterns.
Let me check the implementation of the sanitizeFileName function to ensure it's properly handling paths.
Let me see the complete implementation of sanitizeFileName since it was cut off.
Let me get the complete function implementation as it seems to be cut off.
Path handling is properly implemented and secure
The sanitizeFileName function is well-implemented with proper platform-specific handling:
- On non-Windows systems, it safely extracts the base filename
- On Windows, it properly sanitizes invalid characters and handles query strings
- The function is correctly used in the codebase for safe filename construction
All other instances of filepath.Join in the codebase are used appropriately for path construction, and don't require additional sanitization as they're working with internal paths rather than external URIs.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for other instances where file paths are constructed
rg "filepath\.Join\([^)]*\)" -A 1
Length of output: 5968
Script:
#!/bin/bash
# Look for direct file path construction without sanitization
rg "filepath\.Join.*\(" -C 2 internal/exec/
Length of output: 12862
Script:
#!/bin/bash
# Check for the sanitizeFileName function implementation and usage
ast-grep --pattern 'func sanitizeFileName($_) { $$$ }'
Length of output: 56
Script:
#!/bin/bash
# Look for other file name handling functions
rg "func.*[Ff]ile[Nn]ame" -A 2
Length of output: 2685
Script:
#!/bin/bash
# Get the implementation of sanitizeFileName
rg -A 10 "func sanitizeFileName" internal/exec/vendor_utils.go
Length of output: 355
Script:
#!/bin/bash
# Get the complete implementation of sanitizeFileName with more context
rg -A 20 "func sanitizeFileName" internal/exec/vendor_utils.go
Length of output: 623
Script:
#!/bin/bash
# Get the complete implementation with more lines
rg -A 30 "func sanitizeFileName" internal/exec/vendor_utils.go
Length of output: 928
Script:
#!/bin/bash
# Look for any calls to this function
rg "sanitizeFileName\(" internal/exec/
Length of output: 184
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exec/vendor_model_component.go (1)
162-162: Avoid duplicating path-sanitization logic."tempDir2 = filepath.Join(tempDir, sanitizeFileName(p.uri))" repeats the logic at line 127. Consider extracting this path creation into a helper method to improve reusability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/exec/vendor_component_utils.go(4 hunks)internal/exec/vendor_model.go(1 hunks)internal/exec/vendor_model_component.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/exec/vendor_model.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/exec/vendor_component_utils.go
🔇 Additional comments (1)
internal/exec/vendor_model_component.go (1)
127-127: Consider validating the sanitized path.
Using "sanitizeFileName(p.uri)" is effective for removing invalid path characters. However, ensure that sanitized names don't become empty or produce collisions if different URIs map to the same sanitized value.
osterman
added
minor
|
These changes were released in v1.131.0. |
This reverts commit 2051592.
what
why
Summary by CodeRabbit