Helper CLI function for merging muliple analyzer results into one #5317
Conversation
This helper CLI function is used by Porsche to solve the issue oss-review-toolkit#4364 The rationale behind this is that some projects at Porsche deliver individual analyzer-results for each subproject in a large monorepo. The FOSS analysts need to see a condensed form of the individual dependency graphs across the project monorep. We solve this issue by merging all individual analyzer results into one. Signed-off-by: Rainer Bieniek <[email protected]>
|
@porsche-rbieniek can you please elaborate how this PR relates to #4698? |
There was a problem hiding this comment.
Commit message: Please stick to the line length limit.
If have further explained the reason behind @fviernau's concerns from #4698 (comment) in the code review. Based on the rationale from the commit message and the discussion in #4698 I think merging analyzer results is not the correct approach, because it is technically not possible to do it correctly. If all the mentioned limitations of this approach are no issue for your use case and all you need is a list of projects and packages, the better approach would be to let this helper-cli command output a custom format that fits your needs. Another alternative would be to implement this as a reporter, because these kind of reports are exactly what the reporter module is there for.
| @@ -101,7 +102,8 @@ internal class HelperMain : CliktCommand(name = ORTH_NAME, epilog = "* denotes r | |||
| SetLabelsCommand(), | |||
| SubtractScanResultsCommand(), | |||
| TransformResultCommand(), | |||
| VerifySourceArtifactCurationsCommand() | |||
| VerifySourceArtifactCurationsCommand(), | |||
| MergeAnalyzerResultsCommand() | |||
There was a problem hiding this comment.
Please put this in alphabetical order.
| @@ -0,0 +1,291 @@ | |||
| /* | |||
| * Copyright (C) 2021 Porsche AG | |||
There was a problem hiding this comment.
Copyright year should probably be 2021-2022.
| import org.ossreviewtoolkit.utils.core.Environment | ||
|
|
||
| class MergeAnalyzerResultsCommand : CliktCommand( | ||
| help = "Read multiple analyzer result files and merge them into one combined analyzer result file." |
There was a problem hiding this comment.
Please mention the most important limitations of the command here, esp. the flattening of the dependency tree.
| ) { | ||
| companion object { | ||
| private val utcClock = Clock.systemUTC() | ||
| private fun now(): Instant = ZonedDateTime.now(utcClock).toInstant() |
There was a problem hiding this comment.
This function is not required, you can just use Instant.now() like in the rest of the codebase.
| val inputOrtResults: MutableList<AnalyzerRun> = LinkedList() | ||
| val inputRepositories: MutableList<Repository> = LinkedList() | ||
|
|
||
| inputAnalyzerResultFiles.stream() |
There was a problem hiding this comment.
No need to use stream() in Kotlin (same several times below).
| } | ||
|
|
||
| private fun aggregateRepositoryConfigurations(repositories: List<Repository>): RepositoryConfiguration { | ||
| fun mergeExcludes(leftExlcudes: Excludes, rightExcludes: Excludes): Excludes = Excludes( |
| private fun aggregateRepositoryConfigurations(repositories: List<Repository>): RepositoryConfiguration { | ||
| fun mergeExcludes(leftExlcudes: Excludes, rightExcludes: Excludes): Excludes = Excludes( | ||
| paths = (leftExlcudes.paths + rightExcludes.paths).distinct(), | ||
| scopes = (leftExlcudes.scopes + rightExcludes.scopes).distinct() |
There was a problem hiding this comment.
Merging excludes can lead to unexpected results, if excludes from on analyzer result accidentally match content from another analyzer result.
There was a problem hiding this comment.
Point taken. The excludes will not be merged then
| packageLicenseChoices = ( | ||
| leftChoices.packageLicenseChoices | ||
| + rightChoices.packageLicenseChoices | ||
| ).distinct() |
There was a problem hiding this comment.
Merging license choices can lead to unexpected results, if they are contradictory, or if license choices from one analyzer result apply to findings from another analyzer result.
There are more similar consistency issues in the code below where data is lost because the model was not designed for this use case.
There was a problem hiding this comment.
This might be true but we chose to leave that problem to the analyst to sort it out.
Anyway, it is bad sign if project team choose contradicting license statements
| .orElse(RepositoryConfiguration()) | ||
| } | ||
|
|
||
| private fun aggregateAnalyzerConfiguration(inputOrtResults: List<AnalyzerRun>): AnalyzerConfiguration { |
There was a problem hiding this comment.
Consistently use either plural or singular for the aggregate function names, but don't mix them.
| set.addAll(right) | ||
|
|
||
| return set | ||
| } |
There was a problem hiding this comment.
This could be shorter:
sortedSetOf().apply {
addAll(left)
addAll(right)
}…view-toolkit#4364 The rationale behind this is that some projects at Porsche deliver individual analyzer-results for each subproject in a large monorepo. The FOSS analysts need to see a condensed form of the individual dependency graphs across the project monorep. We solve this issue by merging all individual analyzer results into one. This commit fixes issues raised during the community code review. Signed-off-by: Rainer Bieniek <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #5317 +/- ##
============================================
+ Coverage 72.36% 72.39% +0.03%
- Complexity 1964 1966 +2
============================================
Files 260 262 +2
Lines 13899 13920 +21
Branches 1960 1957 -3
============================================
+ Hits 10058 10078 +20
- Misses 2803 2807 +4
+ Partials 1038 1035 -3
Continue to review full report at Codecov.
|
This helper CLI function is used by Porsche to solve the issue #4364
The rationale behind this is that some projects at Porsche deliver individual analyzer-results for each subproject in a large monorepo. The FOSS analysts need to see a condensed form of the individual dependency graphs across the project monorep. We solve this issue by merging all individual analyzer results into one.
Signed-off-by: Rainer Bieniek [email protected]
Please ensure that your pull request adheres to our contribution guidelines. Thank you!