-
Notifications
You must be signed in to change notification settings - Fork 447
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
Fixed bug when using without_vulnerability_details
and vulnerability filters
#24769
Conversation
a0c0683
to
4a88f50
Compare
4a88f50
to
3e75532
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24769 +/- ##
==========================================
+ Coverage 54.28% 63.57% +9.29%
==========================================
Files 1601 1601
Lines 151676 151716 +40
Branches 3952 3952
==========================================
+ Hits 82334 96451 +14117
+ Misses 62544 47591 -14953
- Partials 6798 7674 +876
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
3e75532
to
50b0df4
Compare
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.
Need to wait for Ci to run to confirm we still disallow vuln details for Fleet Free even when without_vulnerability_details=false
. Otherwise see nits.
@@ -0,0 +1 @@ | |||
* Fixed bug when using the `without_vulnerability_details` param along with vulnerability filters in fleet premium. |
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.
We don't do changes files for unreleased bugs
@@ -917,7 +917,7 @@ func listSoftwareDB( | |||
DetailsLink: fmt.Sprintf("https://nvd.nist.gov/vuln/detail/%s", cveID), | |||
CreatedAt: *result.CreatedAt, | |||
} | |||
if opts.IncludeCVEScores { | |||
if opts.IncludeCVEScores && !opts.WithoutVulnerabilityDetails { |
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.
Do we need !without here still?
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.
Unfortunately we do, because when we are filtering with a vulnerability opts.IncludeCVEScores
will be true
, and the json response will be large. However, if we put !opts.WithoutVulnerabilityDetails
, it will add all the joins, but the json it has to serialize will still be the smaller version.
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.
Got it; thx for the clarification!
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.
LGTM. Free to merge as soon as CI is complete/green, and tag me when you have the cherry-pick up.
…y filters (#24769) #24765 # Checklist for submitter If some of the following don't apply, delete the relevant line. <!-- Note that API documentation changes are now addressed by the product design team. --> - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files) for more information. - [x] Added/updated tests
#24765
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/
,orbit/changes/
oree/fleetd-chrome/changes
.See Changes files for more information.