CONSOLE-5315: Get Node inventory card items from extensions#16582
Open
jeff-phillips-18 wants to merge 1 commit into
Open
CONSOLE-5315: Get Node inventory card items from extensions#16582jeff-phillips-18 wants to merge 1 commit into
jeff-phillips-18 wants to merge 1 commit into
Conversation
Contributor
|
@jeff-phillips-18: This pull request references CONSOLE-5315 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.23" instead. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
@jeff-phillips-18: GitHub didn't allow me to assign the following users: rh-joshbeverly. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Contributor
WalkthroughRefactors Node inventory to load items from a new ChangesNode Inventory Extension System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx (1)
55-69: ⚡ Quick winInconsistent StackItem wrapping across inventory components.
NodeImagesInventoryItemwraps its content inStackItem, butNodePodInventoryItem(lines 20-53) returnsResourceInventoryItemdirectly without aStackItemwrapper. This inconsistency may cause layout differences.For consistency, either:
- Both components should wrap in
StackItem, or- Neither should wrap (let the parent Stack handle layout), or
- The map in the render (line 118) should wrap each item in
StackItem♻️ Recommended fix: Remove StackItem wrapper for consistency
const NodeImagesInventoryItem: ComponentType<{ obj: NodeKind }> = ({ obj }) => { const { t } = useTranslation(); return ( - <StackItem> - <InventoryItem - isLoading={!obj} - title={t('console-app~Image')} - titlePlural={t('console-app~Images')} - count={obj.status?.images?.length} - error={!obj.status?.images} - /> - </StackItem> + <InventoryItem + isLoading={!obj} + title={t('console-app~Image')} + titlePlural={t('console-app~Images')} + count={obj.status?.images?.length} + error={!obj.status?.images} + /> ); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx` around lines 55 - 69, NodeImagesInventoryItem is wrapped in a StackItem while NodePodInventoryItem returns ResourceInventoryItem directly, causing layout inconsistency; remove the StackItem wrapper from NodeImagesInventoryItem so it returns InventoryItem directly (matching NodePodInventoryItem), keeping the same props (isLoading, title, titlePlural, count, error) and leaving imports/usage of InventoryItem, ResourceInventoryItem, NodeImagesInventoryItem and NodePodInventoryItem intact.frontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsx (1)
86-107: ⚡ Quick winUnify the
isLoadingprop across all inventory items.Line 88 uses
isLoading={!loaded}, while lines 96 and 104 useisLoading={!obj}. This inconsistency can cause visual glitches whenloadedis true butobjis still undefined (e.g., no matching BareMetalHost found).♻️ Standardize loading state
<InventoryItem title={t('metal3-plugin~NIC')} - isLoading={!obj} + isLoading={!loaded} count={getHostNICs(obj).length} TitleComponent={NICTitleComponent} /> </StackItem> <StackItem> <InventoryItem title={t('metal3-plugin~CPU')} - isLoading={!obj} + isLoading={!loaded} count={getHostCPU(obj).count} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsx` around lines 86 - 107, InventoryItem components use inconsistent loading checks: Disk uses isLoading={!loaded} while NIC and CPU use isLoading={!obj}; change all InventoryItem isLoading props (the Disk, NIC, and CPU InventoryItem instances that use DiskTitleComponent, NICTitleComponent and getHostStorage/getHostNICs/getHostCPU) to the same unified condition—preferably isLoading={!loaded || !obj}—so loading state is consistent when either overall data isn't loaded or the specific obj is missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsx`:
- Around line 55-110: BareMetalHostInventoryItems currently calls
getNamespace(obj), getName(obj), getHostStorage(obj), getHostNICs(obj), and
getHostCPU(obj) without guarding against obj being undefined; update
BareMetalHostInventoryItems to early-return null (or a loading/empty state) when
obj is undefined or otherwise provide safe fallback values for
hostName/namespace and counts (e.g., empty arrays and zero) before using
NICTitleComponent, DiskTitleComponent, getHostStorage, getHostNICs, and
getHostCPU so no runtime access occurs on an undefined obj.
In `@frontend/packages/metal3-plugin/src/hooks/useAccessibleResources.ts`:
- Around line 43-49: The initResources builder may spread an undefined
initResource which yields invalid WatchK8sResource objects; update the
useAccessibleResources hook to validate initResource before using it (inside the
initResources useMemo) — either return an empty WatchK8sResources object or
throw a clear error if initResource is falsy, or make initResource required in
the hook signature; ensure the logic around projectsNames and the created
resources[namespace] entries only runs when initResource is defined so each
entry includes required fields like groupVersionKind.
- Around line 38-41: projectsNames computed in the useMemo can be undefined when
projectsLoaded is true but projectsData is undefined, causing a forEach runtime
error; update the useMemo (projectsNames) to ensure it always yields an array by
using a nullish fallback around the map result (e.g., use
projectsData?.map(getName) ?? [] or a conditional projectsData ?
projectsData.map(getName) : []) and keep the same dependencies (isAdmin,
projectsData, projectsLoaded) so subsequent code that calls forEach on
projectsNames is safe.
---
Nitpick comments:
In
`@frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx`:
- Around line 55-69: NodeImagesInventoryItem is wrapped in a StackItem while
NodePodInventoryItem returns ResourceInventoryItem directly, causing layout
inconsistency; remove the StackItem wrapper from NodeImagesInventoryItem so it
returns InventoryItem directly (matching NodePodInventoryItem), keeping the same
props (isLoading, title, titlePlural, count, error) and leaving imports/usage of
InventoryItem, ResourceInventoryItem, NodeImagesInventoryItem and
NodePodInventoryItem intact.
In
`@frontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsx`:
- Around line 86-107: InventoryItem components use inconsistent loading checks:
Disk uses isLoading={!loaded} while NIC and CPU use isLoading={!obj}; change all
InventoryItem isLoading props (the Disk, NIC, and CPU InventoryItem instances
that use DiskTitleComponent, NICTitleComponent and
getHostStorage/getHostNICs/getHostCPU) to the same unified condition—preferably
isLoading={!loaded || !obj}—so loading state is consistent when either overall
data isn't loaded or the specific obj is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7281cc07-991e-4730-a085-1fd4bc9c9f2e
📒 Files selected for processing (14)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/VirtualMachinesInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/BareMetalInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/InventoryCard.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/VirtualMachinesInventoryItems.spec.tsxfrontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.mdfrontend/packages/console-dynamic-plugin-sdk/src/extensions/node.tsfrontend/packages/console-dynamic-plugin-sdk/src/schema/console-extensions.tsfrontend/packages/metal3-plugin/console-extensions.jsonfrontend/packages/metal3-plugin/package.jsonfrontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsxfrontend/packages/metal3-plugin/src/hooks/useAccessibleResources.ts
💤 Files with no reviewable changes (4)
- frontend/packages/console-app/src/components/nodes/node-dashboard/VirtualMachinesInventoryItems.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/VirtualMachinesInventoryItems.spec.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/BareMetalInventoryItems.spec.tsx
04f2638 to
3fdfb21
Compare
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jeff-phillips-18 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Member
Author
|
/retest |
1 similar comment
Member
Author
|
/retest |
3fdfb21 to
f80360f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsx (1)
105-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent loading state for CPU inventory item.
The CPU inventory item uses
isLoading={!obj}(line 108) while Disk and NIC useisLoading={!loaded}(lines 92, 100). Sinceobjis already guarded to be non-null after the previous fix, all three should use the consistentloadedflag that reflects when both BareMetalHost and Machine resources have finished loading.🔧 Proposed fix
<InventoryItem title={t('metal3-plugin~CPU')} - isLoading={!obj} + isLoading={!loaded} count={getHostCPU(obj).count} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsx` around lines 105 - 110, The CPU InventoryItem uses isLoading={!obj} while Disk/NIC use isLoading={!loaded}, causing inconsistent loading state; update the CPU item in BareMetalHostInventoryItemsForNode.tsx to use isLoading={!loaded} instead of isLoading={!obj}, keeping the getHostCPU(obj).count call as-is (getHostCPU, InventoryItem, obj, and loaded are the relevant symbols) so the CPU, Disk, and NIC items share the same loaded flag that reflects both BareMetalHost and Machine resource readiness.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.md`:
- Line 63: Update the link fragment for the [console.tab/horizontalNav] entry so
it exactly matches the existing heading anchor: replace the camelCase fragment
"`#consoletabhorizontalNav`" with the all-lowercase "`#consoletabhorizontalnav`" to
satisfy the linter's exact-match requirement and ensure the fragment points to
the heading at the doc's anchor.
In
`@frontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsx`:
- Around line 55-85: Move the early guard for obj to before any selectors are
called: in the BareMetalHostInventoryItems functional component, check if (!obj)
return null at the top of the function body and only after that call
getNamespace(obj) and getName(obj); this ensures getNamespace/getName (used to
compute namespace, hostName and captured by NICTitleComponent and
DiskTitleComponent useCallbacks) are not invoked with undefined.
---
Duplicate comments:
In
`@frontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsx`:
- Around line 105-110: The CPU InventoryItem uses isLoading={!obj} while
Disk/NIC use isLoading={!loaded}, causing inconsistent loading state; update the
CPU item in BareMetalHostInventoryItemsForNode.tsx to use isLoading={!loaded}
instead of isLoading={!obj}, keeping the getHostCPU(obj).count call as-is
(getHostCPU, InventoryItem, obj, and loaded are the relevant symbols) so the
CPU, Disk, and NIC items share the same loaded flag that reflects both
BareMetalHost and Machine resource readiness.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b264aab9-57e2-4429-aef2-a1647373c23e
📒 Files selected for processing (16)
frontend/packages/console-app/locales/en/console-app.jsonfrontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/VirtualMachinesInventoryItems.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/BareMetalInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/InventoryCard.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/VirtualMachinesInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/utils/NodeBareMetalUtils.tsfrontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.tsfrontend/packages/console-dynamic-plugin-sdk/docs/console-extensions.mdfrontend/packages/console-dynamic-plugin-sdk/src/extensions/node.tsfrontend/packages/console-dynamic-plugin-sdk/src/schema/console-extensions.tsfrontend/packages/metal3-plugin/console-extensions.jsonfrontend/packages/metal3-plugin/package.jsonfrontend/packages/metal3-plugin/src/components/baremetal-nodes/BareMetalHostInventoryItemsForNode.tsxfrontend/packages/metal3-plugin/src/hooks/useAccessibleResources.ts
💤 Files with no reviewable changes (5)
- frontend/packages/console-app/src/components/nodes/node-dashboard/BareMetalInventoryItems.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/BareMetalInventoryItems.spec.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/VirtualMachinesInventoryItems.tsx
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/VirtualMachinesInventoryItems.spec.tsx
- frontend/packages/console-app/src/components/nodes/utils/NodeVmUtils.ts
✅ Files skipped from review due to trivial changes (2)
- frontend/packages/metal3-plugin/package.json
- frontend/packages/console-app/locales/en/console-app.json
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/packages/metal3-plugin/console-extensions.json
- frontend/packages/console-dynamic-plugin-sdk/src/schema/console-extensions.ts
- frontend/packages/console-app/src/components/nodes/node-dashboard/tests/InventoryCard.spec.tsx
- frontend/packages/console-dynamic-plugin-sdk/src/extensions/node.ts
- frontend/packages/metal3-plugin/src/hooks/useAccessibleResources.ts
Member
Author
|
/test e2e-gcp-console |
Member
Author
|
/test e2e-playwright |
Member
Author
|
/test e2e-gcp-console |
Contributor
|
@jeff-phillips-18: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Analysis / Root cause:
The Node inventory card in the Node dashboard was hardcoded to display only Pods and Images, with BareMetalHost and VirtualMachine inventory
items conditionally rendered based on the NODE_MGMT_V1 feature flag. This tight coupling prevented dynamic plugins from contributing their
own inventory items to the Node dashboard, limiting extensibility.
The static plugin architecture required BareMetalHost-specific inventory logic to reside in the core console-app package, violating
separation of concerns and making it difficult for other plugins to add custom inventory items.
Solution description:
This PR introduces a new extension point console.node/inventory-item that allows dynamic and static plugins to contribute inventory items to
the Node dashboard's inventory card.
Key changes:
Priority ordering:
Screenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
Public API Changes:
Related Work:
Migration Notes:
Reviewers and assignees:
Console Approver:
/assign @jhadvig
Docs approver:
/assign @jseseCCS
PX approver:
/assign @rh-joshbeverly
🤖 Generated with https://claude.com/claude-code
Summary by CodeRabbit
New Features
Refactor
Documentation
Localization