ROX-31173: do not embed component in resolver context#17154
ROX-31173: do not embed component in resolver context#17154dashrews78 merged 14 commits intomasterfrom
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at 502f883. To use with deploy scripts, first |
02548fc to
75928a0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17154 +/- ##
==========================================
+ Coverage 48.86% 48.87% +0.01%
==========================================
Files 2720 2720
Lines 203374 203193 -181
==========================================
- Hits 99371 99305 -66
+ Misses 96178 96093 -85
+ Partials 7825 7795 -30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `central/graphql/resolvers/image_scan_test.go:106-110` </location>
<code_context>
return nil, err
}
if features.FlattenCVEData.Enabled() {
- return getImageComponentV2Resolvers(resolver.ctx, resolver.root, resolver.data, query)
+ return resolver.root.ImageComponents(resolver.ctx, args)
}
return getImageComponentResolvers(resolver.ctx, resolver.root, resolver.data, query)
}
</code_context>
<issue_to_address>
**suggestion (testing):** Missing negative test for FlattenCVEData feature flag disabled.
Please add a test case with FlattenCVEData disabled to verify correct behavior when the feature flag is off.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c58a45a to
718d07d
Compare
3e0ae95 to
a8e3bf3
Compare
a8e3bf3 to
4cda588
Compare
charmik-redhat
left a comment
There was a problem hiding this comment.
Backend looks good. Wait for someone on the UI to approve too
dvail
left a comment
There was a problem hiding this comment.
UI side LGTM.
One comment that would be nice to change, but not super pressing due to it being in a deprecated section.
ui/apps/platform/src/Containers/VulnMgmt/Entity/Image/VulnMgmtImageOverview.jsx
Show resolved
Hide resolved
4cda588 to
3bbf294
Compare
|
@dashrews78 The gke-ui-e2e-tests failures are almost definitely related to this. If you check out the video you can see that the images page in config mgmt is crashing. |
|
/test gke-upgrade-tests |
Description
Embedding the the EmbeddedScan Object into the image scan resolver caused us to have to convert between EmbeddedScanComponent objects and ImageComponent Objects needlessly. This is why we had to have the ID to get a deterministic ID. Without that requirement we can eliminate the hashing function in #17200. The reason it was embedded in this resolver to begin with was for performance, but that performance tradeoff is too much and was rarely used. Additionally the usage of that resolver in general is to populate deprecated dashboards and thus we don't get much value by caching that data.
Provided the UI and testing updates to no longer use the previously deprecated imageScan->Components and imageScan->components->vulns in favor of the ones in the new model.
Cursor assisted with the UI changes.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Worked through and openshift cluster making sure hopping the various screens worked. Also updated tests and such.