Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support discovery of symlinked modules #3562

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

rodrigorfk
Copy link
Contributor

@rodrigorfk rodrigorfk commented Nov 14, 2024

Description

Fixes #1611.

This PR continues the work that was started by @aslafy-z in the staled #3101 PR as following:

  • It adds a WalkWithSymlinks function to the util package, here is the reason for having it's own implementation in the Terragrunt codebase:
    • the facebookgo/symwalk implementation is the closest one for Terragrunt's use case, however that library is archived and the license does not adhere to Terragrunt's rules, it also does not handle circular symlinks.
    • the edwardrf/symwalk implementation does handle circular symlinks, but it does not allow walking the full logical folder structure, therefore the end result provided by it won't match the expectations of this issue.
    • It is also important to clarify the usage of the edwardrf/symwalk on the original implementation was working only because the usage of helpers.CopyEnvironment in the tests, was resolving copying and symlinks statically, and not really preserving it on the temporary env path, which defeats the purpose of test and does not mimic what terragrunt will be doing when running targeting a real directory with symlinks directly into it, this issue was addressed in my tests and a comment regarding to it was left in the codebase.
  • The integration tests are also adding some tests regarding to .terragrunt-cache invalidation, which was one of the concerns added by @denis256 feat: support discovery of symlinked modules #3101 (comment)

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added support for discovery of symlinked modules

@denis256
Copy link
Member

1 test failed out of 1501
TestWalkWithCircularSymlinks

@rodrigorfk
Copy link
Contributor Author

Thanks for the input @denis256, there was a minor issue in the func, regarding to circular links, that was producing different results on MacOS and Linux. It is now fixed and the lint strict issues were also addressed. Could you please run the tests again?

@rodrigorfk rodrigorfk force-pushed the fix-symlinks-modules branch 2 times, most recently from 9268d37 to c9f79ea Compare November 15, 2024 10:09
@yhakbar
Copy link
Collaborator

yhakbar commented Nov 18, 2024

Minor spelling issue that we can ignore, but tests are passing.

./util/file.go:909: evalutate ==> evaluate
./util/file.go:922: evalutate ==> evaluate

I'll let @denis256 decide if we should approve and merge.

@rodrigorfk
Copy link
Contributor Author

Thanks for running the tests @yhakbar.

More than happy fix the spelling issue if needed @denis256. Also a bit puzzled by the strict_lint failure. I ran make run-strict-lint locally and didn't get any pending issue. Could you please let me know what is the issue? Thanks in advance.

@denis256
Copy link
Member

denis256 commented Nov 19, 2024

./util/file.go:909: evalutate ==> evaluate
./util/file.go:922: evalutate ==> evaluate

🕵️

@rodrigorfk
Copy link
Contributor Author

@denis256, spelling issue fixed, both codespell . and make run-strict-lint succeeded at my end now:

image

@yhakbar
Copy link
Collaborator

yhakbar commented Nov 19, 2024

Thanks, @rodrigorfk . We're testing some edge cases to make sure that nothing is awry before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symlinks in Terragrunt
3 participants