ROX-10987: Splitting Components into Node and Image Components by sachaudh · Pull Request #2163 · stackrox/stackrox

sachaudh · 2022-06-23T00:50:13Z

Description

Added two new entity types: IMAGE_COMPONENT and NODE_COMPONENT
It's difficult to feature flag the entityRelationships.ts because that module doesn't live within the React lifecycle so I'm open to suggestions on how to go about feature flagging it. For now, I commented out the changes so the entity relationships won't be updated right now

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

openshift-ci · 2022-06-23T00:50:16Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ghost · 2022-06-23T01:17:42Z

Tag for build #723355 is 3.70.x-545-gd4e86e6221.

💻 For deploying this image using the dev scripts, run the following first:

export MAIN_IMAGE_TAG='3.70.x-545-gd4e86e6221'

🕹️ A roxctl binary can be downloaded from the CircleCI artifacts.

sachaudh · 2022-06-24T23:12:34Z

ui/apps/platform/src/Containers/VulnMgmt/Dashboard/VulnMgmtDashboardPage.js

+        entityMenuTypes = [...baseEntityMenuTypes, ...splitComponentMenuTypes];
+    } else {
+        entityMenuTypes = [...baseEntityMenuTypes, ...componentMenuType];
+    }


Feature flagged the entities we show in the VM dashboard menu dropdown

sachaudh · 2022-06-24T23:14:37Z

ui/apps/platform/src/Containers/VulnMgmt/List/Images/VulnMgmtListImages.js


-export function getCurriedImageTableColumns(watchedImagesTrigger) {
+export function getCurriedImageTableColumns(watchedImagesTrigger, isFeatureFlagEnabled) {
+    const isFrontendVMUpdatesEnabled = isFeatureFlagEnabled('ROX_FRONTEND_VM_UDPATES');


Feature flagging the count link for components

sachaudh · 2022-06-24T23:19:12Z

ui/apps/platform/src/utils/entityRelationships.ts

    },
    [entityTypes.NODE]: {
+        // @TODO: Uncomment this once we're using the new entity
+        // children: [entityTypes.NODE_COMPONENT],


Open to suggestions on how to feature flag the following in entityRelationships. Since it's a separate module and outside the React lifecycle, I can't use the feature flag hook here

As long as we feature flag the links, it may not be worth feature-flagging the direct "sub-routes" built from this map. (Re-factoring this map everywhere it is used would be a risky change.)

ghost · 2022-06-28T19:14:09Z

Images are ready for the commit at d4e86e6.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.70.x-546-g4aaee764e0.

vjwilson

A good foundation.
A couple of places I think we should revisit as we integrate the split CVE types, and one stylistic suggestion for tile sidebar width.

Regardless, let's merge once tests pass.

vjwilson · 2022-06-28T18:50:09Z

ui/apps/platform/src/Containers/VulnMgmt/Entity/Component/VulnMgmtComponentOverview.js

+    const currentEntity = { [currentEntityType]: id };
    const newEntityContext = { ...entityContext, ...currentEntity };

+    const entityGridContainerClassName = `${entityGridContainerBaseClassName} grid-columns-1 md:grid-columns-2 lg:grid-columns-2`;


Why did you switch to the other exported class and re-concatenate?

So this is the before and after of this change

Before:

After:

I noticed that we were only showing two columns in the component overview so it differed from the class we were using in general, which was for 3 columns. So this change will instead use the base class and then append specific classes for this view

^ That's probably a side-effect of widening the tiles right sidebar.

I'm fine with your fix for it, but I'm afraid this may turn into whack-a-mole. Let's get UX suggestion on this. cc: @alanonthegit

Oh no, that wasn't the issue. We're using grid columns for the overview and it was using a 3 column grid before. If you look at the first image you'll see it looks like you can add another third element to the right. It was on master as well. It's not really related to splitting the components but I just did it anyway.

vjwilson · 2022-06-28T20:36:38Z

ui/apps/platform/src/Containers/VulnMgmt/Entity/RelatedEntitiesSideList.js

    return (
        <div
-            className={` h-full relative border-base-100 border-l w-32 ${
+            className={` h-full relative border-base-100 border-l w-43 ${


Rather than making the right tile bar wider, would it be better to make long entity names wrap to two lines?

Hmm, yea that's true. This probably wasn't the best solution. I could do two things:

Remove the width restriction and add a max-width and wrapping

Keep the width restriction and just add wrapping

I'll try #2 which is what you're suggesting.

Seems like text wrapping does look a bit odd (at least to me)

I ended up doing #1 instead. So I'll remove the w-32, add max-w-43, and add a text wrap class in case the text might be longer than that

vjwilson · 2022-06-28T20:40:35Z

ui/apps/platform/src/Containers/VulnMgmt/Entity/TableWidgetFixableCves.js

+    let queryVulnCounterFieldName = 'vulnCounter';
+    let queryVulnsFieldName = 'vulns';
+    let queryCVEFieldsName = 'cveFields';
+    let queryFragment = VULN_CVE_LIST_FRAGMENT;


I was also experimenting with this file, because we will have to show all three kinds of fixable CVEs on the Cluster Overview, and choose the correct type for the other entities. We probably should have broken this down across both of these features.

For now, I'll pause my branch, until we get this version merged in, and then expand on this approach.

vjwilson · 2022-06-28T22:12:51Z

ui/apps/platform/src/Containers/VulnMgmt/List/NodeComponents/VulnMgmtListNodeComponents.js

+const VulnMgmtNodeComponents = ({ selectedRowId, search, sort, page, data, totalResults }) => {
+    const query = gql`
+        query getComponents($query: String, $pagination: Pagination) {
+            results: nodeComponents(query: $query, pagination: $pagination) {
+                ...nodeComponentFields
+            }
+            count: nodeComponentCount(query: $query)
+        }
+        ${VULN_NODE_COMPONENT_LIST_FRAGMENT}
+    `;


I wonder if it is worth doing 2 different sets of files at this "container" level. We should compare the complexity of the single CVE list file with conditionals for the three new types of CVEs, and decide which way is better. Then we made them consistent one way or the other.

We can try to leave this as-is for now, and come back tomorrow and make refactors so it's more consistent with your changes

vjwilson · 2022-06-28T22:15:25Z

ui/apps/platform/src/Containers/VulnMgmt/VulnMgmt.fragments.js


 export const IMAGE_CVE_LIST_FRAGMENT = gql`
-    fragment cveFields on ImageVulnerability {
+    fragment imageCVEFields on ImageVulnerability {


I think we need to use this new name in the file ui/apps/platform/src/Containers/VulnMgmt/List/Cves/VulnMgmtListCves.js above, at line 321.

Oh, good catch

vjwilson · 2022-06-28T22:22:21Z

ui/apps/platform/src/utils/entityRelationships.ts

    },
    [entityTypes.NODE]: {
+        // @TODO: Uncomment this once we're using the new entity
+        // children: [entityTypes.NODE_COMPONENT],


As long as we feature flag the links, it may not be worth feature-flagging the direct "sub-routes" built from this map. (Re-factoring this map everywhere it is used would be a risky change.)

sachaudh · 2022-06-30T17:28:50Z

@vjwilson I think the failed ui-unit-tests aren't related to this PR. Maybe @dvail you can let me know in case it is, since that component might be related to your recent changes

dvail · 2022-06-30T17:35:56Z

@sachaudh You're right, this is related to my stuff on not this PR. Investigating why this passed on my PR branch and not master now... :/

sachaudh · 2022-06-30T17:58:11Z

@sachaudh You're right, this is related to my stuff on not this PR. Investigating why this passed on my PR branch and not master now... :/

Ah, yea let me know. I don't think I can push my changes in until it's fixed. I'll try re-running

openshift-ci bot added the do-not-merge/work-in-progress label Jun 23, 2022

github-actions bot added the area/ui label Jun 23, 2022

sachaudh force-pushed the ROX-10987 branch from ffdeb44 to bf92958 Compare June 24, 2022 23:09

sachaudh changed the base branch from master to saif/vm-updates-feature-flag June 24, 2022 23:10

sachaudh commented Jun 24, 2022

View reviewed changes

sachaudh requested a review from vjwilson June 24, 2022 23:19

sachaudh force-pushed the ROX-10987 branch 2 times, most recently from ce55387 to ce1d820 Compare June 24, 2022 23:25

sachaudh force-pushed the saif/vm-updates-feature-flag branch from 6cddd70 to efdce6e Compare June 27, 2022 19:03

sachaudh force-pushed the ROX-10987 branch from ce1d820 to 80118b0 Compare June 27, 2022 19:04

sachaudh marked this pull request as ready for review June 27, 2022 19:05

openshift-ci bot removed the do-not-merge/work-in-progress label Jun 27, 2022

sachaudh force-pushed the saif/vm-updates-feature-flag branch from efdce6e to 48f536d Compare June 27, 2022 22:33

sachaudh requested a review from a team June 27, 2022 22:33

sachaudh force-pushed the ROX-10987 branch from 80118b0 to d5824cb Compare June 27, 2022 22:34

Base automatically changed from saif/vm-updates-feature-flag to master June 28, 2022 18:08

sachaudh force-pushed the ROX-10987 branch from d5824cb to 084c691 Compare June 28, 2022 18:26

vjwilson approved these changes Jun 28, 2022

View reviewed changes

sachaudh requested review from charmik-redhat, keyallis and md2119 June 30, 2022 17:33

sachaudh enabled auto-merge (squash) June 30, 2022 17:56

sachaudh disabled auto-merge June 30, 2022 17:56

ROX-10987: Splitting Components into Node and Image Components

34d5c79

sachaudh added 6 commits June 30, 2022 10:58

fixes

1a497df

feature flagging

63fff80

fixing tests

5ac4a39

fixes after rebase...will need to refactor stuff

0efb207

fix: addressing comments

ac599c7

fix: addressing comments

d4e86e6

sachaudh force-pushed the ROX-10987 branch from a02c7d8 to d4e86e6 Compare June 30, 2022 17:58

sachaudh merged commit bf1e5e5 into master Jun 30, 2022

sachaudh deleted the ROX-10987 branch June 30, 2022 18:56

Conversation

sachaudh commented Jun 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

openshift-ci bot commented Jun 23, 2022

Uh oh!

ghost commented Jun 23, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 28, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vjwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachaudh Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachaudh Jun 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachaudh commented Jun 30, 2022

Uh oh!

dvail commented Jun 30, 2022

Uh oh!

sachaudh commented Jun 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sachaudh commented Jun 23, 2022 •

edited

Loading

ghost commented Jun 23, 2022 •

edited by ghost

Loading

ghost commented Jun 28, 2022 •

edited by ghost

Loading

sachaudh Jun 30, 2022 •

edited

Loading

sachaudh Jun 30, 2022 •

edited

Loading