MGMT-21058: Expose BareMetalHost (BMH) Status and Events in the Assisted Installer / CIM Console#3363
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgyselov: This pull request references MGMT-21058 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 task to target the "4.22.0" version, but no target version was set. 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-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
approved
📝 WalkthroughWalkthroughAdds BMH events UI and status info, a new useEvents hook and Event type, extends BMH status types, extracts/reorganizes host/agent table column modules, threads an optional Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as BMHEventsModal
participant Hook as useEvents
participant K8s as Kubernetes API
participant Table as Events Table
User->>UI: Click "View BMH events"
UI->>Hook: useEvents({ namespace, fieldSelector: "involvedObject.name=<bmhName>" })
Hook->>K8s: Watch Event.v1 with fieldSelector
K8s-->>Hook: Events list / stream
Hook-->>UI: { data, loading, error }
alt loading
UI->>UI: Render LoadingState
else error
UI->>UI: Render error message
else no events
UI->>UI: Render "No BMH events found."
else events
UI->>Table: Render sorted events (Time, Type, Message)
Table->>Table: Format Time (getHumanizedDateTime)
Table-->>User: Display rows
end
User->>UI: Close modal
UI->>UI: Cleanup modal state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@jgyselov: This pull request references MGMT-21058 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 task to target the "4.22.0" version, but no target version was set. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/cim/components/Agent/BMHStatus.tsx`:
- Around line 12-24: BMHStatus currently passes bmh ? <BMHStatusInfo .../> :
bmhStatus.errorMessage to Popover which can be undefined and show an empty
popover; update the component so the Popover body only renders when there is
content: check for bmh OR a non-empty bmhStatus.errorMessage and either (a)
render the Popover with bmh ? <BMHStatusInfo bmhStatus={bmhStatus} bmh={bmh}/> :
fallbackMessage (e.g. "No details available") or (b) render just the Button when
neither bmh nor bmhStatus.errorMessage exist; locate the BMHStatus component and
modify its conditional rendering accordingly to ensure no empty popover is
shown.
In `@libs/ui-lib/lib/cim/components/Agent/BMHStatusInfo.tsx`:
- Around line 57-64: Replace the hardcoded "Bare metal host" string inside the
LevelItem in the BMHStatusInfo component with a translated string: import and
use the i18n translation hook/function (e.g., useTranslation/t) in BMHStatusInfo
and call t('Bare metal host') or a proper key like t('bmh.label') where the
current LevelItem className="pf-v6-u-font-weight-bold" renders the label so the
component uses the translation function instead of a literal string.
In `@libs/ui-lib/lib/cim/components/Agent/tableColumns.tsx`:
- Around line 1-4: The import path contains an invalid './' segment; update the
import that brings in Host from
'@openshift-assisted/types/./assisted-installer-service' to use the correct
package export without the './' segment (i.e.,
'@openshift-assisted/types/assisted-installer-service'). Locate the import
statement referencing Host in tableColumns.tsx and replace the module specifier
accordingly so the module resolves via the package exports.
In `@libs/ui-lib/lib/cim/types/k8s/bare-metal-host.ts`:
- Line 31: Update the operationalStatus union type on the BareMetalHost type:
remove the 'delayed' literal and add 'servicing' so the union becomes 'OK' |
'discovered' | 'error' | 'detached' | 'servicing'; locate the operationalStatus
property in the BareMetalHost (or relevant interface) and replace the existing
union accordingly.
In `@libs/ui-lib/lib/common/components/hosts/tableColumns.tsx`:
- Around line 4-9: The import path for Cluster, Host, HostUpdateParams, and
Interface is incorrect due to an extra "./" in the module specifier; update the
import in tableColumns.tsx that references
'@openshift-assisted/types/./assisted-installer-service' to remove the './' so
it imports from '@openshift-assisted/types/assisted-installer-service' so the
symbols Cluster, Host, HostUpdateParams, and Interface resolve correctly.
🧹 Nitpick comments (14)
libs/locales/lib/en/translation.json (1)
125-125: Minor inconsistency in terminology: "Bare metal host" vs "BMH".The translation keys mix full term and acronym:
"ai:Bare metal host events"(spelled out)"ai:No BMH events found."and"ai:View BMH events"(acronym)This is a minor UI polish concern. Consider using consistent terminology for better user experience—either always spell out "Bare metal host" for clarity, or use "BMH" consistently if users are expected to know the acronym.
Also applies to: 602-602, 946-946
libs/ui-lib/lib/cim/types/fromOCP/k8sTypes.ts (1)
47-58: Add thereasonfield to EventK8sResource for better event categorization.The
reasonfield is a standard field in Kubernetes core/v1 Events—a short, machine-readable string describing why the event occurred (e.g.,"Created","FailedScheduling"). Including it enables clearer UI filtering and categorization of events.Add reason field
export type EventK8sResource = { type?: string; message?: string; eventTime?: string; + reason?: string; involvedObject?: { apiVersion?: string; kind?: string; name?: string; namespace?: string; uid?: string; }; } & K8sResourceCommon;libs/ui-lib/lib/common/components/hosts/tableColumns.tsx (1)
325-343: Missing i18n for column headers.The
ipv4Column,ipv6Column, andmacAddressColumnheaders use hardcoded strings instead of the translation functiont(), which is inconsistent with other columns in this file. Consider adding thetparameter and wrapping the titles.Example fix for ipv4Column
-export const ipv4Column = (cluster: Cluster): TableRow<Host> => ({ +export const ipv4Column = (cluster: Cluster, t: TFunction): TableRow<Host> => ({ header: { - title: 'IPv4 address', + title: t('ai:IPv4 address'), sort: true, },Also applies to: 345-363, 365-383
libs/ui-lib/lib/common/components/hosts/tableUtils.tsx (2)
53-59: Missing translation for "Install host" action.The "Install host" string is hardcoded while other action titles use
t()for translation.Proposed fix
if (onInstallHost && canInstallHost?.(host)) { actions.push({ - title: 'Install host', + title: t('ai:Install host'), id: `button-install-host-${hostname}`, onClick: () => onInstallHost(host), }); }
145-157: Add documentation for BMH detection convention.The condition
host.href === 'bmc'correctly identifies BMH-backed hosts and is consistently applied intoAssisted.ts. Consider enhancing documentation by adding a JSDoc comment to the BMH host creation intoAssisted.ts(line 103) that explicitly explains this convention serves as a marker for BMH identification in UI components liketableUtils.tsx.libs/ui-lib/lib/cim/components/Agent/BMHStatusInfo.tsx (1)
76-80: Consider formatting the timestamp for consistency.The
lastUpdatedvalue is displayed as-is, which is likely an ISO timestamp string. Other parts of this PR (e.g., BMHEventsModal) usehumanizedTimefor date formatting. Consider applying similar formatting here for a consistent user experience.libs/ui-lib/lib/cim/components/Agent/BMHEventsModal.tsx (4)
35-36: Consider more user-friendly error display.When
erroris truthy, it's rendered directly as content. However,errorfromuseK8sWatchResourcemay be an Error object rather than a string. Rendering an Error object directly may display[object Object]instead of a meaningful message.♻️ Suggested improvement
} else if (error) { - modalBody = <Content component={ContentVariants.p}>{error}</Content>; + modalBody = <Content component={ContentVariants.p}>{error instanceof Error ? error.message : String(error)}</Content>;
58-64: Sorting mutates the original array and sorts in ascending order (oldest first).Two observations:
Array.sort()mutates the original array. Consider using[...events].sort()to avoid mutating the hook's returned data.- The sort shows oldest events first. For events, users typically expect newest first. Consider reversing the sort order.
♻️ Suggested improvement
- {events - .sort( + {[...events] + .sort( (a, b) => - a.metadata?.creationTimestamp?.localeCompare( - b.metadata?.creationTimestamp || '', - ) || 0, + (b.metadata?.creationTimestamp || '').localeCompare( + a.metadata?.creationTimestamp || '', + ), )
65-66: Use a stable unique key instead of array index.Using the array index as a React key can cause issues with reconciliation when the list changes (e.g., new events arrive). Since events have
metadata?.uid, consider using that for a stable key.♻️ Suggested improvement
- .map((e, i) => ( - <Tr key={`bmc-event-${i}`}> + .map((e) => ( + <Tr key={e.metadata?.uid || e.metadata?.name}>
41-41: Consider making the height configurable or using CSS classes.The inline style with a fixed
400pxheight works but could be replaced with a PatternFly utility class or made configurable via props for better consistency and flexibility.libs/ui-lib/lib/cim/components/Agent/tableColumns.tsx (4)
89-90: AvoidhasOwnPropertyvia prototype; useObject.hasOwn()orinoperator.The eslint-disable comment acknowledges the issue. Using
Object.hasOwn()(ES2022) or theinoperator is preferred for checking property existence.♻️ Suggested improvement
- // eslint-disable-next-line no-prototype-builtins - discoveryType = agent?.metadata?.labels?.hasOwnProperty(AGENT_BMH_NAME_LABEL_KEY) + discoveryType = agent?.metadata?.labels && AGENT_BMH_NAME_LABEL_KEY in agent.metadata.labels ? t('ai:BMC') : t('ai:Discovery ISO');
237-237: Type assertion may hide undefined agent.Casting the result of
find()toAgentK8sResource(line 237) suppresses TypeScript'sundefinedpossibility. If no agent is found,getInfraEnvNameOfAgent(agent)receivesundefinedanyway (since the function accepts optional parameter), but the explicit cast is misleading.♻️ Suggested improvement
- const agent = agents.find((a) => a.metadata?.uid === host.id) as AgentK8sResource; + const agent = agents.find((a) => a.metadata?.uid === host.id);
240-244: Hardcoded URL path reduces flexibility.The infrastructure environment link path
/multicloud/infrastructure/environments/details/...is hardcoded. If this path changes or differs across deployments (e.g., ACM vs standalone), it would require code changes. Consider extracting this to a configuration or accepting a link generator function similar togetClusterDeploymentLinkinclusterColumn.
108-118: Consider exportingAgentStatusColumnPropstype.The type
AgentStatusColumnPropsis defined but not explicitly exported. If consumers need to construct props foragentStatusColumn, they would benefit from having access to this type. The AI summary indicates it should be a public export.♻️ Suggested improvement
-type AgentStatusColumnProps = { +export type AgentStatusColumnProps = {
openshift-merge-robot
removed
the
needs-rebase
jgyselov
force-pushed
the
bmc_events
branch
6 times, most recently
from
b20bcd2 to
1f96792
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@jgyselov: This pull request references MGMT-21058 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 task to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/cim/components/Agent/tableColumns.tsx`:
- Around line 237-238: The code unsafely asserts the result of agents.find into
AgentK8sResource and may pass undefined into getInfraEnvNameOfAgent; change the
logic to handle a missing agent instead of using "as AgentK8sResource" — e.g.,
store the result in a possibly-undefined variable (agent), check if agent is
undefined, and either provide a safe default/early-return or call
getInfraEnvNameOfAgent only when agent is defined; update references to agent
(the find call and the getInfraEnvNameOfAgent invocation) so the types are
narrowed and runtime errors are prevented.
- Line 248: Replace the hard-coded 'N/A' literal in the table column definition
with the translation call used elsewhere (use the existing translator function
t, e.g., t<string>('ai:N/A')) so the column is localized and consistent with
other columns like t<string>('ai:Unknown'); ensure the translation key ai:N/A is
added to your i18n resource files.
♻️ Duplicate comments (1)
libs/ui-lib/lib/cim/types/k8s/bare-metal-host.ts (1)
31-31: UpdateoperationalStatusunion to match metal3-io specification.The
operationalStatustype includes'delayed'which is not documented in the metal3-io/baremetal-operator specification, and is missing'servicing'which is a documented operational state.The type should be updated to:
'OK' | 'discovered' | 'error' | 'detached' | 'servicing'
🧹 Nitpick comments (2)
libs/ui-lib/lib/common/components/hosts/tableUtils.tsx (1)
108-114: Consider adding a guard foronViewHostEventsaction.Unlike other actions that have corresponding
can*guard functions (e.g.,canInstallHost,canEnable,canDownloadHostLogs), the "View host events" action is unconditionally added whenonViewHostEventsis provided. If there are scenarios where viewing events should be disabled, consider adding acanViewHostEventsguard for consistency.libs/ui-lib/lib/cim/components/Agent/tableColumns.tsx (1)
89-92: Consider usinginoperator instead ofhasOwnProperty.The eslint-disable comment suggests awareness of the prototype pollution risk. A cleaner alternative is the
inoperator which doesn't require the eslint exception:♻️ Suggested alternative
- // eslint-disable-next-line no-prototype-builtins - discoveryType = agent?.metadata?.labels?.hasOwnProperty(AGENT_BMH_NAME_LABEL_KEY) + discoveryType = (AGENT_BMH_NAME_LABEL_KEY in (agent?.metadata?.labels ?? {})) ? t<string>('ai:BMC') : t<string>('ai:Discovery ISO');
|
@jgyselov: This pull request references MGMT-21058 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 task to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/cim/components/Agent/BMHStatus.tsx`:
- Around line 13-18: The Popover in BMHStatus (the Popover component rendering
bodyContent={<BMHStatusInfo bmhStatus={bmhStatus} bmh={bmh} />}) has no
accessible name; add an aria-label prop to the Popover (for example
aria-label="Bare Metal Host status details" or similar descriptive text) so
screen readers can identify the popover content, ensuring the accessible name
clearly references the BMH status details.
🧹 Nitpick comments (2)
libs/ui-lib/lib/cim/types/fromOCP/k8sTypes.ts (1)
46-58: Ensure Event timestamps align with actual API payloads.Core/v1 Events frequently omit
eventTimeand still populatefirstTimestamp/lastTimestamp(deprecated) or rely onmetadata.creationTimestamp. Please verify the UI’s timestamp source and consider extending the type accordingly to avoid empty displays or sorting issues.🔧 Optional type extension
export type EventK8sResource = { type?: string; message?: string; eventTime?: string; + firstTimestamp?: string; + lastTimestamp?: string; involvedObject?: { apiVersion?: string; kind?: string; name?: string; namespace?: string; uid?: string; }; } & K8sResourceCommon;libs/ui-lib/lib/cim/hooks/useEvents.tsx (1)
1-14: Confirm the intended Event API group and make it explicit.If you’re targeting core/v1 Events, consider setting
group: ''for clarity; if you intendevents.k8s.io/v1, the group should be set accordingly. Please verify against the SDK expectations and cluster API.🔧 Optional explicit group (core/v1)
useK8sWatchResource<EventK8sResource[]>( { groupVersionKind: { + group: '', kind: 'Event', version: 'v1', }, }, props, );
|
@jgyselov: This pull request references MGMT-21058 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 task to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/cim/components/Agent/BMHEventsModal.tsx`:
- Around line 35-36: The modal currently casts `error` to string which can
render "[object Object]"; in the BMHEventsModal branch that sets `modalBody`
(the else-if with `error`), convert the error into a safe, human-readable string
before rendering (e.g., if `error` is an `Error` use `error.message`, else use
`String(error)` and fall back to `JSON.stringify` when appropriate) and pass
that string into the `Content` component (`ContentVariants.p`), keeping the use
of `useK8sWatchResource` and `modalBody` unchanged.
- Around line 58-73: The events rendering mutates the original events array by
calling sort() and uses the array index as the React key; fix by sorting a
shallow copy (e.g., use [...events] or events.slice()) before mapping to avoid
in-place mutation, and replace the unstable key `bmc-event-${i}` with a stable
identifier such as `e.metadata?.uid` (falling back to a deterministic composite
like `${e.type}-${e.metadata?.creationTimestamp}` if uid is absent) so keys
remain stable across updates.
In `@libs/ui-lib/lib/cim/components/Agent/tableColumns.tsx`:
- Around line 250-254: The returned column object sets sortableValue to
infraEnvName which can be undefined; change sortableValue to use a stable
fallback (e.g., infraEnvName ?? '' or infraEnvName || '') so sorting behaves
consistently when no agent is found—update the return in the same function where
sortableValue and infraEnvName are used to apply this fallback.
🧹 Nitpick comments (3)
libs/ui-lib/lib/cim/components/Agent/tableColumns.tsx (2)
89-92: Consider usingObject.hasOwn()or theinoperator.The
hasOwnPropertycall with eslint disable works but is slightly awkward. Modern alternatives are cleaner:// Using 'in' operator (avoids prototype pollution concerns) discoveryType = AGENT_BMH_NAME_LABEL_KEY in (agent?.metadata?.labels || {}) ? t<string>('ai:BMC') : t<string>('ai:Discovery ISO');This is a minor stylistic suggestion; the current implementation is functionally correct.
240-246: Consider extracting the hardcoded route to a constant or helper function.The URL path
/multicloud/infrastructure/environments/details/...is hardcoded here. If this route pattern is used elsewhere or subject to change, centralizing it would improve maintainability.This is a minor suggestion for future-proofing; the current implementation works correctly.
libs/ui-lib/lib/cim/components/Agent/BMHStatus.tsx (1)
12-25: Good implementation with accessibility improvements.The refactored component addresses previous review concerns:
bmhis now required (eliminating empty popover scenarios) and anaria-labelis added for screen readers.Consider using the translation function for the aria-label to maintain i18n consistency:
<Popover - aria-label="Bare metal host status details" + aria-label={t('ai:Bare metal host status details')} bodyContent={<BMHStatusInfo bmhStatus={bmhStatus} bmh={bmh} />}This would require adding
useTranslationto the component.
|
/cherry-pick releases/v2.17-cim |
|
@jgyselov: once the present PR merges, I will cherry-pick it on top of 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. |
|
@jgyselov: This pull request references MGMT-21058 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 task to target the "4.22.0" version, but no target version was set. 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. |
|
@jgyselov: This pull request references MGMT-21058 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 task to target the "4.22.0" version, but no target version was set. 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. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libs/ui-lib/lib/cim/components/Agent/tableUtils.tsx (1)
276-301: Handle BMH statuses in statusCount and filtering.
statusCountis initialized only fromagentStatuses, but you increment withbmhStatusestitles later (Line 380). If a BMH status title isn’t present inagentStatuses, the count becomesNaNand filter matching (Line 286) won’t pick up BMH status titles. This can break BMH status counts and filters.🐛 Proposed fix
-const filterByStatus = ( +const filterByStatus = ( hosts: Host[], agents: AgentK8sResource[], agentStatuses: HostStatus<string>, bareMetalHosts: BareMetalHostK8sResource[], + bmhStatuses: HostStatus<string> | undefined, statusFilter: string[], ) => { if (!statusFilter?.length) { return hosts; } - const statusKeys = Object.keys(agentStatuses).filter((k) => - statusFilter.includes(agentStatuses[k].title), - ); + const statusKeys = [ + ...Object.keys(agentStatuses).filter((k) => statusFilter.includes(agentStatuses[k].title)), + ...Object.keys(bmhStatuses || {}).filter((k) => statusFilter.includes(bmhStatuses![k].title)), + ]; return hosts.filter((h) => { const agent = agents.find((a) => a.metadata?.uid === h.id); if (!agent) { const bmh = bareMetalHosts.find((bmh) => bmh.metadata?.uid === h.id); if (!bmh) { return false; } return statusKeys.includes(getBMHStatusKey(bmh) || ''); } const { status } = getAgentStatus(agent, agentStatuses); return statusKeys.includes(status.key); }); }; -const statusCount = Object.keys(agentStatuses).reduce((acc, curr) => { +const statusCount = { + ...Object.keys(agentStatuses).reduce((acc, curr) => { + acc[agentStatuses[curr].title] = 0; + return acc; + }, {} as StatusCount), + ...Object.keys(bmhStatuses || {}).reduce((acc, curr) => { + acc[bmhStatuses![curr].title] = 0; + return acc; + }, {} as StatusCount), +}; - acc[agentStatuses[curr].title] = 0; - return acc; -}, {} as StatusCount);- const { hosts: filteredHosts, sorted } = filterHosts( + const { hosts: filteredHosts, sorted } = filterHosts( hosts, agents, agentStatuses, bmhs, + bmhStatuses, hostnameFilter, statusFilter, labelFilter, );-const filterHosts = (hosts, agents, agentStatuses, bareMetalHosts, hostnameFilter, statusFilter, labelFilter) => { +const filterHosts = (hosts, agents, agentStatuses, bareMetalHosts, bmhStatuses, hostnameFilter, statusFilter, labelFilter) => { ... - const byStatus = filterByStatus(byHostname, agents, agentStatuses, bareMetalHosts, statusFilter); + const byStatus = filterByStatus(byHostname, agents, agentStatuses, bareMetalHosts, bmhStatuses, statusFilter);Also applies to: 368-381, 387-395
libs/ui-lib/lib/common/components/hosts/tableUtils.tsx (2)
53-58: Localize “Install host” label.
This is the only action title not wrapped int(...).🔧 Suggested fix
- actions.push({ - title: 'Install host', + actions.push({ + title: t('ai:Install host'), id: `button-install-host-${hostname}`, onClick: () => onInstallHost(host), });
60-86: Fix inconsistent pattern for Edit Host action to match other action handlers.The Edit Host action checks for
canEditHostexistence differently than all other actions in the resolver. While Install, Enable, Disable, Reset, and Download Logs all use optional chaining (canCheck?.(host)), Edit Host requires an explicit null check (if (canEditHost)), making it fragile if a caller passesonEditHostwithoutcanEditHost. Both current call sites provide the check, but this structure deviates from the established pattern and risks silent failures in future code.Align the Edit Host action with other handlers by defaulting to
truewhencanEditHostis missing, allowing the action to display when no permission check is provided.Suggested fix
- if (onEditHost) { - if (canEditHost) { - const canEdit = canEditHost(host); - if (typeof canEdit === 'boolean') { - canEdit && - actions.push({ - title: t('ai:Change hostname'), - id: `button-edit-host-${hostname}`, - onClick: () => onEditHost(host), - }); - } else { - const [enabled, reason] = canEdit; - actions.push({ - title: ( - <ActionTitle - disabled={!enabled} - description={reason} - title={t('ai:Change hostname')} - /> - ), - id: `button-edit-host-${hostname}`, - onClick: () => onEditHost(host), - isDisabled: !enabled, - }); - } - } - } + if (onEditHost) { + const canEdit = canEditHost ? canEditHost(host) : true; + if (typeof canEdit === 'boolean') { + canEdit && + actions.push({ + title: t('ai:Change hostname'), + id: `button-edit-host-${hostname}`, + onClick: () => onEditHost(host), + }); + } else { + const [enabled, reason] = canEdit; + actions.push({ + title: ( + <ActionTitle + disabled={!enabled} + description={reason} + title={t('ai:Change hostname')} + /> + ), + id: `button-edit-host-${hostname}`, + onClick: () => onEditHost(host), + isDisabled: !enabled, + }); + } + }
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/cim/components/Agent/BMHStatusInfo.tsx`:
- Around line 84-91: The modal is rendered even when bmh.metadata is missing,
causing useEvents to query with empty namespace/name; update the render guard to
require metadata and its name/namespace before mounting BMHEventsModal (e.g.,
change the condition from isModalOpen to isModalOpen && bmh.metadata &&
bmh.metadata.name && bmh.metadata.namespace) and/or prevent setModalOpen(true)
when metadata is absent so BMHEventsModal (and its useEvents hook) never runs
with empty values.
In `@libs/ui-lib/lib/cim/components/Agent/tableColumns.tsx`:
- Around line 44-53: The readonly calculation uses canEditHostname/canEditBMH
which return an ActionCheck tuple, so negating the tuple yields incorrect
results; update the logic in the agent/bmh branches (where readonly is set) to
normalize the ActionCheck result to a boolean first (e.g., call
canEditHostname(host) / canEditBMH(host), if the result is an array take the
first element as the allowed flag, otherwise coerce to Boolean), then set
readonly = !allowed; reference the variables/functions readonly, agent, bmh,
canEditHostname, canEditBMH and the useAgentsTable ActionCheck return shape when
making the change.
In `@libs/ui-lib/lib/common/components/hosts/tableColumns.tsx`:
- Around line 325-381: The IPv4/IPv6/MAC column headers in ipv4Column,
ipv6Column, and macAddressColumn are hard-coded; update each header.title to use
the localization function (e.g., t<string>(...)) instead of raw strings so they
follow the same i18n pattern as other table headers—replace 'IPv4 address',
'IPv6 address', and 'MAC address' with calls like
t<string>('hosts.table.ipv4Address') (or your existing translation keys) to
ensure localization and consistency.
🧹 Nitpick comments (1)
libs/ui-lib/lib/cim/components/Agent/BMHStatusInfo.tsx (1)
61-63: Consider adding a fallback for undefined operationalStatus.When
bmh.status?.operationalStatusisundefined, the component renders nothing before the icon, which may result in inconsistent spacing or a confusing UI state.Suggested improvement
<LevelItem> - {bmh.status?.operationalStatus} {icon} + {bmh.status?.operationalStatus ?? t('ai:Unknown')} {icon} </LevelItem>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgyselov, LiorSoffer 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 |
openshift-merge-bot
bot
merged commit 33008bf
into
openshift-assisted:master
|
@jgyselov: #3363 failed to apply on top of branch "releases/v2.17-cim": 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. |
5 participants
https://issues.redhat.com/browse/MGMT-21058
Exposes bare metal host status inside the status popover for BMC hosts. Adds a "Bare metal host events" for BMC events. (Note that these are temporary and disappear after ~24h.)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.