feat(Provenance): Add RemoteProvenance sub interface
#9476
Conversation
|
Hi @sschuberth, do you have any feedback for me yet? |
model/src/main/kotlin/ScannerRun.kt
Outdated
| @@ -108,7 +108,7 @@ data class ScannerRun( | |||
|
|
|||
| init { | |||
| scanResults.forEach { scanResult -> | |||
| require(scanResult.provenance is KnownProvenance) { | |||
| require(scanResult.provenance is RemoteProvenance) { | |||
There was a problem hiding this comment.
I'm not sure whether this has to be / should be changed. If the requirement fails, the log message explicitly talks about "an unknown provenance". And as we said it would be fine for the scanner to have the restriction that local provenances can only be scanned if the scanner is executed on the same machine as the analyzer, I think this can stay at KnownProvenance.
There was a problem hiding this comment.
More clearly: We should be able to scan non-remote provenance, but not store it in any scan storage, I believe. What's your view @mnonnenmacher?
There was a problem hiding this comment.
I'm not sure whether this has to be / should be changed. If the requirement fails, the log message explicitly talks about "an unknown provenance". [...] I think this can stay at
KnownProvenance.
Sounds reasonable. Removed the change.
Keeping the Thread open for @mnonnenmacher 's opinion.
I had started a review but wasn't able to finish it. I did so now. |
pepper-jk
force-pushed
the
provenance-hierarchy
branch
from
39d170a to
7a6f396
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9476 +/- ##
============================================
- Coverage 68.12% 68.08% -0.04%
Complexity 1292 1292
============================================
Files 250 250
Lines 8840 8843 +3
Branches 917 919 +2
============================================
- Hits 6022 6021 -1
- Misses 2431 2434 +3
- Partials 387 388 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pepper-jk
force-pushed
the
provenance-hierarchy
branch
2 times, most recently
from
b823ee6 to
e67dac2
Compare
|
@sschuberth are you happy with the changes I made? Also any word from @mnonnenmacher and @fviernau regarding your questions? |
pepper-jk
force-pushed
the
provenance-hierarchy
branch
from
e67dac2 to
285aaee
Compare
In preparation to split the `KnownProvenance` hierarchy further, for now introduce a `RemoteProvenance` that bundles the `RepositoryProvenance` and `ArtifactProvance`. Later an additional `LocalProvance` will be introduced. See [1] for context. [1]: oss-review-toolkit#8803 (comment) Signed-off-by: Jens Keim <[email protected]>
In some instances, the verification of `KnownProvenance` is a failsafe before expecting a `RepositoryProvenance` or `ArtifactProvenance`. Since these classes are now more strictly `RemoteProvenance`s, the conditionals are now using the new interface. All cases of `is KnownProvenance` conditionals were investigated for this, especially the s, using the _Find Usage_ Feature from IntelliJ, the recommended IDE for ORT. Both context of surrounding and calling code was investigated. Signed-off-by: Jens Keim <[email protected]>
In some instances, the casting to `KnownProvenance` is not explicitly required by receiving functions, here a more strict casting to `RemoteProvenance` is cleaner. All cases of `KnownProvenance` as a cast type were investigated by the same methods as the previous commit. Signed-off-by: Jens Keim <[email protected]>
Functions accepting a `KnownProvenance` often only handle the two explicit cases of `Artifact` or `RepositoryProvenance`. Add `else` cases to conditional `when`s. All conditional cases of `Artifact` and `RepositoryProvenance` following after `KnownProvenance` as a parameter type were investigated by the same methods as the previous commits. Signed-off-by: Jens Keim <[email protected]>
pepper-jk
force-pushed
the
provenance-hierarchy
branch
from
285aaee to
4e947ba
Compare
|
Closed in favor of #9860. |
In preparation to split
KnownProvenancehierarchy between the upcomingLocalProvances and the existingRemoteProvenances, bothRepositoryProvenanceandArtifactProvancenow inherit from theRemoteProvenanceinterface instead ofKnownProvenance.Conditional types, casts, and conditional cases were adjusted as to fit the new interface hierarchy.
Signed-off-by: Jens Keim [email protected]