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

Attempt to fix Task not following symlinks #831

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

ilewin
Copy link
Contributor

@ilewin ilewin commented Aug 8, 2022

Please check if this could be a good solution for the described bug.

@ilewin ilewin force-pushed the check_path_for_symlinks_issue_826 branch from 5e32164 to 98ba3fd Compare August 12, 2022 09:30
@andreynering andreynering changed the title Issue #826 Attempt to fix Task not following symlinks Aug 18, 2022
Copy link
Member

@andreynering andreynering left a comment

Choose a reason for hiding this comment

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

Hi @ilewin,

Given there's an issue open on mvdan/sh#843, I think we should not try to fix it when running shell commands and wait for a fix upstream.

Let's code the fix for the source calculation only.


Regarding the PR itself, please write a meaningful commit message and be careful with unnecessary changes to the codebase like changing new lines, commented code, etc (specially on files you didn't change). The diff should be on relevant code only.

Comment on lines 33 to 34
//fs, err := zglob.Glob(g) //Looks like this does not evaluate **/* glob correctly
fs, err := filepath.Glob(g)
Copy link
Member

Choose a reason for hiding this comment

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

filepath.Glob does not support double star (foo/**/*.js). We should keep using zglob.Glob

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, Thanks I will dig more.
Zglob does not follow symlinks so I will try to figure out how to have both ** and symlinks
mattn/go-zglob#3

@ilewin ilewin force-pushed the check_path_for_symlinks_issue_826 branch from 98ba3fd to 6e18872 Compare August 23, 2022 14:51
@ilewin ilewin force-pushed the check_path_for_symlinks_issue_826 branch from 6e18872 to 2a67499 Compare August 23, 2022 16:25
@andreynering andreynering added the type: enhancement A change to an existing feature or functionality. label Aug 23, 2022
@andreynering
Copy link
Member

Thanks @ilewin!

@andreynering andreynering merged commit beb927f into go-task:master Aug 23, 2022
andreynering added a commit that referenced this pull request Aug 23, 2022
@ilewin ilewin deleted the check_path_for_symlinks_issue_826 branch August 24, 2022 06:15
@ilewin ilewin restored the check_path_for_symlinks_issue_826 branch August 24, 2022 06:15
@ilewin ilewin deleted the check_path_for_symlinks_issue_826 branch August 24, 2022 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A change to an existing feature or functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants