-
Notifications
You must be signed in to change notification settings - Fork 242
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(pattern-matcher): add CanSkipDir
method in patternMatcher
#1656
base: main
Are you sure you want to change the base?
Conversation
4fc6694
to
eb17cc5
Compare
Add `CanSkipDir` to `MatchesResult` Signed-off-by: Black-Hole1 <bh@bugs.cc>
eb17cc5
to
6842c6f
Compare
Thank you for your CR @vrothberg. Modification Completed. |
} | ||
} | ||
} | ||
|
||
if res.matches > 0 { | ||
if res.isMatched { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous implementation had a bug. When the rule is ["1", "!1"]
, it would also enter this conditional statement, but it should not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! @saschagrunert PTAL
CanSkipDir
method in patternMatcherCanSkipDir
method in patternMatcher
LGTM |
The comment in the podman source is incorrect - podman only needs to (and by that I mean should) descend into a directory that's covered by an ignore pattern if the directory exactly matches the initial portion of a '!' pattern - it doesn't need to descend into directories that might only match a '!' pattern by matching using wild cards. I don't think that this will help podman determine if that's the case. |
Hey, @nalind. I didn't understand what you meant. Could you please provide a few examples to illustrate your point? If you could provide an example in the format of a unit test, it would be even better. For example: |
Specifically, if I have a directory in my build context named "foo" with a file in it named "bar", and my ignores list is In this version of the PR, the last test case in |
@nalind I think I may understand the meaning of what you said. Here are my arguments: TL;DR
This comment explains under what conditions Therefore, in this PR, the conditions for setting it to true are very strict: the |
@nalind Ping. |
@nalind PTAL |
Add
CanSkipDir
toMatchesResult
This PR is mainly aimed at addressing https://github.com/containers/podman/blob/fca3c2ef8476669825564af89b008946bdf87599/pkg/bindings/images/build.go#L699-L701