-
Notifications
You must be signed in to change notification settings - Fork 314
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
feat(Provenance): Add RemoteProvenance
sub interface
#9476
base: main
Are you sure you want to change the base?
Conversation
Hi @sschuberth, do you have any feedback for me yet? |
@@ -38,6 +38,7 @@ import org.ossreviewtoolkit.model.OrtResult | |||
import org.ossreviewtoolkit.model.PackageCuration | |||
import org.ossreviewtoolkit.model.Project | |||
import org.ossreviewtoolkit.model.Provenance | |||
import org.ossreviewtoolkit.model.RemoteProvenance |
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.
The intention of the commit is good. I'm just curious how you ensured to capture all occurrences. Can you describe your approach for that in the commit message?
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.
I utilized IntelliJ's Find Usage feature to find all occurrences of KnownProvenance
by categories, such as is
conditionals, cast type, function parameter, return type, etc.
For this commit (78e4be6) I went through the is
conditional findings and investigated their context by checking the surrounding and when necessary the code calling the relevant function.
For 2d70747 I checked the cast types and for 7a6f396 I checked the function parameters for KnownProvenance
as well as the is
conditionals for RepositoryProvenance
and ArtifactProvenance
.
Would you like me to add a note to all three commits in regards to the process? (I could always refer to "as described in the previous commit" ofc)
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.
I just added the descriptions to the 3 commits. Let me know if this describes the approach clear enough.
@@ -169,7 +170,7 @@ internal fun OrtResult.getLicenseFindingsById( | |||
packageConfigurationProvider.getPackageConfigurations(id, provenance).flatMap { it.licenseFindingCurations } | |||
} | |||
|
|||
getScanResultsForId(id).filter { it.provenance is KnownProvenance }.map { | |||
getScanResultsForId(id).filter { it.provenance is RemoteProvenance }.map { |
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.
Shouldn't that actually have been RepositoryProvenance
only to begin with, @fviernau, because the map calls filterByVcsPath()
?
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.
Since it uses forEach
and the ?
operator, I assume any non-RepositoryProvenance
s would be skipped by default, which was why KnownProvenance
was the conditional here, not the stricter RepositoryProvenance
.
EDIT: If my conclusion is correct, using KnownProvenance
should be perfectly fine though. Let me know what you think and I will remove the change if you agree.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 <jens.keim@forvia.com>
39d170a
to
7a6f396
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9476 +/- ##
============================================
- Coverage 68.09% 67.93% -0.17%
+ Complexity 1291 1290 -1
============================================
Files 250 249 -1
Lines 8814 8813 -1
Branches 916 975 +59
============================================
- Hits 6002 5987 -15
- Misses 2427 2436 +9
- Partials 385 390 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
7a6f396
to
b823ee6
Compare
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 <jens.keim@forvia.com>
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 <jens.keim@forvia.com>
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 <jens.keim@forvia.com>
b823ee6
to
e67dac2
Compare
@sschuberth are you happy with the changes I made? Also any word from @mnonnenmacher and @fviernau regarding your questions? |
In preparation to split
KnownProvenance
hierarchy between the upcomingLocalProvance
s and the existingRemoteProvenance
s, bothRepositoryProvenance
andArtifactProvance
now inherit from theRemoteProvenance
interface instead ofKnownProvenance
.Conditional types, casts, and conditional cases were adjusted as to fit the new interface hierarchy.
Signed-off-by: Jens Keim jens.keim@forvia.com