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

Cherry-Pick: Optimized software versions endpoint #24573

Merged

Conversation

ksykulev
Copy link
Contributor

@ksykulev ksykulev commented Dec 9, 2024

#23679

Original PR: #24496

The software versions endpoint cve details can be truncated using the
`without_vulnerability_details` flag.

#23679

# 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
- [x] Manual QA for all new/changed functionality

---------

Co-authored-by: Ian Littman <iansltx@gmail.com>
@ksykulev ksykulev requested review from a team as code owners December 9, 2024 22:26
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (rc-minor-fleet-v4.61.0@6356bb4). Learn more about missing BASE report.

Additional details and impacted files
@@                    Coverage Diff                    @@
##             rc-minor-fleet-v4.61.0   #24573   +/-   ##
=========================================================
  Coverage                          ?   63.56%           
=========================================================
  Files                             ?     1591           
  Lines                             ?   150969           
  Branches                          ?     3883           
=========================================================
  Hits                              ?    95958           
  Misses                            ?    47363           
  Partials                          ?     7648           
Flag Coverage Δ
backend 64.36% <100.00%> (?)
frontend 53.54% <ø> (?)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jacobshandling
Copy link
Contributor

jacobshandling commented Dec 9, 2024

@ksykulev can you please reference the associated PR to main somewhere in title/description, so it's easy to compare the PRs?

@jacobshandling
Copy link
Contributor

Or connect them

@@ -125,6 +125,7 @@ const SoftwareTitles = ({
teamId,
addedSoftwareToken,
...vulnFilters,
...(showVersions ? { without_vulnerability_details: true } : {}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking (I see this is the cherry pick to RC) but out of curiosity – why not something like withoutVulnDetails: showVersions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to exclude the entire query string parameter when it is not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nice, right. Didn't read below to see that this is referencing the query key directly for the service method arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I probably should have gotten a ✅ from someone on front end on the original PR before merging. I will make sure to do that next time there are any react changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually now that we're on the subject..withoutVulnerabilityDetails: true would be better, since getSoftwareVersions uses convertParamsToSnakeCase to process all of these field names accordingly anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to keep me honest here:
Screenshot 2024-12-10 at 11 54 05 AM

convertParamsToSnakeCase does indeed working properly with already snake cased keys.

@ksykulev ksykulev merged commit edf9662 into rc-minor-fleet-v4.61.0 Dec 9, 2024
24 checks passed
@ksykulev ksykulev deleted the 23679-software-versions-cherry-pick branch December 9, 2024 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants