Skip to content

Conversation

@Rupikz
Copy link
Contributor

@Rupikz Rupikz commented Oct 18, 2025

Description

  1. Close source in tests and examples
  2. Make fileresolver.NewFromFile itself-resolve absolute paths
  3. Move GetDirectoryExclusionFunctions to a common package
  4. Defer fileAnalysisPath calculation to preserve the cleanupFn

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation (updates the documentation)
  • Chore (improve the developer experience, fix a test flake, etc, without changing the visible behavior of Syft)

Checklist:

  • I have added unit tests that cover changed behavior
  • I have tested my code in common scenarios and confirmed there are no regressions
  • I have added comments to my code, particularly in hard-to-understand sections

Copy link
Contributor

@wagoodman wagoodman left a 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).

Copy link
Contributor

@kzantow kzantow left a 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

// 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) {
Copy link
Contributor

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

Copy link
Contributor Author

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]>
@Rupikz
Copy link
Contributor Author

Rupikz commented Oct 20, 2025

Thanks for review and the detailed explanations. I'll be more careful to avoid making breaking changes like this in the future

@wagoodman wagoodman changed the title Refactoring fileresolver Refactor fileresolver to not require base path Oct 29, 2025
@wagoodman wagoodman dismissed kzantow’s stale review October 29, 2025 14:41

technically the base path is not expressible in the SBOM output when the file source is used

@wagoodman wagoodman merged commit f5c7651 into anchore:main Oct 29, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants