fix: provider cache path handling in Windows#5788
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWarm-up now distinguishes local filesystem paths from remote URLs by checking for "://"; local files set archivePath without download, remote URLs use existing download/retry. Cleanup no longer removes archivePath unless archive was cached. Adds Windows test fixtures, integration test, and changelog entry. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
denis256
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/tf/cache/services/provider_cache.go`:
- Around line 335-337: The code currently repoints cache.archivePath to
cache.DownloadURL when downloadURLIsLocalFile, but later cleanup removes
cache.archivePath unconditionally which can delete user-provided files; modify
the logic so that when downloadURLIsLocalFile is true you mark the archive as
user-provided (e.g., set a boolean like cache.archiveIsUserProvided or
cache.shouldCleanupArchive=false) and ensure the cleanup routine that removes
cache.archivePath (the block referenced around the existing cleanup) checks that
flag (or the inverse of downloadURLIsLocalFile) before deleting; update any
constructors/structs to include the flag and use it in the cleanup path so only
temporary downloaded archives are removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82f65c94-c321-4438-b2fd-a704434d8185
📒 Files selected for processing (4)
internal/tf/cache/services/provider_cache.gotest/fixtures/provider-cache/windows-remote-url/main.tftest/fixtures/provider-cache/windows-remote-url/terragrunt.hcltest/integration_windows_test.go
yhakbar
reviewed
Apr 2, 2026
| return errors.New(err) | ||
| // DownloadURL can be a local file path or a remote URL. | ||
| // On Windows, passing a URL to FileExists fails because the colon in "https:" is invalid path syntax. | ||
| var downloadURLIsLocalFile bool |
Collaborator
There was a problem hiding this comment.
NIT: I feel like this would be better served as a private function call.
yhakbar
previously approved these changes
Apr 2, 2026
Collaborator
yhakbar
left a comment
yhakbar
left a comment
There was a problem hiding this comment.
Make sure to update the changelog too:
https://github.com/gruntwork-io/terragrunt/blob/main/docs/src/data/changelog/unreleased.mdx
Feel free to rename the file to v1.0.0.mdx when you edit it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/data/changelog/v1.0.1.mdx`:
- Around line 48-49: Update the sentence on line 48 to accurately reflect the
code path: replace the claim that the URL was passed to os.Stat with wording
that the remote URL reached the vfs.FileExists check (which calls fs.Stat
underneath) in provider_cache.go; mention vfs.FileExists and fs.Stat to make the
documentation precise about where the filesystem existence check is skipped for
URLs containing "://".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77ce32c9-bfe2-40a3-ae16-055a7aac2f05
📒 Files selected for processing (1)
docs/src/data/changelog/v1.0.1.mdx
Comment on lines
+48
to
+49
| The provider cache failed on Windows with `CreateFile https://...: The filename, directory name, or volume label syntax is incorrect` because remote download URLs were passed to `os.Stat`, and the colon in `https:` is invalid Windows path syntax. The fix skips the filesystem existence check when the download URL is a remote URL (`://`), going directly to the download path. | ||
|
|
Contributor
There was a problem hiding this comment.
Tighten Line 48 wording to match the actual code path.
Line 48 says the URL was passed to os.Stat, but the implementation guards vfs.FileExists (which calls fs.Stat underneath). Updating this avoids implying a direct os.Stat call in provider_cache.go.
Suggested docs wording
-The provider cache failed on Windows with `CreateFile https://...: The filename, directory name, or volume label syntax is incorrect` because remote download URLs were passed to `os.Stat`, and the colon in `https:` is invalid Windows path syntax. The fix skips the filesystem existence check when the download URL is a remote URL (`://`), going directly to the download path.
+The provider cache failed on Windows with `CreateFile https://...: The filename, directory name, or volume label syntax is incorrect` because remote download URLs were being treated like local filesystem paths during existence checks (`vfs.FileExists`/`fs.Stat`), and the colon in `https:` is invalid Windows path syntax. The fix skips the filesystem existence check when the download URL is remote (`://`) and proceeds directly with provider download/caching.As per coding guidelines docs/**/*.md*: documentation should be clear and accurate.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| The provider cache failed on Windows with `CreateFile https://...: The filename, directory name, or volume label syntax is incorrect` because remote download URLs were passed to `os.Stat`, and the colon in `https:` is invalid Windows path syntax. The fix skips the filesystem existence check when the download URL is a remote URL (`://`), going directly to the download path. | |
| The provider cache failed on Windows with `CreateFile https://...: The filename, directory name, or volume label syntax is incorrect` because remote download URLs were being treated like local filesystem paths during existence checks (`vfs.FileExists`/`fs.Stat`), and the colon in `https:` is invalid Windows path syntax. The fix skips the filesystem existence check when the download URL is remote (`://`) and proceeds directly with provider download/caching. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/src/data/changelog/v1.0.1.mdx` around lines 48 - 49, Update the sentence
on line 48 to accurately reflect the code path: replace the claim that the URL
was passed to os.Stat with wording that the remote URL reached the
vfs.FileExists check (which calls fs.Stat underneath) in provider_cache.go;
mention vfs.FileExists and fs.Stat to make the documentation precise about where
the filesystem existence check is skipped for URLs containing "://".
yhakbar
previously approved these changes
Apr 2, 2026
yhakbar
approved these changes
Apr 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Before
After
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Summary by CodeRabbit