-
Notifications
You must be signed in to change notification settings - Fork 308
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
Helper CLI function for merging muliple analyzer results into one #5317
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 <extern.rainer.bieniek@porsche.de>
@porsche-rbieniek can you please elaborate how this PR relates to #4698? |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put this in alphabetical order.
@@ -0,0 +1,291 @@ | |||
/* | |||
* Copyright (C) 2021 Porsche AG |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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( |
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.
Typo in leftExlcudes
.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point taken. The excludes will not be merged then
packageLicenseChoices = ( | ||
leftChoices.packageLicenseChoices | ||
+ rightChoices.packageLicenseChoices | ||
).distinct() |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <extern.rainer.bieniek@porsche.de>
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 extern.rainer.bieniek@porsche.de
Please ensure that your pull request adheres to our contribution guidelines. Thank you!