follow symlinks for path_relative_to_include #1550
Conversation
|
Happy to work on this more if I get confirmation that the 'help wanted' tag on #543 is still accurate. (Maybe after >2 years the change is no longer desired?) |
|
@Patricol Sorry for the delay. We are super, super buried right now. The PR is very much appreciated and we'll review it as soon as we can! |
|
Thanks for the PR and your patience as we got around to reviewing it!
Unless you know of a specific use case where the current approach would break as a result, I'd rather avoid extra flags / branches.
Yes, probably.
Sure. Note that for symlinks to work, we may also need to solve #1611. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for submitting this pull request. |
WalkthroughThis change updates the Changes
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (3)
config/config_helpers.go (3)
302-310:⚠️ Potential issueGood addition, but needs error handling!
Adding
filepath.EvalSymlinkshelps follow symlinks when determining paths, which is great for the use case of numbered symlinks to Terragrunt directories. However, this function can return an error that's not being handled. If the symlink is broken or can't be resolved, this will likely panic at runtime.Consider handling the error by modifying the code like this:
currentPath := filepath.Dir(terragruntOptions.TerragruntConfigPath) -currentPath = filepath.EvalSymlinks(currentPath) +resolvedPath, err := filepath.EvalSymlinks(currentPath) +if err != nil { + return "", errors.WithStackTrace(err) +} +currentPath = resolvedPath
313-327: 🛠️ Refactor suggestionUpdate
pathRelativeFromIncludefor consistencyFor complete symlink support,
pathRelativeFromIncludeshould also evaluate symlinks like we're doing inpathRelativeToInclude. According to the PR discussion, brikis98 agreed this function should likely receive similar treatment.Add symlink evaluation to this function too:
includePath := filepath.Dir(include.Path) currentPath := filepath.Dir(terragruntOptions.TerragruntConfigPath) + +resolvedPath, err := filepath.EvalSymlinks(currentPath) +if err != nil { + return "", errors.WithStackTrace(err) +} +currentPath = resolvedPath
302-310: 💡 Verification agent🧩 Analysis chain
Need to add tests for symlink behavior
The PR mentioned that testing wasn't completed yet, particularly for relative include paths. Adding tests would be valuable to ensure the symlink behavior works as expected in different scenarios.
🏁 Script executed:
#!/bin/bash # Look for existing tests related to the pathRelativeToInclude function echo "Searching for existing tests for pathRelativeToInclude..." rg -A 3 "pathRelativeToInclude" --type goLength of output: 1584
🏁 Script executed:
#!/bin/bash # Check if existing tests simulate or reference symlinks echo "Searching for 'Symlink' or 'symlink' in config_helpers tests..." rg -n "Symlink" -A5 config/config_helpers_test.go || rg -n "symlink" -A5 config/config_helpers_test.go || echo "No symlink references found in tests."Length of output: 333
Add symlink-specific tests for pathRelativeToInclude
We’ve got good coverage for normal include paths, but there are no tests hitting the
EvalSymlinksbranch. Let’s add a few cases inconfig/config_helpers_test.goto exercise include and config paths that go through symlinks:• Create a temp dir, make a real folder (
realDir), then a symlink (linkDir) pointing at it.
• PointterragruntOptions.TerragruntConfigPathatlinkDir/config.hcland aninclude.Pathinside that symlink.
• Also test an absoluteinclude.Paththat resolves via a symlink.
• Assert that the returned relative path matches what you get by manually callingfilepath.EvalSymlinks+GetPathRelativeTo.That way we catch any regressions around
EvalSymlinksbehavior. Thanks!
🧹 Nitpick comments (1)
config/config_helpers.go (1)
304-304: Consider also evaluating the includePathFor complete symlink resolution, it might make sense to also resolve symlinks in the includePath, not just the currentPath. This would ensure that relative path calculations work correctly regardless of which path contains symlinks.
includePath := filepath.Dir(include.Path) currentPath := filepath.Dir(terragruntOptions.TerragruntConfigPath) currentPath = filepath.EvalSymlinks(currentPath) + +resolvedIncludePath, err := filepath.EvalSymlinks(includePath) +if err != nil { + return "", errors.WithStackTrace(err) +} +includePath = resolvedIncludePath
Closes #543
My use case involves wanting to prefix terragrunt dirs with numbers to make the stage order obvious, generate state keys based on path to ensure uniqueness, and use a fair number of
terraform_remote_stateobjects.Reordering or inserting stages is a time-consuming process involving moving many different state files, adjusting
terraform_remote_stateobjects, and renaming folders.Also,
cding isn't as frictionless as it could be; with the extra often 0-padded prefixes.Creating numbered symlinks to the terragrunt dirs (but having terragrunt ignore those symlinks) would be an ideal solution.
Things to consider:
path_relative_to_includeshould take a boolean argument if possible; defaulting to the old behavior but allowing the use of this behavior.path_relative_from_includetoo?filepath.Cleanon the path too?Summary by CodeRabbit