ROX-33571: Consolidate container display to shared components#19512
ROX-33571: Consolidate container display to shared components#19512dvail wants to merge 1 commit intodv/ROX-33571-flatten-nested-expandablesfrom
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
SecurityContextCard, the filter usescontainer?.securityContext?.addCapabilities.length/dropCapabilities.length, which will throw ifsecurityContextexists but those arrays areundefined; consider guarding with optional chaining on the array itself (e.g.container.securityContext?.addCapabilities?.length) or defaulting to an empty array. - Risk
DeploymentDetailsno longer renders any security context at all (the<SecurityContext>usage was removed andSecurityContextCardis 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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 ( |
There was a problem hiding this comment.
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.
|
Images are ready for the commit at 4fdb017. To use with deploy scripts, first |
b9617b8 to
e1376fb
Compare
4fdb017 to
df2f821
Compare
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
e1376fb to
6592f45
Compare
df2f821 to
5095833
Compare
Description
Consolidates three separate container display implementations into shared PatternFly 6 components:
New shared components:
DeploymentContainersCard- Wrapper for displaying multiple containersSecurityContextCard- Filtered security context displayMigrations completed:
Results:
All three original locations (Violations, Risk, Network Graph) now share the same components.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Part of ROX-33571 container display consolidation stack.
Depends on: #19511