refactor(PackageBasedScanStorageReader)!: Align read() to take a Package
#7641
Conversation
sschuberth
force-pushed
the
sschuberth/scan-storage-id-to-pkg
branch
from
885e4d9 to
d48f748
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #7641 +/- ##
=========================================
Coverage 68.03% 68.03%
Complexity 2023 2023
=========================================
Files 344 344
Lines 16727 16727
Branches 2372 2372
=========================================
Hits 11381 11381
Misses 4363 4363
Partials 983 983
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| */ | ||
| fun read(id: Identifier): Result<List<ScanResult>> { | ||
| val (result, duration) = measureTimedValue { readInternal(id) } | ||
| fun read(pkg: Package): Result<List<ScanResult>> { |
There was a problem hiding this comment.
commit and this read() function: I have difficulties understanding which parts of the package will be used to determine which scan results are being returned and how. Can you help?
There was a problem hiding this comment.
It's indeed a bit hard to understand as the relevant parts are not implemented yet... because they couldn't, as the ClearlyDefined scan storage provider did not have access to the necessary information before this change.
Basically, the ClearlyDefined implementation would need to have access to the VCS location, source artifact location (and maybe also the binary source artifact location) in order to create a proper Coordinate for querying harvested (scan) results. A Coordinate includes a ComponentType and Provider, and in order to determine these properly, more Package properties than just the Identifier are required.
There was a problem hiding this comment.
As Package contains many irrelevant properties I would prefer if the interface would only require the relevant values to have a cleaner API. But I would be fine with looking into it after fixing the ClearlyDefinedStorage.
| */ | ||
| fun read(id: Identifier): Result<List<ScanResult>> { | ||
| val (result, duration) = measureTimedValue { readInternal(id) } | ||
| fun read(pkg: Package): Result<List<ScanResult>> { |
There was a problem hiding this comment.
As Package contains many irrelevant properties I would prefer if the interface would only require the relevant values to have a cleaner API. But I would be fine with looking into it after fixing the ClearlyDefinedStorage.
sschuberth
force-pushed
the
sschuberth/scan-storage-id-to-pkg
branch
from
d48f748 to
686c9ec
Compare
A *package* based reader should indeed take a `Package` for all its `read()` functions. This aligns with the existing `read()` function that takes a `ScannerCriteria`, and it is a requirement for a proper implementation of `ClearlyDefinedStorage` which uses coordinates with a "provider" for lookup that can only be correctly constructed when provenance information is available. So far the `ClearlyDefinedStorage` hard-codes some assumptions in this regard, which will be corrected in upcoming changes. Signed-off-by: Sebastian Schuberth <[email protected]>
sschuberth
force-pushed
the
sschuberth/scan-storage-id-to-pkg
branch
from
686c9ec to
5ae52c3
Compare
Thanks. I was considering an API like for |
|
@fviernau are yo also OK to merge? |
A package based reader should indeed take a
Packagefor all itsread()functions. This aligns with the existingread()function that takes aScannerCriteria, and it is a requirement for a proper implementation ofClearlyDefinedStoragewhich uses coordinates with a "provider" for lookup that can only be correctly constructed when provenance information is available. So far theClearlyDefinedStoragehard-codes some assumptions in this regard, which will be corrected in upcoming changes.