Skip to content

ROX-33571: Consolidate container display to shared components#19512

Draft
dvail wants to merge 1 commit intodv/ROX-33571-flatten-nested-expandablesfrom
dv/ROX-33571-consolidate-container-display
Draft

ROX-33571: Consolidate container display to shared components#19512
dvail wants to merge 1 commit intodv/ROX-33571-flatten-nested-expandablesfrom
dv/ROX-33571-consolidate-container-display

Conversation

@dvail
Copy link
Contributor

@dvail dvail commented Mar 19, 2026

Description

Consolidates three separate container display implementations into shared PatternFly 6 components:

New shared components:

  • DeploymentContainersCard - Wrapper for displaying multiple containers
  • SecurityContextCard - Filtered security context display

Migrations completed:

  • Violations container configuration → shared components
  • Violations security context → shared components
  • Risk container configuration → shared components
  • Risk security context → shared components

Results:

  • Removes 5 Violations-specific components (189 lines deleted)
  • Removes custom Risk implementations (227 lines deleted)
  • Eliminates Tailwind classes from container components
  • Standardizes on PatternFly 6 throughout
  • Fixes snake_case → camelCase in Risk security context fields
  • Net code reduction: -327 lines

All three original locations (Violations, Risk, Network Graph) now share the same components.

User-facing documentation

  • CHANGELOG.md is updated OR update is not needed - Internal refactor, no user-facing changes
  • documentation PR is created and is linked above OR is not needed - no docs needed

Testing and quality

  • the change is production ready: the change is GA, or otherwise the functionality is gated by a feature flag - refactor only
  • CI results are inspected

Automated testing

  • added unit tests - tests added in follow-up PR
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

  • Manually tested all three contexts (Violations, Risk, Network Graph)
  • Verified all container information displays correctly in each context
  • Confirmed image links work with platform vs user workload paths
  • Checked security context filtering works properly
  • Verified null deployment handling in Violations
  • Confirmed Risk components render without Tailwind wrapper styles
  • All existing functionality preserved

Part of ROX-33571 container display consolidation stack.
Depends on: #19511

@dvail
Copy link
Contributor Author

dvail commented Mar 19, 2026

@openshift-ci
Copy link

openshift-ci bot commented Mar 19, 2026

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

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • In SecurityContextCard, the filter uses container?.securityContext?.addCapabilities.length / dropCapabilities.length, which will throw if securityContext exists but those arrays are undefined; consider guarding with optional chaining on the array itself (e.g. container.securityContext?.addCapabilities?.length) or defaulting to an empty array.
  • Risk DeploymentDetails no longer renders any security context at all (the <SecurityContext> usage was removed and SecurityContextCard is only wired into Violations); double-check whether Risk is supposed to still display container security context and, if so, wire the shared component in there as well.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `SecurityContextCard`, the filter uses `container?.securityContext?.addCapabilities.length` / `dropCapabilities.length`, which will throw if `securityContext` exists but those arrays are `undefined`; consider guarding with optional chaining on the array itself (e.g. `container.securityContext?.addCapabilities?.length`) or defaulting to an empty array.
- Risk `DeploymentDetails` no longer renders any security context at all (the `<SecurityContext>` usage was removed and `SecurityContextCard` is only wired into Violations); double-check whether Risk is supposed to still display container security context and, if so, wire the shared component in there as well.

## Individual Comments

### Comment 1
<location path="ui/apps/platform/src/Containers/Violations/Details/Deployment/ContainerConfiguration.tsx" line_range="19-20" />
<code_context>
-    let content: JSX.Element[] | string = 'None';
+    const getImageUrl = (imageId: string) => `${vulnMgmtBasePath}/images/${imageId}`;

     if (deployment === null) {
-        content =
-            'Container configurations are unavailable because the alert’s deployment no longer exists.';
-    } else if (deployment.containers.length !== 0) {
-        content = deployment.containers.map((container, i) => (
-            <Fragment key={container.id}>
-                <Title headingLevel="h4" className="pf-v6-u-mb-md">{`containers[${i}]`}</Title>
-                <ContainerConfigurationDescriptionList
-                    key={container.id}
-                    container={container}
-                    vulnMgmtBasePath={vulnMgmtBasePath}
-                />
-            </Fragment>
-        ));
+        return (
+            <Card>
+                <CardTitle component="h3">Container configuration</CardTitle>
</code_context>
<issue_to_address>
**question:** Consider preserving an explicit "None" state when there are zero containers.

With the previous implementation, the card always rendered and showed either a “deployment missing” message or “None” when there were no containers. With `DeploymentContainersCard`, when `deployment` is non-null but `deployment.containers` is empty/undefined, nothing is rendered because the component returns `null`. If you want to keep users informed in the empty case, consider retaining a small wrapper card here that shows an explicit “None” (or similar) when there are zero containers, and only delegating to `DeploymentContainersCard` when there is at least one container.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 19 to +20
if (deployment === null) {
content =
'Container configurations are unavailable because the alert’s deployment no longer exists.';
} else if (deployment.containers.length !== 0) {
content = deployment.containers.map((container, i) => (
<Fragment key={container.id}>
<Title headingLevel="h4" className="pf-v6-u-mb-md">{`containers[${i}]`}</Title>
<ContainerConfigurationDescriptionList
key={container.id}
container={container}
vulnMgmtBasePath={vulnMgmtBasePath}
/>
</Fragment>
));
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Consider preserving an explicit "None" state when there are zero containers.

With the previous implementation, the card always rendered and showed either a “deployment missing” message or “None” when there were no containers. With DeploymentContainersCard, when deployment is non-null but deployment.containers is empty/undefined, nothing is rendered because the component returns null. If you want to keep users informed in the empty case, consider retaining a small wrapper card here that shows an explicit “None” (or similar) when there are zero containers, and only delegating to DeploymentContainersCard when there is at least one container.

@rhacs-bot
Copy link
Contributor

Images are ready for the commit at 4fdb017.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.11.x-392-g4fdb017dde.

@dvail dvail force-pushed the dv/ROX-33571-flatten-nested-expandables branch from b9617b8 to e1376fb Compare March 19, 2026 21:00
@dvail dvail force-pushed the dv/ROX-33571-consolidate-container-display branch from 4fdb017 to df2f821 Compare March 19, 2026 21:00
Create DeploymentContainersCard wrapper for container display

Migrate Violations container config to shared component

Create SecurityContextCard for filtered security context display

Migrate Violations security context to shared component

Migrate Risk container config to shared PF6 component

Migrate Risk security context to shared PF6 component

Fix ESLint: use ?? instead of ||

Add component tests for container display components

Remove low-value tests, keep filtering logic test

Simplify test mocks with type casting

Fix type assertions with unknown cast

Add missing jest-dom import
@dvail dvail force-pushed the dv/ROX-33571-flatten-nested-expandables branch from e1376fb to 6592f45 Compare March 19, 2026 21:05
@dvail dvail force-pushed the dv/ROX-33571-consolidate-container-display branch from df2f821 to 5095833 Compare March 19, 2026 21:06
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.

2 participants