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

fix: exclude symlinks to directories from hashing #1044

Merged
merged 2 commits into from
Jul 30, 2019
Merged

Conversation

10ko
Copy link
Member

@10ko 10ko commented Jul 29, 2019

What this PR does / why we need it:
This PR fixes #1029, which was caused by bad handling of symlink when initializing garden.

Which issue(s) this PR fixes:

Fixes #1029

Special notes for your reviewer:
We use the git client to retrieve all the files from a project, from where to extract then the module config paths. The current implementation then doesn't allow the use of symlinks: the git client will never resolve a symlink if it's a directory. It will always treat it as file and (this is now fixed/prevented) would try to hash it using the node streams, failing.
From a design point of view, I am not 100% aware of all the reasons behinf choosing git ls-files instead of something else. What I find strange is the jump from "using a library" to retrieve files and "using bare node streams" to hash it. It would be nice to understand if it makes sense to convert the search function to the same node standard or viceversa if we can use the git client to do the hashing.

Does this PR introduce a user-facing change?:
No

@10ko 10ko requested review from edvald, eysi09 and thsig July 29, 2019 21:21
@10ko 10ko force-pushed the fix-symlink-crash branch from a25c7a4 to d3bbc05 Compare July 29, 2019 21:23
Copy link
Collaborator

@edvald edvald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but it'd be great to have a simple test for this. I can imagine refactors accidentally breaking this at some point.

@10ko
Copy link
Member Author

10ko commented Jul 30, 2019

Makes sense, thanks!

@edvald edvald merged commit 514f9f5 into master Jul 30, 2019
@edvald edvald deleted the fix-symlink-crash branch July 30, 2019 14:39
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.

garden commands error if there's a broken symlink
2 participants