Skip to content
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

Fix vulnerability count #7196

Merged
merged 2 commits into from
Jun 26, 2023
Merged

Fix vulnerability count #7196

merged 2 commits into from
Jun 26, 2023

Conversation

mnonnenmacher
Copy link
Member

Please see the commit messages for details.

@mnonnenmacher mnonnenmacher requested a review from a team as a code owner June 24, 2023 16:45
@mnonnenmacher mnonnenmacher enabled auto-merge (rebase) June 24, 2023 16:46
@codecov
Copy link

codecov bot commented Jun 24, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (39013ad) 61.50% compared to head (ce15cc8) 61.50%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7196   +/-   ##
=========================================
  Coverage     61.50%   61.50%           
  Complexity     1965     1965           
=========================================
  Files           333      333           
  Lines         16495    16495           
  Branches       2359     2359           
=========================================
  Hits          10146    10146           
  Misses         5372     5372           
  Partials        977      977           
Flag Coverage Δ
funTest-docker 69.24% <ø> (ø)
funTest-non-docker 31.91% <ø> (ø)
test 37.50% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

The map returned by `getVulnerabilities()` contains entries for all
packages, also those that do not have any vulnerabilities. Also, the
previous calculation also counted excluded packages which was not
consistent with the total package count which ignores excluded packages.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
Print the vulnerability count only for included packages to be
consistent with the printed package count.

Signed-off-by: Martin Nonnenmacher <martin.nonnenmacher@bosch.io>
@@ -140,12 +140,16 @@ class AdvisorCommand : OrtCommand(
println("The advice took $duration.")

with(advisorRun.results.getVulnerabilities()) {
val totalPackageCount = ortResultOutput.getPackages(omitExcluded = true).size
val includedPackages = ortResultOutput.getPackages(omitExcluded = true).map { it.metadata.id }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map returned by getVulnerabilities() contains entries for all
packages, also those that do not have any vulnerabilities.

BTW, this is something that also occurred to me in the context of #6613: The advisor's retrievePackageFindings() API definition does not make clear whether the returned map should contain entries for packages that have empty defects and vulnerabilities as part of the AdvisorResult. Should it? Just to get the AdvisorDetails and AdvisorSummary returned?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this as well, in some situations like package curations we decided that it has benefits to have explicit empty results to document that something was requested.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed as part of the Kotlin developer meeting.

@mnonnenmacher mnonnenmacher merged commit ce007ec into main Jun 26, 2023
@mnonnenmacher mnonnenmacher deleted the fix-vulnerability-count branch June 26, 2023 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants