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

Add images table for Workload CVE Image single page #5175

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

dvail
Copy link
Contributor

@dvail dvail commented Mar 9, 2023

Description

Adds initial table support to the workload cve image page.

Does not include:

  • pagination
  • sorting
  • expandable table rows
  • search filter integration

Note: this may be easier to review with GitHub's "Hide whitespace" option enabled due to some component nesting changes that make the diff look larger than it is.

TODO - For some reason this is causing either the getImageDetails or the getImageVulnerabilities graphql query to fire a second time when they both complete the initial requests
This was due to the same image.metadata subresolver in both queries, but with a different field selection in each. Moving the entire image.metadata to the initial query and passing the result down resolves this issue (and clears up a network waterfall).

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

Load an image single page.
image
image

Mobile:
image

Test clicking one of the links in the CVE column:
image

@openshift-ci
Copy link

openshift-ci bot commented Mar 9, 2023

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

@roxbot
Copy link
Contributor

roxbot commented Mar 9, 2023

Images are ready for the commit at 57fa3a3.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.74.x-383-g57fa3a3469.

@dvail dvail force-pushed the dv/ROX-15056-add-table-workload-cve-images-single branch 3 times, most recently from 29fa098 to ce00bf1 Compare March 10, 2023 16:11
<Tbody>
{vulnerabilities.map(
({ cve, severity, isFixable, imageComponents, discoveredAtImage }) => {
const SeverityIcon: React.FC<SVGIconProps> | undefined =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two reasons for the explicit type annotations:

  1. The backend is returning a string for severity here, instead of a GraphQL enum for the severity types. Directly accessing SeverityIcons[severity] silently returns a type of any instead of React.FC.
  2. After investigating the above, I found it is technically possible for the backend to return a severity of UNKNOWN_VULNERABILITY_SEVERITY, which we don't handle on the front end, so it is possible that this component will be undefined and crash the UI if the "unknown" value makes it through somehow.

import { ImageVulnerabilitiesResponse } from './hooks/useImageVulnerabilities';
import { getEntityPagePath } from './searchUtils';

export type SingleEntityVulnerabilitiesTableProps = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This table is almost identical in both the image and deployment single pages, with the exception that the image name is displayed as a column next to image components in the deployment page. The way the gql resolvers are structured make it so that we will need to do some pre-processing of the data on that page before feeding it into this table.

e.g. The query might looks something like

images {
    imageVulnerabilities {
        imageComponents {}
    }
}

so we would need to get the image metadata from the top level and pass it down to the image components to get the UI structure we need.

Mandar is looking into possible alternate queries for this, so I didn't generalize the prop type here too much in order to avoid having to change it a lot later.

@dvail dvail force-pushed the dv/ROX-15056-add-table-workload-cve-images-single branch from ce00bf1 to 86c9ab6 Compare March 10, 2023 16:28
@dvail dvail force-pushed the dv/add-cve-severity-icons branch from 92d7f16 to 4a0f7ad Compare March 13, 2023 15:26
@dvail dvail force-pushed the dv/ROX-15056-add-table-workload-cve-images-single branch from 86c9ab6 to 6b6392d Compare March 13, 2023 15:27
@dvail dvail force-pushed the dv/ROX-15056-add-table-workload-cve-images-single branch 3 times, most recently from e2e729f to 38a80a2 Compare March 13, 2023 16:49
@@ -0,0 +1,57 @@
import { gql } from '@apollo/client';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of the gql queries used in this section we running into the same awkwardness:

  • The query and types are defined in a higher level component that was also in charge of making the request
  • Child components were passed the result data and required access to the response types
  • Both components requiring the type resulted in circular import errors

The two options that came to mind were:

  • Move the type down to the child component, and import both the response type and child component in the parent (awkward, since the child component wasn't making the request)
  • Pull the queries out into new hooks and import the type from there

Copy link
Contributor

Choose a reason for hiding this comment

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

i've been doing the former tbh but pulling out into hooks is probably the cleaner solution here

@dvail dvail force-pushed the dv/add-cve-severity-icons branch from 4a0f7ad to 3ff1f2f Compare March 13, 2023 19:53
@dvail dvail force-pushed the dv/ROX-15056-add-table-workload-cve-images-single branch from 38a80a2 to 7771160 Compare March 13, 2023 19:57
@dvail dvail marked this pull request as ready for review March 13, 2023 20:00
@dvail dvail requested a review from alwayshooin March 13, 2023 20:00
@dvail dvail force-pushed the dv/add-cve-severity-icons branch from 1dce322 to 3a1a704 Compare March 14, 2023 19:12
@dvail dvail force-pushed the dv/ROX-15056-add-table-workload-cve-images-single branch from 7771160 to fc72d3a Compare March 14, 2023 19:13
Base automatically changed from dv/add-cve-severity-icons to master March 14, 2023 20:56
@dvail dvail force-pushed the dv/ROX-15056-add-table-workload-cve-images-single branch from fc72d3a to 57fa3a3 Compare March 14, 2023 20:57
<Tr>
<Th>CVE</Th>
<Th>Severity</Th>
<Th>CVE Status</Th>
Copy link
Contributor

Choose a reason for hiding this comment

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

i think technically PF conventions would have this be CVE status (https://www.patternfly.org/v4/ux-writing/capitalization)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good catch, I'll make this change now in the next PR I'm working since CI is happy with this one as-is.

@@ -0,0 +1,57 @@
import { gql } from '@apollo/client';
Copy link
Contributor

Choose a reason for hiding this comment

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

i've been doing the former tbh but pulling out into hooks is probably the cleaner solution here

@dvail dvail merged commit 04d1c2d into master Mar 15, 2023
@dvail dvail deleted the dv/ROX-15056-add-table-workload-cve-images-single branch March 15, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants