Support/use recursive matching required file patterns as needed#1394
Conversation
It looks like there are some cases where checks need to extract recursive subdirectories/files. Use the doublestar library to support Bash-style globstar patterns, which can support this. Signed-off-by: Caleb Xu <caxu@redhat.com>
Some container images may place licenses in a nested subdirectory structure in /licenses (while no license files reside in /licenses itself). This was previously supported before f7103d0. Signed-off-by: Caleb Xu <caxu@redhat.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR introduces the doublestar library to support recursive matching (globstar) in the engine's untar function and uses it for HasLicenseCheck. While the changes to the engine are solid and include a good test case, the HasLicenseCheck update is currently incomplete as its validation logic is not yet recursive.
🔍 General Feedback
- Recursion Depth: The engine now correctly extracts files recursively when using
**. - Validation Consistency: Ensure that checks using recursive patterns also update their internal validation logic to traverse the extracted directories.
- Dependency:
doublestaris a well-established library for this purpose in the Go ecosystem.
|
|
||
| matches := slices.ContainsFunc(filterPatterns, func(p string) bool { | ||
| result, _ := filepath.Match(p, header.Name) | ||
| result, _ := doublestar.Match(p, header.Name) |
There was a problem hiding this comment.
🟢 The error returned from doublestar.Match is ignored here. While it was also ignored for filepath.Match, it might be worth logging or handling it if a pattern is malformed (e.g., if an invalid globstar pattern is used). Since the patterns are defined within the codebase, it's low risk, but good for future-proofing.
|
from change #1394: |
|
from change #1394: |
caxu-rh
force-pushed
the
required-file-patterns-globstar
branch
from
3dccf7b to
4a98bc6
Compare
openshift-ci
Bot
added
lgtm
|
from change #1394: |
There was a problem hiding this comment.
This PR introduces recursive file pattern matching using the doublestar library, primarily to support deeper license file checks in the HasLicenseCheck. The overall approach is solid and the test coverage for nested subdirectories is a good addition.
🔍 General Feedback
- Recursion behavior: The use of
doublestar.PathMatchis recommended overdoublestar.Matchto ensure that existing glob patterns (using*) remain single-level unless explicitly switched to**. - Validation errors: In Preflight, validation failures (like a missing directory) should generally return
false, nilrather than an error to distinguish them from engine-level execution failures. - Refactoring: The code cleanup in
has_license.goto useslices.ContainsFuncis a nice improvement in readability and follows modern Go idioms.
|
|
||
| matches := slices.ContainsFunc(filterPatterns, func(p string) bool { | ||
| result, _ := filepath.Match(p, header.Name) | ||
| result, _ := doublestar.Match(p, header.Name) |
There was a problem hiding this comment.
🟡 Use doublestar.PathMatch instead of doublestar.Match.
doublestar.Match allows * and ? to match across directory separators (e.g., licenses/* would match licenses/nested/LICENSE). This change affects all existing checks that use * but do not expect recursive matching, potentially leading to more files being extracted than necessary.
doublestar.PathMatch is a closer replacement for filepath.Match as it restricts * to a single directory level while still supporting ** for explicit recursion, which aligns with the goal of only using nested matching where explicitly requested (via **).
| result, _ := doublestar.Match(p, header.Name) | |
| matches := slices.ContainsFunc(filterPatterns, func(p string) bool { | |
| result, _ := doublestar.PathMatch(p, header.Name) | |
| return result | |
| }) |
caxu-rh
force-pushed
the
required-file-patterns-globstar
branch
from
4a98bc6 to
3770fe0
Compare
Only consider files, not directories, as valid license files, and search in subdirs within the license directory, as valid license files may not be at the top level. Signed-off-by: Caleb Xu <caxu@redhat.com>
caxu-rh
force-pushed
the
required-file-patterns-globstar
branch
from
3770fe0 to
0b859f7
Compare
|
from change #1394: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acornett21, bcrochet, caxu-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
bcrochet
merged commit 8381364
into
redhat-openshift-ecosystem:main
5 participants
Alternative fix for the problem/context mentioned in #1393.
**) in the situations where it's needed (so far, onlyHasLicenseCheck)/licensesdirectory containing only directories at the top level (regardless of whether these child directories contained licenses or were empty) would always be considered passingHasLicensecheck