CONSOLE-5284: Put new node inventory items behind tech-preview#16414
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jeff-phillips-18: This pull request references CONSOLE-5284 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 bug to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" 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. |
📝 WalkthroughWalkthroughThe InventoryCard component gates inventory display sections behind a feature flag ( 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-app/src/components/nodes/node-dashboard/InventoryCard.tsx`:
- Around line 61-64: The VM watch hook useWatchVirtualMachineInstances is being
invoked unconditionally (creating side effects) even when FLAG_NODE_MGMT_V1 is
off; modify the hook or its usage so watching is gated by the feature flag:
update useWatchVirtualMachineInstances to accept an enabled boolean (e.g.,
useWatchVirtualMachineInstances(nodeName: string, enabled = true)) or, if you
cannot change the hook, only call it when nodeMgmtV1Enabled && showVms and
provide safe fallback values for vms, vmsLoaded, and vmsLoadError; in
InventoryCard.tsx replace the unconditional call with a guarded call using
nodeMgmtV1Enabled (and showVms) so the VM watch traffic only starts when
FLAG_NODE_MGMT_V1 is true.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3569d736-5944-47b0-ac9a-35fe6aa34216
📒 Files selected for processing (1)
frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx
📜 Review details
🔇 Additional comments (1)
frontend/packages/console-app/src/components/nodes/node-dashboard/InventoryCard.tsx (1)
95-124: Good boundary for tech-preview UI gating.This correctly keeps Pods/Images always visible while placing bare metal + VM inventory UI behind
nodeMgmtV1Enabled.
jeff-phillips-18
force-pushed
the
node-inventory-items-flagged
branch
2 times, most recently
from
8ac9499 to
43a209c
Compare
openshift-ci
Bot
added
the
kind/i18n
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
Clean, well-scoped change. The feature flag gating is correct — no items leak when NODE_MGMT_V1 is off, pod inventory is unaffected, and the refactoring of VM inventory into its own component is a solid improvement. All dead imports properly cleaned up, no barrel import violations, i18n keys preserved.
The only gap is test coverage for the new component and the flag-gating behavior. Once tests are added this is good to go. See inline comments.
| {nodeMgmtV1Enabled && ( | ||
| <> | ||
| <BareMetalInventoryItems /> | ||
| <VirtualMachinesInventoryItems /> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
The core behavioral change of this PR — gating bare metal and VM items behind NODE_MGMT_V1 — has no automated test coverage. Please add tests for InventoryCard verifying:
- With
NODE_MGMT_V1off:BareMetalInventoryItemsandVirtualMachinesInventoryItemsdo not render - With
NODE_MGMT_V1on: both render - Pod inventory always renders regardless of flag state
| ); | ||
| }; | ||
|
|
||
| export default VirtualMachinesInventoryItems; |
There was a problem hiding this comment.
New component without unit tests. There's an existing BareMetalInventoryItems.spec.tsx that establishes a clear testing pattern — a matching VirtualMachinesInventoryItems.spec.tsx should be added for parity. At minimum:
- Renders nothing when kubevirt plugin is not active
- Renders VM count with link when kubevirt is active and VMs exist
- Handles loading and error states
|
|
||
| const [vms, vmsLoaded, vmsLoadError] = useWatchVirtualMachineInstances(obj.metadata.name); | ||
|
|
||
| if (!showVms) { |
There was a problem hiding this comment.
👍 Good extraction. The early-return guard pattern mirrors BareMetalInventoryItems nicely, and useWatchVirtualMachineInstances already handles the disabled case internally (passes undefined to useK8sWatchResource when kubevirt is inactive), so no API watch leaks here despite the hook being called before the guard — this is the correct approach given Rules of Hooks.
jeff-phillips-18
force-pushed
the
node-inventory-items-flagged
branch
from
43a209c to
b5bd35d
Compare
QA Verification Evidence
Verification Steps
Animated overview (click to expand)
Step 1: description (status)
Step 2: Node list page (pass)
Step 3: Worker node dashboard (pass)
Step 4: Worker node Inventory card (flag disabled) (pass)
Warning This verification was performed by an AI agent. Results may contain false positives or miss Automated QA verification by Claude Code |
openshift-ci-robot
added
the
verified
|
@jhadvig: This PR has been marked as verified by 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. |
openshift-ci
Bot
added
lgtm
jhadvig
added
the
plugin-api-approved
|
/label px-approved |
openshift-ci
Bot
added
the
px-approved
jseseCCS
left a comment
jseseCCS
left a comment
There was a problem hiding this comment.
looked over user-facing strings; left minor suggestions inline.
approving in good faith that you'll consider these comments, apply them as appropriate. Thanks!
| "Network transfer": "Network transfer", | ||
| "Pod count": "Pod count", | ||
| "Virtual machines": "Virtual machines", | ||
| "This count reflects your access permissions and might not include all virtual machines.": "This count reflects your access permissions and might not include all virtual machines.", |
There was a problem hiding this comment.
396/413: "This count reflects your access permissions and might not include all virtual machines" leads w/technical explanation instead of user impact. rewrite to focus on UX?
--> You might not see all virtual machines, depending on your access permissions.
OR
The total shown is based on your access permissions and might not include all virtual machines.
| <DescriptionListTermHelp | ||
| text={t('console-app~Virtual machines')} | ||
| textHelp={t( | ||
| 'console-app~This count reflects your access permissions and might not include all virtual machines.', |
|
/label docs-approved |
openshift-ci
Bot
added
the
docs-approved
jeff-phillips-18
force-pushed
the
node-inventory-items-flagged
branch
from
b5bd35d to
76bb8b0
Compare
openshift-ci-robot
removed
the
verified
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@jhadvig: This PR has been marked as verified by 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeff-phillips-18, jhadvig, jseseCCS The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@jeff-phillips-18: all tests passed! 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. |
|
/cherry-pick release-4.22 |
|
@jeff-phillips-18: new pull request created: #16524 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. |
6 participants
Analysis / Root cause:
New node inventory items (bare metal hardware metrics and virtual machines) need to be gated behind the tech-preview
NODE_MGMT_V1feature flag as part of the node management v1 feature rollout.Solution description:
FLAG_NODE_MGMT_V1flag check to the node dashboard InventoryCard componentBareMetalInventoryItemsand virtual machine inventory items in a conditional render based on the flag**Screenshots **:
*** Before / With Tech-Preview:
*** After / without Tech-Preview
Test setup:
NODE_MGMT_V1feature flag both enabled and disabledTest cases:
NODE_MGMT_V1flag disabled: BareMetalInventoryItems and VM inventory items should not be displayed on node dashboardNODE_MGMT_V1flag enabled: BareMetalInventoryItems (disks, NICs, CPUs) should be displayed when a BareMetalHost is associated with the nodeNODE_MGMT_V1flag enabled: VM inventory count should be displayed when KubeVirt plugin is active and VMs exist on the nodeBrowser conformance:
Additional info:
This change is part of the larger node management v1 feature work tracked in CONSOLE-5284. The feature flag allows for controlled rollout and testing of the new inventory items.
Reviewers and assignees:
/cc @jhadvig
Summary by CodeRabbit