-
Notifications
You must be signed in to change notification settings - Fork 731
Refactor fileresolver to not require base path #4298
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
Conversation
Signed-off-by: Kudryavcev Nikolay <[email protected]>
Signed-off-by: Kudryavcev Nikolay <[email protected]>
Signed-off-by: Kudryavcev Nikolay <[email protected]>
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.
points 1, 2 and 4 are good changes (🚀 ), however, we wont be able to accept point 3 here (since migrating a public function across packages is a breaking change).
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.
EDIT: Alex already noted these things
syft/internal/fileresolver/file.go
Outdated
| // path is the filepath of the file we're creating content access for | ||
| func NewFromFile(parent, path string, pathFilters ...PathIndexVisitor) (*File, error) { | ||
| chroot, err := NewChrootContextFromCWD(parent, parent) | ||
| func NewFromFile(path string, pathFilters ...PathIndexVisitor) (*FileResolver, error) { |
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.
It looks like this is removing the parent here, I'm pretty sure there are some use cases where this is necessary to be a directory different than the parent of the provided path
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.
Looking at the tests and fileSource.FileResolver(), I didn't see a need for this. But on a broader note, is it even correct to index a directory that's different from the file's absolute path?
Signed-off-by: Kudryavcev Nikolay <[email protected]>
|
Thanks for review and the detailed explanations. I'll be more careful to avoid making breaking changes like this in the future |
technically the base path is not expressible in the SBOM output when the file source is used
Description
fileresolver.NewFromFileitself-resolve absolute pathsMoveGetDirectoryExclusionFunctionsto a common packagefileAnalysisPathcalculation to preserve the cleanupFnType of change
Checklist: