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.

Sign up for GitHub

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

Jump to bottom

feat: allow restricting filesystem walk to specific folders #5481

Closed
wants to merge 1 commit into from
Closed

feat: allow restricting filesystem walk to specific folders #5481

wants to merge 1 commit into from

Conversation

lebauce
Copy link
Contributor

@lebauce lebauce commented Oct 31, 2023 •
edited
Loading

Description

This PR passes a new parameter to the walker to only parse the files
in a specified set of folders.

This can greatly improve the performance when a specific set of parsers
is used and the required paths for these parsers is known.

Related issues

  • Close #XXX

Related PRs

  • #XXX
  • #YYY

Remove this section if you don't have related PRs.

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@lebauce lebauce requested a review from knqyf263 as a code owner October 31, 2023 16:30
@lebauce lebauce force-pushed the only-dirs branch 6 times, most recently from 66dc684 to 75aabe4 Compare November 1, 2023 00:43
@lebauce lebauce marked this pull request as draft November 1, 2023 02:07
@lebauce lebauce marked this pull request as ready for review November 1, 2023 02:22
knqyf263
knqyf263 reviewed Nov 2, 2023
View reviewed changes
pkg/fanal/walker/walk.go
@@ -91,5 +100,17 @@ func (w *walker) shouldSkipDir(dir string) bool {
}
}

if dir != "." && len(w.onlyDirs) > 0 {
for _, onlyDir := range w.onlyDirs {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it work with the nested dir? Let's say --only-dirs /*/bar/** is passed. Then, Trivy walks from the root and reaches /foo. This function returns false as /*/bar/** doesn't match /foo. It never reaches /foo/bar even though /foo/bar/baz.txt exists. I may be missing something.

https://go.dev/play/p/wdZ1cZ89kSx

Copy link
Contributor Author

@lebauce lebauce Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You're correct. At first I didn't support wildcards but when rebasing, I saw the use of "doublestar.Match" and switched to it. In this case, it only works with leading "wildcards" - which brings no value - because of the case you mentionned.

I suggest toswitch back to use regular paths. WDYT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the patterns but apply them when considering files. When possible, it also skips some directories to avoid having traversing them.

@lebauce lebauce force-pushed the only-dirs branch 4 times, most recently from 0fe6eb8 to 7bacd88 Compare November 10, 2023 09:59
@lebauce lebauce requested a review from knqyf263 November 10, 2023 10:19
@lebauce lebauce force-pushed the only-dirs branch 3 times, most recently from 2dd828c to f3265c6 Compare November 16, 2023 08:54
@lebauce
Copy link
Contributor Author

lebauce commented Nov 16, 2023

@knqyf263 Could you please have an other look ? Thanks !

@knqyf263
Copy link
Collaborator

I'm sorry for the late reply. I'm moving to a new country and busy with paperwork, transportation, etc.
I'm unsure if we need this functionality. Can you please raise a GitHub discussion first? I'd like to see how much the community needs.

@github-actions GitHub Actions
Copy link

This PR is stale because it has been labeled with inactivity.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed. label Mar 18, 2024
@github-actions github-actions bot closed this Apr 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knqyf263 knqyf263 Awaiting requested review from knqyf263 knqyf263 is a code owner

Assignees
No one assigned
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and will be auto-closed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lebauce @knqyf263