AGENT-1351: late binding hosts to cluster in ABI#3259
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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. |
WalkthroughThis PR introduces late binding functionality for hosts in the assisted installer UI. It adds APIs and services to bind hosts to clusters, creates hooks for fetching and polling infra environment hosts, implements a late-binding hook that automatically binds hosts when a feature flag is enabled, updates components to integrate this flow, and extends alert infrastructure to support custom alert identification. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as HostDiscovery<br/>Component
participant useLB as useLateBinding<br/>Hook
participant useIEH as useInfraEnvHosts<br/>Hook
participant HS as HostsService
participant API as HostsAPI
participant Alerts as Alert<br/>Manager
Component->>useLB: render (cluster, feature flag)
useLB->>useIEH: fetch hosts from infraEnv
useIEH->>API: getHosts(infraEnvId)
API-->>useIEH: hosts[]
useIEH->>useIEH: poll periodically
useIEH-->>useLB: hosts, isLoading, error
rect rgb(200, 220, 255)
Note over useLB: When flag enabled & host<br/>has different clusterId
useLB->>Alerts: removeAlert(host.id)
useLB->>HS: bind(host, clusterId)
HS->>API: bindHost(infraEnvId, hostId, clusterId)
API-->>HS: success or error
alt Error
HS-->>useLB: error
useLB->>Alerts: addAlert(key, error message)
end
end
useLB-->>Component: isBinding state
Component->>Component: disable next button if isBinding
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
openshift-ci
bot
added
the
size/L
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(3 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx
🧬 Code graph analysis (5)
libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts (1)
libs/types/assisted-installer-service.d.ts (2)
InfraEnv(1750-1827)Host(1144-1317)
libs/ui-lib/lib/ocm/services/HostsService.ts (1)
libs/types/assisted-installer-service.d.ts (2)
Host(1144-1317)Cluster(110-407)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (4)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-407)Host(1144-1317)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
useInfraEnvId(12-66)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts (1)
getHosts(71-75)libs/ui-lib/lib/common/config/constants.ts (1)
DEFAULT_POLLING_INTERVAL(13-13)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
libs/ui-lib/lib/common/features/featureGate.tsx (1)
useFeature(56-59)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
useInfraEnvHosts(9-62)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
libs/types/assisted-installer-service.d.ts (3)
InfraEnv(1750-1827)Host(1144-1317)Cluster(110-407)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: format
🔇 Additional comments (2)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (2)
1-19: Hook signature and imports look good.The parameters and return type are well-defined, and all imports are correctly structured.
20-28: State management and loading logic are correct.The state initialization and
isLoadingcomputation correctly reflect the loading state: loading when neitherhostsnorerroris present, and not loading once either is set.Also applies to: 61-61
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
0e7d5da to
3cdd468
Compare
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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: 0
♻️ Duplicate comments (4)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
69-74: Fix incorrectuseInfraEnvHostsarguments.The hook call passes only four arguments, causing
cluster.openshiftVersionto be shifted into thepullSecretparameter slot, leaving the actualopenshiftVersionparameter undefined. This breaks the infra-env lookup and registration flow, preventing hosts from binding correctly.Apply this diff to fix the argument order:
const { hosts: infraEnvHosts, error: infraEnvError, isLoading: infraEnvLoading, } = useInfraEnvHosts( cluster.id, cluster.cpuArchitecture as CpuArchitecture, cluster.name, - cluster.openshiftVersion, + undefined, + cluster.openshiftVersion, );
88-100: Handle bind failures so polling can retry.The
boundHostIdsRefis updated before the asynchronous bind operation completes. IfHostsService.bindfails (e.g., due to a transient network error), the host ID remains in the ref, preventing any future retry attempts during polling. This leaves the host unbound indefinitely.Apply this diff to handle bind failures:
if (host.clusterId !== cluster.id && !boundHostIdsRef.current.has(host.id)) { boundHostIdsRef.current.add(host.id); - void HostsService.bind(host, cluster.id); + void HostsService.bind(host, cluster.id).catch(() => { + boundHostIdsRef.current.delete(host.id); + }); }libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (2)
30-41: Critical bug: hardcodedcpuArchitecturein cache invalidation.Line 38 uses
CpuArchitecture.USE_DAY1_ARCHITECTUREinstead of thecpuArchitectureparameter. SinceuseInfraEnvIdretrieves the infraEnvId using thecpuArchitectureparameter, cache invalidation must use the same value to correctly clear the cached entry.Additionally, when an error occurs, the
hostsstate is not cleared, leaving stale data visible. ThecpuArchitecturedependency is also missing from the callback's dependency array.Apply this diff:
const getHosts = React.useCallback(async () => { try { if (infraEnvId) { const { data: hostsData } = await InfraEnvsAPI.getHosts(infraEnvId); setHosts(hostsData); } } catch (e) { // Invalidate this cluster's cached data - InfraEnvIdsCacheService.removeInfraEnvId(clusterId, CpuArchitecture.USE_DAY1_ARCHITECTURE); + InfraEnvIdsCacheService.removeInfraEnvId(clusterId, cpuArchitecture); + setHosts(undefined); setError(getErrorMessage(e)); } - }, [clusterId, infraEnvId]); + }, [clusterId, cpuArchitecture, infraEnvId]);
43-59: Consider clearing stale hosts wheninfraEnvIdErroris set.When
infraEnvIdErrorbecomes truthy (line 44), the error is set buthostsis not cleared. If hosts were previously loaded, they remain visible alongside the error, which could confuse consumers of the hook.Additionally, polling continues unconditionally once started, even if
getHostsrepeatedly fails. Consider whether a backoff strategy or stopping after consecutive failures would improve resilience and reduce unnecessary API calls.Apply this diff to clear hosts on
infraEnvIdError:React.useEffect(() => { if (infraEnvIdError) { + setHosts(undefined); setError(infraEnvIdError); } else {
🧹 Nitpick comments (1)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
66-77: LGTM!The
bindHostmethod correctly implements the bind endpoint as described in the PR objectives. The method signature, endpoint construction, and request body format are all appropriate.The explicit
acceptandContent-Typeheaders are typically redundant if the axios client instance already sets these defaults. You may simplify by removing the headers configuration unless they're specifically required to override client defaults:bindHost(infraEnvId: InfraEnv['id'], hostId: Host['id'], clusterId: Cluster['id']) { return client.post<Host, AxiosResponse<Host>, { cluster_id: string }>( `${HostsAPI.makeActionsBaseURI(infraEnvId, hostId)}/bind`, { cluster_id: clusterId }, - { - headers: { - accept: 'application/json', - 'Content-Type': 'application/json', - }, - }, ); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(3 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/ocm/services/HostsService.ts
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx
🧬 Code graph analysis (3)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
libs/ui-lib/lib/common/features/featureGate.tsx (1)
useFeature(56-59)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
useInfraEnvHosts(9-62)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
libs/types/assisted-installer-service.d.ts (3)
InfraEnv(1750-1827)Host(1144-1317)Cluster(110-407)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (4)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-407)Host(1144-1317)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
useInfraEnvId(12-66)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts (1)
getHosts(71-75)libs/ui-lib/lib/common/config/constants.ts (1)
DEFAULT_POLLING_INTERVAL(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (8)
libs/ui-lib/lib/ocm/hooks/index.ts (1)
8-8: LGTM!The export follows the established pattern for this barrel file and correctly exposes the new hook.
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
2-2: LGTM!The
Clustertype import is correctly added to support the newbindHostmethod signature.libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (3)
1-1: LGTM!The new React hook imports (
useEffect,useRef) and utility imports (CpuArchitecture,useFeature) are correctly added and used in the binding logic.Also applies to: 9-10
34-35: LGTM!The imports for
useInfraEnvHostsandHostsServiceare correctly added and used in the binding flow.
76-86: LGTM!The ref tracking and cluster change reset logic correctly prevent stale binding state when the cluster changes. The effect appropriately clears the bound host IDs when
cluster.idchanges.libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (3)
1-8: LGTM!All imports are correctly used throughout the hook implementation.
9-28: LGTM!The hook signature, state initialization, and
useInfraEnvIdinvocation are correctly implemented.
61-61: LGTM!The
isLoadingderivation correctly indicates loading state when neither hosts nor error are available.
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
3cdd468 to
63fc2ed
Compare
ElayAharoni
requested review from
rawagner
and removed request for
batzionb and
celdrake
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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: 0
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (1)
89-103: Past issue resolved - bind failures are now handled.The
.catch()handler properly removes failed hosts from the ref, allowing retries. The implementation is sound.Optional improvement: Consider adding retry limits and error visibility.
Currently, failed bind operations will retry indefinitely on each poll. For production robustness, you might want to:
- Track retry counts per host to avoid infinite retry loops
- Surface bind errors to users (e.g., via toast notifications or error state)
- Implement exponential backoff for failed binds
These enhancements would improve observability and prevent resource waste from persistent failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(3 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
- libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx
🧬 Code graph analysis (2)
libs/ui-lib/lib/ocm/services/HostsService.ts (1)
libs/types/assisted-installer-service.d.ts (2)
Host(1144-1317)Cluster(110-407)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
libs/ui-lib/lib/common/features/featureGate.tsx (1)
useFeature(56-59)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
useInfraEnvHosts(9-63)
🔇 Additional comments (9)
libs/ui-lib/lib/ocm/hooks/index.ts (1)
8-8: LGTM!The export follows the established pattern and properly exposes the new hook to consumers.
libs/ui-lib/lib/ocm/services/HostsService.ts (3)
2-2: LGTM!The Cluster import is necessary for the new bind method's type signature.
88-94: LGTM!The bind method follows the established service pattern with proper validation and delegation to the API layer.
96-103: Good refactor for code reuse.Using
HostsService.installinstead of direct API calls improves maintainability and ensures consistent validation logic.libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (5)
1-1: LGTM!All new imports are necessary for the infraEnv-host binding feature.
Also applies to: 9-10, 34-35
63-63: LGTM!Proper feature flag check to gate the new binding functionality.
65-75: Past issue resolved - arguments are now correct.The hook call now properly passes
undefinedas the pullSecret placeholder andcluster.openshiftVersionas the fifth argument, matching the expected signature. Good fix!
78-79: LGTM!Using refs to track bound hosts and detect cluster changes is the right approach for preventing duplicate bind operations.
82-87: LGTM!The reset logic properly clears bound host tracking when the cluster changes, preventing stale state issues.
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
63fc2ed to
1e8c230
Compare
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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
🧹 Nitpick comments (1)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
66-77: Consider removing redundant headers for consistency.The
bindHostimplementation is correct, but the explicit headers (lines 71-75) are unnecessary since the axios client setsContent-Type: application/jsonby default for POST requests with JSON bodies. Other methods in this file (reset,installHost) don't explicitly set these headers.Apply this diff to align with the existing pattern:
bindHost(infraEnvId: InfraEnv['id'], hostId: Host['id'], clusterId: Cluster['id']) { return client.post<Host, AxiosResponse<Host>, { cluster_id: string }>( `${HostsAPI.makeActionsBaseURI(infraEnvId, hostId)}/bind`, { cluster_id: clusterId }, - { - headers: { - accept: 'application/json', - 'Content-Type': 'application/json', - }, - }, ); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(3 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-lib/lib/ocm/hooks/index.ts
- libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx
🧬 Code graph analysis (3)
libs/ui-lib/lib/ocm/services/HostsService.ts (2)
libs/types/assisted-installer-service.d.ts (2)
Host(1144-1317)Cluster(110-407)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
HostsAPI(6-6)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
libs/ui-lib/lib/common/features/featureGate.tsx (1)
useFeature(56-59)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
useInfraEnvHosts(9-63)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
libs/types/assisted-installer-service.d.ts (3)
InfraEnv(1750-1827)Host(1144-1317)Cluster(110-407)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: format
🔇 Additional comments (3)
libs/ui-lib/lib/ocm/services/HostsService.ts (1)
88-94: LGTM! Clean implementation following established patterns.The
bindmethod correctly validateshost.infraEnvIdpresence before delegating to the API layer, consistent with other methods in this service.libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
82-87: LGTM! Correct reset logic when cluster changes.The effect properly clears the bound host IDs when switching to a different cluster, preventing stale bindings from affecting the new cluster context.
89-103: LGTM! Proper error handling for bind operations.The binding logic correctly:
- Gates execution on the feature flag
- Prevents duplicate bindings via the ref-tracked Set
- Handles failures by removing the host ID, allowing retry on the next poll
- Includes all necessary dependencies in the effect array
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
1e8c230 to
6788f40
Compare
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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: 0
🧹 Nitpick comments (4)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
66-77: LGTM with optional simplification.The bind method is correctly implemented and follows the API pattern. The explicit headers (lines 70-74) are redundant since axios automatically sets
Content-Type: application/jsonfor object bodies and handlesacceptheaders. Other methods in this file (e.g.,installHostat lines 61-65) omit these headers.Apply this diff to simplify:
bindHost(infraEnvId: InfraEnv['id'], hostId: Host['id'], clusterId: Cluster['id']) { return client.post<Host, AxiosResponse<Host>, { cluster_id: string }>( `${HostsAPI.makeActionsBaseURI(infraEnvId, hostId)}/bind`, { cluster_id: clusterId }, - { - headers: { - accept: 'application/json', - 'Content-Type': 'application/json', - }, - }, ); },libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
43-60: Consider adding resilience for repeated failures.Line 45 now correctly clears stale hosts when
infraEnvIdErroris set (fixing the previous review issue). However, the polling logic (lines 52-55) continues indefinitely even whengetHostsrepeatedly fails. While this provides resilience for transient errors, a backoff strategy or stopping after consecutive failures would reduce unnecessary API calls and improve UX.For example, add a failure counter:
const [consecutiveFailures, setConsecutiveFailures] = React.useState(0); const MAX_FAILURES = 5; const getHosts = React.useCallback(async () => { try { if (infraEnvId) { const { data: hostsData } = await InfraEnvsAPI.getHosts(infraEnvId); setHosts(hostsData); setConsecutiveFailures(0); // Reset on success } } catch (e) { InfraEnvIdsCacheService.removeInfraEnvId(clusterId, cpuArchitecture); setError(getErrorMessage(e)); setConsecutiveFailures(prev => prev + 1); } }, [clusterId, cpuArchitecture, infraEnvId]); React.useEffect(() => { if (infraEnvIdError) { setHosts(undefined); setError(infraEnvIdError); } else { if (infraEnvId && consecutiveFailures < MAX_FAILURES) { void getHosts(); const intervalId = setInterval(() => { void getHosts(); }, DEFAULT_POLLING_INTERVAL); return () => clearInterval(intervalId); } } }, [getHosts, infraEnvId, infraEnvIdError, consecutiveFailures]);libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
65-77: Consider extracting cpuArchitecture fallback and displaying errors.Lines 71-73 address the previous review concern by providing a fallback, but the ternary is verbose. Consider extracting to a const or using a helper function for clarity. More importantly,
infraEnvError(line 67) is retrieved but never displayed to the user, providing no feedback when infraEnv retrieval fails.Simplify the cpuArchitecture handling:
+ const effectiveCpuArchitecture = cluster.cpuArchitecture + ? (cluster.cpuArchitecture as CpuArchitecture) + : CpuArchitecture.USE_DAY1_ARCHITECTURE; + const { hosts: infraEnvHosts, error: infraEnvError, isLoading: infraEnvLoading, } = useInfraEnvHosts( cluster.id, - cluster.cpuArchitecture - ? (cluster.cpuArchitecture as CpuArchitecture) - : CpuArchitecture.USE_DAY1_ARCHITECTURE, + effectiveCpuArchitecture, cluster.name, undefined, cluster.openshiftVersion, );Additionally, consider displaying
infraEnvErrorto the user (e.g., via an Alert or inline message) so they understand why host binding is failing.
91-105: Consider surfacing binding errors to the user.While line 99-101 correctly handles bind failures by allowing retries, the
.catch()silently swallows errors. Users have no visibility when host binding fails, making troubleshooting difficult. Consider displaying errors via an Alert or notification.For example, track binding errors in state:
const [bindingErrors, setBindingErrors] = React.useState<Record<string, string>>({}); useEffect(() => { if (infraEnvHosts && !infraEnvError && !infraEnvLoading && isSingleClusterFeatureEnabled) { infraEnvHosts.forEach((host) => { if (host.clusterId !== cluster.id && !boundHostIdsRef.current.has(host.id)) { boundHostIdsRef.current.add(host.id); void HostsService.bind(host, cluster.id).catch((error) => { boundHostIdsRef.current.delete(host.id); setBindingErrors(prev => ({ ...prev, [host.id]: getErrorMessage(error) })); }); } }); } }, [cluster.id, infraEnvHosts, infraEnvError, infraEnvLoading, isSingleClusterFeatureEnabled]);Then display errors in the UI (e.g., above the table).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(3 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
- libs/ui-lib/lib/ocm/hooks/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsxlibs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts
🧬 Code graph analysis (4)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
libs/ui-lib/lib/common/features/featureGate.tsx (1)
useFeature(56-59)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
useInfraEnvHosts(9-63)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
libs/types/assisted-installer-service.d.ts (3)
InfraEnv(1750-1827)Host(1144-1317)Cluster(110-407)
libs/ui-lib/lib/ocm/services/HostsService.ts (2)
libs/types/assisted-installer-service.d.ts (2)
Host(1144-1317)Cluster(110-407)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
HostsAPI(6-6)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (4)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-407)Host(1144-1317)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
useInfraEnvId(12-66)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts (1)
getHosts(71-75)libs/ui-lib/lib/common/config/constants.ts (1)
DEFAULT_POLLING_INTERVAL(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: circular-deps
- GitHub Check: translation-files
- GitHub Check: unit-tests
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (3)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
30-41: LGTM! Previous issues resolved.The critical bug from the previous review has been fixed: Line 38 now correctly uses the
cpuArchitectureparameter (instead of the hardcoded value), and line 41 includescpuArchitecturein the dependency array. The cache invalidation will now work correctly.libs/ui-lib/lib/ocm/services/HostsService.ts (1)
88-94: LGTM!The
bindmethod follows the established pattern in this service: validates thatinfraEnvIdis present, throws a descriptive error if not, and delegates to the correspondingHostsAPImethod. The implementation is consistent with other methods likedelete,reset, andinstall.libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (1)
91-105: LGTM! Bind error handling fixed.The binding logic correctly prevents duplicate bindings using
boundHostIdsRefand now properly handles failures. Line 99-101's.catch()handler removes the host ID from the set on error (fixing the issue from the previous review), allowing retries for transient failures. The feature flag gating and dependencies are correct.
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
6788f40 to
e59a5bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
30-41: Clear hosts state on fetch errors.When
getHostscatches an error (line 36-40), it invalidates the cache and sets the error message but does not clear thehostsstate. This leaves stale host data visible alongside the error, which could confuse consumers of the hook. Apply this fix:} catch (e) { // Invalidate this cluster's cached data InfraEnvIdsCacheService.removeInfraEnvId(clusterId, cpuArchitecture); + setHosts(undefined); setError(getErrorMessage(e)); }Note: The cache invalidation correctly uses the
cpuArchitectureparameter, addressing the previous review concern.
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
65-77: Consider conditional hook invocation or undefined clusterId.When the feature flag is disabled, passing an empty string as
clusterId(line 70) will trigger an error state inuseInfraEnvId("Missing clusterId..."), creating unnecessary error state that is never used since the binding logic is also gated by the same feature flag. Consider either:
- Conditionally calling the hook only when the feature is enabled, or
- Passing
undefinedand having the hook handle it gracefully by short-circuiting.The current implementation works but generates side effects (error states) that serve no purpose.
Note: The
cpuArchitecturefallback correctly addresses the previous review concern about optional fields.
91-105: Add error logging for debugging bind failures.The bind failure handling correctly removes the host ID from the tracking ref (addressing the previous review comment), but the error is silently swallowed. For debugging and monitoring purposes, consider logging bind failures:
void HostsService.bind(host, cluster.id).catch(() => { + console.error(`Failed to bind host ${host.id} to cluster ${cluster.id}:`, error); boundHostIdsRef.current.delete(host.id); });libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
43-60: Consider implementing backoff for failed polling.The polling logic (lines 52-55) continues indefinitely at a fixed interval, even when
getHostsrepeatedly fails. If the service is experiencing issues, this generates unnecessary API load every 10 seconds. Consider implementing:
- Exponential backoff after consecutive failures, or
- Stopping polling after N consecutive failures with user-visible messaging
Note: The
infraEnvIdErrorhandling correctly clears hosts (line 45), addressing the previous review concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(3 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
- libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts
- libs/ui-lib/lib/ocm/services/HostsService.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.tslibs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx
🧬 Code graph analysis (2)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (4)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-407)Host(1144-1317)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
useInfraEnvId(12-66)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts (1)
getHosts(71-75)libs/ui-lib/lib/common/config/constants.ts (1)
DEFAULT_POLLING_INTERVAL(13-13)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (2)
libs/ui-lib/lib/common/features/featureGate.tsx (1)
useFeature(56-59)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
useInfraEnvHosts(9-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unit-tests
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (8)
libs/ui-lib/lib/ocm/hooks/index.ts (1)
8-8: LGTM!The export follows the established pattern and correctly exposes the new hook.
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (4)
1-1: LGTM!All new imports are properly utilized in the implementation.
Also applies to: 9-9, 33-35
63-63: LGTM!Proper feature flag usage to gate the new binding functionality.
80-81: LGTM!Appropriate use of refs to track binding state across renders without triggering re-renders.
84-89: LGTM!Correctly resets the bound host tracking when the cluster changes, preventing cross-cluster binding state pollution.
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (3)
9-19: LGTM!Hook signature and return type are well-defined and properly typed.
20-28: LGTM!State initialization and hook composition are correctly implemented.
62-62: LGTM!The
isLoadingcomputation correctly handles all states: initial loading, loaded with data, and error states.
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
e59a5bb to
e6a592f
Compare
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
26-37: Previous critical bug fixed, but consider clearing hosts on error.The previously reported critical issue (hardcoded
CpuArchitecture.USE_DAY1_ARCHITECTURE) has been correctly fixed—line 34 now uses thecpuArchitectureparameter. However, whengetHostsfails, the error is set buthostsis not cleared. If hosts were previously loaded, consumers will see both stale hosts data and an error, which can be confusing.Apply this diff to clear stale hosts on error:
} catch (e) { // Invalidate this cluster's cached data InfraEnvIdsCacheService.removeInfraEnvId(clusterId, cpuArchitecture); + setHosts(undefined); setError(getErrorMessage(e)); }
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
39-56: LGTM: Effect correctly handles errors and polling.The effect properly clears stale hosts on line 41 when
infraEnvIdErroris set, addressing the previous review concern. The polling logic and cleanup are correctly implemented.Note: Polling continues indefinitely even if
getHostsrepeatedly fails. While this provides resilience for transient errors, you might consider adding a simple failure counter or backoff to reduce unnecessary API calls during persistent failures. However, the current implementation is acceptable for most use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx(2 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(2 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useLateBinding.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsxlibs/ui-lib/lib/ocm/hooks/useLateBinding.tslibs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx
🧬 Code graph analysis (5)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
libs/types/assisted-installer-service.d.ts (3)
InfraEnv(1750-1827)Host(1144-1317)Cluster(110-407)
libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx (4)
libs/ui-lib/lib/common/types/clusters.ts (1)
HostDiscoveryValues(54-57)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFormikErrorFields(34-37)libs/ui-lib/lib/common/features/featureGate.tsx (1)
useFeature(56-59)
libs/ui-lib/lib/ocm/hooks/useLateBinding.ts (1)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-407)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (4)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-407)Host(1144-1317)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
useInfraEnvId(12-66)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts (1)
getHosts(71-75)libs/ui-lib/lib/common/config/constants.ts (1)
DEFAULT_POLLING_INTERVAL(13-13)
libs/ui-lib/lib/ocm/services/HostsService.ts (2)
libs/types/assisted-installer-service.d.ts (2)
Host(1144-1317)Cluster(110-407)libs/ui-lib/lib/ocm/services/apis/index.ts (1)
HostsAPI(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: tests
- GitHub Check: lint
🔇 Additional comments (6)
libs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx (1)
81-81: The review comment misidentifies HostsDiscoveryTable's prop interface. The component only acceptscluster(as defined byHostsDiscoveryTableProps), not a separatehostsprop. Thehostsparameter derived fromcluster.hostsis extracted internally at line 93. Passing an unsupportedhostsprop would be ignored or cause a TypeScript error, not create a "dual source of truth" scenario.Likely an incorrect or invalid review comment.
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (5)
1-8: LGTM: Imports are clean and appropriate.All necessary dependencies are imported correctly.
9-15: LGTM: Hook signature is well-designed.The parameters correctly support both infraEnv resolution and host fetching.
16-24: LGTM: State initialization is correct.The hook properly initializes state and delegates infraEnv resolution to
useInfraEnvId. The previously reported issue about stale hosts wheninfraEnvIdErroris set has been addressed in the effect at line 41.
58-58: LGTM: Return statement is correct.The return signature matches the expected public API, and the
isLoadingcomputation correctly indicates the loading state.
60-60: LGTM: Export statement is correct.The current export pattern is standard and correct for default-exporting a named arrow function. The previous suggestion
export default const useInfraEnvHosts = (is syntactically invalid in JavaScript/TypeScript—you cannot combineexport defaultwithconst. The current approach is the proper way to export this hook.
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
e6a592f to
422a620
Compare
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/hooks/useLateBinding.ts (1)
54-62: Addalertsto the dependency array.The
bindHostscallback reads thealertsarray on line 35, butalertsis not included in the dependency array. This creates a stale closure where the callback uses an outdated alert list from the first render, potentially causing duplicate alert banners when binding fails on subsequent polls.Apply this diff:
}, [ infraEnvHosts, infraEnvError, infraEnvLoading, isSingleClusterFeatureEnabled, cluster.id, addAlert, removeAlert, + alerts, ]);
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
32-35: Consider clearing stale hosts on fetch error.When
getHostsfails (e.g., during polling), the error is set but previously fetched hosts remain in state, potentially showing stale data alongside an error message. While theinfraEnvIdErrorcase (line 41) correctly clears hosts, thegetHostserror case does not.Apply this diff for consistency:
} catch (e) { // Invalidate this cluster's cached data InfraEnvIdsCacheService.removeInfraEnvId(clusterId, cpuArchitecture); + setHosts(undefined); setError(getErrorMessage(e)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx(2 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(2 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useLateBinding.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/ui-lib/lib/ocm/hooks/index.ts
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
- libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsxlibs/ui-lib/lib/ocm/hooks/useLateBinding.ts
🧬 Code graph analysis (5)
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (1)
libs/types/assisted-installer-service.d.ts (1)
Host(1144-1317)
libs/ui-lib/lib/ocm/services/HostsService.ts (1)
libs/types/assisted-installer-service.d.ts (2)
Host(1144-1317)Cluster(110-407)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (4)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-407)Host(1144-1317)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
useInfraEnvId(12-66)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts (1)
getHosts(71-75)libs/ui-lib/lib/common/config/constants.ts (1)
DEFAULT_POLLING_INTERVAL(13-13)
libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx (5)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-407)libs/ui-lib/lib/common/types/clusters.ts (1)
HostDiscoveryValues(54-57)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFormikErrorFields(34-37)libs/ui-lib/lib/common/features/featureGate.tsx (1)
useFeature(56-59)
libs/ui-lib/lib/ocm/hooks/useLateBinding.ts (1)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-407)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (5)
libs/ui-lib/lib/ocm/services/HostsService.ts (2)
88-94: LGTM! Clean addition of bind method.The bind method follows the established pattern of other methods in this service, with proper validation and clear error messaging.
96-103: LGTM! Good refactor for consistency.The refactored
installAllmethod now delegates to the existinginstallmethod for each host, improving code reuse and consistency.libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx (1)
11-11: LGTM! Proper integration of late binding.The integration of
useLateBindingcorrectly blocks wizard progression while binding is in progress, and the feature flag properly controls the behavior.Also applies to: 26-26, 34-42
libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx (1)
43-46: LGTM! Clean refactor to explicit hosts prop.The addition of the
hostsprop provides better flexibility for the component to display hosts from different sources while maintaining clear data flow.Also applies to: 88-89
libs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx (1)
81-81: LGTM! Proper prop passing.The explicit
hostsprop with a safe fallback to an empty array correctly adapts to the updatedHostsDiscoveryTableinterface.
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
422a620 to
e8eab78
Compare
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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: 0
🧹 Nitpick comments (1)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
66-77: Implementation looks solid.The
bindHostmethod correctly implements the late-binding API endpoint with proper type signatures, URL construction, and payload structure matching the PR requirements.Optional: Consider removing explicit headers for consistency.
Other mutation methods in this file (
reset,installHost,update) don't explicitly set headers, as axios automatically setsContent-Type: application/jsonfor JSON payloads. Unless the bind endpoint specifically requires these headers, you could simplify to:bindHost(infraEnvId: InfraEnv['id'], hostId: Host['id'], clusterId: Cluster['id']) { return client.post<Host, AxiosResponse<Host>, { cluster_id: string }>( `${HostsAPI.makeActionsBaseURI(infraEnvId, hostId)}/bind`, { cluster_id: clusterId }, - { - headers: { - accept: 'application/json', - 'Content-Type': 'application/json', - }, - }, ); },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx(1 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(2 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useLateBinding.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
- libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx
- libs/ui-lib/lib/ocm/services/HostsService.ts
- libs/ui-lib/lib/ocm/hooks/useLateBinding.ts
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
libs/types/assisted-installer-service.d.ts (3)
InfraEnv(1750-1827)Host(1144-1317)Cluster(110-407)
🔇 Additional comments (2)
libs/ui-lib/lib/ocm/hooks/index.ts (1)
8-8: LGTM!The export follows the established pattern and is logically placed after the related
useInfraEnvhook. The addition cleanly extends the public API surface for infraEnv host-related features.libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
1-6: LGTM!The
Clusterimport is necessary for the newbindHostmethod's type signature and follows the existing import pattern.
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
e8eab78 to
da1ab3e
Compare
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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. |
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
ElayAharoni
force-pushed
the
late-binding-a-host-ot-the-cluster---ABI
branch
from
da1ab3e to
39ba7da
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
26-37: Clear stale hosts whengetHostsfails.When
getHosts()encounters an error (lines 32-36), it sets the error message but doesn't clear thehostsstate. If hosts were previously loaded, they remain visible alongside the error message, creating a confusing state.For consistency with the
infraEnvIdErrorhandling (line 41), clear hosts before setting the error:} catch (e) { // Invalidate this cluster's cached data InfraEnvIdsCacheService.removeInfraEnvId(clusterId, cpuArchitecture); + setHosts(undefined); setError(getErrorMessage(e)); }This ensures the UI doesn't display stale host data when an error occurs.
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
58-58: Consider exposing polling state separately from initial loading state.The
isLoadingflag is computed as!hosts && !error, which means it's onlytrueduring the initial load. During subsequent polling intervals,isLoadingremainsfalseeven while fetching updated data.This might be intentional to avoid UI flickering during polling, but consumers of the hook cannot distinguish between "initial load" and "refreshing data." Consider whether a separate
isRefreshingflag would benefit consumers who want to show subtle refresh indicators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/common/reducers/alertsSlice.ts(2 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx(1 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(1 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useLateBinding.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/ui-lib/lib/ocm/hooks/index.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
- libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx
- libs/ui-lib/lib/ocm/hooks/useLateBinding.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx
🧬 Code graph analysis (3)
libs/ui-lib/lib/ocm/services/HostsService.ts (1)
libs/types/assisted-installer-service.d.ts (2)
Host(1144-1317)Cluster(110-407)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (4)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-407)Host(1144-1317)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
useInfraEnvId(12-66)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts (1)
getHosts(71-75)libs/ui-lib/lib/common/config/constants.ts (1)
DEFAULT_POLLING_INTERVAL(13-13)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
libs/types/assisted-installer-service.d.ts (3)
InfraEnv(1750-1827)Host(1144-1317)Cluster(110-407)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: tests
- GitHub Check: lint
- GitHub Check: format
- GitHub Check: translation-files
- GitHub Check: unit-tests
- GitHub Check: circular-deps
🔇 Additional comments (5)
libs/ui-lib/lib/common/reducers/alertsSlice.ts (1)
9-9: LGTM! Optional alert key enables better alert management.Allowing callers to provide a custom key for alerts while maintaining the UUID fallback is a clean enhancement. This enables use cases like alert deduplication or targeted alert updates while preserving backward compatibility.
Also applies to: 26-26
libs/ui-lib/lib/ocm/services/HostsService.ts (2)
88-94: LGTM! Bind method follows established patterns.The new
bindmethod is well-structured and consistent with existing service methods (delete,reset,install). The infraEnvId validation and error handling match the established conventions.
96-103: Good refactor: delegate to service method for consistency.Changing
installAllto callthis.install(host)instead of directly invokingHostsAPI.installHostimproves consistency and maintainability by keeping all host operations within the service layer.libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts (1)
66-77: Verify necessity of explicit headers.The
bindHostmethod explicitly setsacceptandContent-Typeheaders toapplication/json(lines 71-75), while similar POST methods likereset(lines 56-59) andinstallHost(lines 61-65) don't set these headers.Please verify whether:
- These headers are required specifically for the bind endpoint
- They're redundant because the axios client already sets them globally
- Other methods should also set them for consistency
If the headers are required, consider documenting why this endpoint needs them explicitly. If they're redundant, remove them for consistency with other methods.
libs/ui-lib/lib/ocm/components/clusterConfiguration/HostInventory.tsx (1)
81-81: LGTM! Explicit hosts prop supports late binding workflow.Passing
hosts={cluster.hosts || []}explicitly toHostsDiscoveryTablealigns with the late binding feature where hosts may be fetched from infraEnv and bound to the cluster dynamically. The empty array fallback is safe.
|
@ElayAharoni: This pull request references AGENT-1351 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 sub-task to target the "4.21.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
♻️ Duplicate comments (2)
libs/ui-lib/lib/ocm/hooks/useLateBinding.ts (1)
9-69: FixisBindinglifecycle and includealertsin hook dependenciesTwo behavioural issues in
bindHosts:
isBindingis toggled inside the per-host loop, so after the first host finishes the flag can flip back tofalsewhile other hosts are still being bound. Anything gating on this value (wizard progression, buttons, etc.) can incorrectly become enabled mid-operation.The callback closes over
alertsbutalertsis not listed in the dependency array, so it may use a stale alert list and fail to remove existing “Failed to bind host …” alerts, causing duplicates on subsequent binding attempts. This was already raised in an earlier review and remains unresolved.You can address both with something like:
- const bindHosts = useCallback(async () => { - if (infraEnvHosts && !infraEnvError && !infraEnvLoading && isSingleClusterFeatureEnabled) { - for (const host of infraEnvHosts) { - if (host.clusterId !== cluster.id) { - setIsBinding(true); - try { - const alertKey = alerts.find((alert) => alert.key === host.id)?.key; - if (alertKey) { - removeAlert(alertKey); - } - await HostsService.bind(host, cluster.id); - } catch (error) { - addAlert({ - title: `Failed to bind host ${host.requestedHostname || ''}`, - message: getErrorMessage(error), - key: host.id, - }); - } finally { - setIsBinding(false); - } - } - } - } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [ - infraEnvHosts, - infraEnvError, - infraEnvLoading, - isSingleClusterFeatureEnabled, - cluster.id, - addAlert, - removeAlert, - ]); + const bindHosts = useCallback(async () => { + if (infraEnvHosts && !infraEnvError && !infraEnvLoading && isSingleClusterFeatureEnabled) { + setIsBinding(true); + try { + for (const host of infraEnvHosts) { + if (host.clusterId !== cluster.id) { + try { + const alertKey = alerts.find((alert) => alert.key === host.id)?.key; + if (alertKey) { + removeAlert(alertKey); + } + await HostsService.bind(host, cluster.id); + } catch (error) { + addAlert({ + title: `Failed to bind host ${host.requestedHostname || ''}`, + message: getErrorMessage(error), + key: host.id, + }); + } + } + } + } finally { + setIsBinding(false); + } + } + }, [ + infraEnvHosts, + infraEnvError, + infraEnvLoading, + isSingleClusterFeatureEnabled, + cluster.id, + addAlert, + removeAlert, + alerts, + ]);This keeps
isBindingtrue for the duration of the entire bind pass and ensures the callback tracks the currentalertsarray.libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (1)
9-59: Avoid stalehosts/errorstate when polling infra-env hostsThe core flow looks good, but the current error handling leaves some inconsistent states:
- When
InfraEnvsAPI.getHostsfails,erroris set buthostsis left untouched, so callers may see an error message while still rendering a stale host list.- If a later poll succeeds,
erroris never cleared, so a transient failure leaves a permanent error even after recovery.Earlier feedback already suggested clearing hosts on error; that piece is still missing here.
You can tighten this up by resetting
erroron success and clearinghostson failure:const getHosts = React.useCallback(async () => { try { if (infraEnvId) { const { data: hostsData } = await InfraEnvsAPI.getHosts(infraEnvId); setHosts(hostsData); + setError(''); } } catch (e) { // Invalidate this cluster's cached data - InfraEnvIdsCacheService.removeInfraEnvId(clusterId, cpuArchitecture); - setError(getErrorMessage(e)); + InfraEnvIdsCacheService.removeInfraEnvId(clusterId, cpuArchitecture); + setHosts(undefined); + setError(getErrorMessage(e)); } }, [clusterId, cpuArchitecture, infraEnvId]);This keeps the tri-state (
hosts,error,isLoading) coherent for consumers of the hook.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts(2 hunks)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts(2 hunks)libs/ui-lib/lib/common/reducers/alertsSlice.ts(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx(1 hunks)libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx(1 hunks)libs/ui-lib/lib/ocm/hooks/index.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts(1 hunks)libs/ui-lib/lib/ocm/hooks/useLateBinding.ts(1 hunks)libs/ui-lib/lib/ocm/services/HostsService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/ui-lib/lib/ocm/components/clusterWizard/HostDiscovery.tsx
- libs/ui-lib/lib/common/api/assisted-service/HostsAPI.ts
- libs/ui-lib/lib/ocm/components/hosts/HostsDiscoveryTable.tsx
- libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts
🧰 Additional context used
🧬 Code graph analysis (3)
libs/ui-lib/lib/ocm/hooks/useInfraEnvHosts.ts (4)
libs/types/assisted-installer-service.d.ts (2)
Cluster(110-407)Host(1144-1317)libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
useInfraEnvId(12-66)libs/ui-lib/lib/common/api/assisted-service/InfraEnvsAPI.ts (1)
getHosts(71-75)libs/ui-lib/lib/common/config/constants.ts (1)
DEFAULT_POLLING_INTERVAL(13-13)
libs/ui-lib/lib/ocm/hooks/useLateBinding.ts (1)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-407)
libs/ui-lib/lib/ocm/services/HostsService.ts (1)
libs/types/assisted-installer-service.d.ts (2)
Host(1144-1317)Cluster(110-407)
🔇 Additional comments (3)
libs/ui-lib/lib/common/reducers/alertsSlice.ts (1)
5-10: LGTM! Clean infrastructure for custom alert identification.The optional
keyfield enables callers to provide custom identifiers for alerts, which is useful for deduplication or targeted alert management.libs/ui-lib/lib/ocm/services/HostsService.ts (1)
2-7: Newbindhelper is consistent with existing host operationsThe
bind(host, clusterId)implementation follows the same infraEnvId guard pattern asupdate,delete,reset, andinstall, and delegates cleanly toHostsAPI.bindHost. The newClusterimport keeps the typing precise. No issues from this service layer’s perspective.Also applies to: 88-94
libs/ui-lib/lib/ocm/hooks/index.ts (1)
8-8: Re-export ofuseInfraEnvHostslooks correctThe new export wires the hook into the existing hooks barrel file with the expected naming and path, so consumers can import it uniformly.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElayAharoni, rawagner 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-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit 4b6c3bf
into
openshift-assisted:master
* Remove preview badge from bundles card (#3169) * Migration to PF6 (#3168) * pf6 deps and codemods updates Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> get emptystate pf6 issues building Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> fix chip/label changes Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> fix icon type Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> fix a prop type that codemods missed Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> update pf5 classnames Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> fix tokens, some cleanup Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> fix tests and remaining v5->v6 Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> yarn lock Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> work on some cy tests Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> fix tests for pf6 changes Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> fix some tests Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> fix build error Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * Change version of package.json to 2.0.0 for testing * MGMT-20974: Rremove unnecessary hyphens in helper text operators * MGMT-20968: align 'learn more about openshift releases' link in the first page of the cluster creation wizard * MGMT-20965: Put the same min-width in the dropdowns of the first page of the cluster creation wizard * MGMT-20966: improving the readibility of text on Troubleshooter Modal * MGMT-20972: Cluster summary section is not collapsable * MGMT-20964: Detached Warning Message for TMPv2 selection * Changes in PrismCode to use the correct color * MGMT-20969: hostname column header is truncated and unredeable * MGMT-20967: errors beneath text boxes dissapears but icon remains in place * fix (20975): make bundle selected operators show in count Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * MGMT-21103: change size of Provisioning type droodown in Add hosts modal * MGMT-21105: Networking page-machine network dropdown truncate values * add layout with gutter around cluster progress/buttons Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * MGMT-21147: R&C page - missing vertical spacing between kubeconfig warning and buttons * MGMT-21151: inconsistend icon and text alignment in preflight checks in R&C page * Add data-test-id to MenuToggle components * Add data-testid to HelperTextItem components * Add data-testid to BreadCrumb component * Add data-testid to Spinner component * Add data-testid to Wizard components * Add data-testid to Host's network configuration group * Add data-testid to Use bond option * Add data-testid to Add hosts modal * Add more data-testids for tests * Solving problem with operators count * Remove duplicated scrollbars * Solving errors in Networking page * Add data-testid to close button in Events modal * Migration of TextContent component * Solving problems with some old components * Solving lint issues * Solving problems with unit tests * Solving format issues * Remove unnecessary test * Solving problems with tests --------- Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> Co-authored-by: gitdallas <5322142+gitdallas@users.noreply.github.com> * Use masthead (#3173) * Button text (#3174) * OCPBUGS-61953: Update dependency sourcing to remote (#3155) * Bump axios from 1.6.8 to 1.12.2 (#3180) Bumps [axios](https://github.com/axios/axios) from 1.6.8 to 1.12.2. - [Release notes](https://github.com/axios/axios/releases) - [Changelog](https://github.com/axios/axios/blob/v1.x/CHANGELOG.md) - [Commits](axios/axios@v1.6.8...v1.12.2) --- updated-dependencies: - dependency-name: axios dependency-version: 1.12.2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Updating assisted-installer-ui-container image to be consistent with ART for 4.21 (#3179) Reconciling with https://github.com/openshift/ocp-build-data/tree/4fbe3fab45239dc4be6f5d9d98a0bf36e0274ec9/images/agent-installer-ui.yml Co-authored-by: AOS Automation Release Team <noreply@redhat.com> * OCPBUGS-61787: Change NMstate operator docs link (#3182) * MGMT-21862: Add message about vSphere limitations (#3181) * OCPBUGS-62680: Include assisted disconnected UI image in release payload (#3188) * Fixing errors when creating cluster from Assisted Migration (#3191) * MGMT-21025: installing Two Node OpenShift with Arbiter (TNA) (#3170) * Clean up ClusterDetailsFormFields * Allow TNA arbiter in CIM * Remove unused component (#3193) * Add unique data-testid to dualstack subnet dropdowns (#3199) * Tweak spacing between advanced network fields (#3198) * Tweak host status spacing (#3200) * Improve alert spacing (#3201) * MGMT-21825: Textarea field should show both error and helper text (#3202) * Improve alert spacing * Show both error and helper text under textarea fields * Add border to table headers (#3197) * Remove border from rich input field (#3208) * Bump happy-dom from 15.10.2 to 20.0.0 (#3211) Bumps [happy-dom](https://github.com/capricorn86/happy-dom) from 15.10.2 to 20.0.0. - [Release notes](https://github.com/capricorn86/happy-dom/releases) - [Commits](capricorn86/happy-dom@v15.10.2...v20.0.0) --- updated-dependencies: - dependency-name: happy-dom dependency-version: 20.0.0 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Remove nested 'Content' components (#3196) * Upgrade the PF modal component in /common and /ocm (#3213) * Bump happy-dom from 20.0.0 to 20.0.2 (#3218) Bumps [happy-dom](https://github.com/capricorn86/happy-dom) from 20.0.0 to 20.0.2. - [Release notes](https://github.com/capricorn86/happy-dom/releases) - [Commits](capricorn86/happy-dom@v20.0.0...v20.0.2) --- updated-dependencies: - dependency-name: happy-dom dependency-version: 20.0.2 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Consistent ISO Download behavior (#3221) * Add 'noopener noreferrer' to ISO download button (#3222) * Fix preflight check collapsed styling (#3215) * Remove unnecessary custom manifest field (#3217) * Bump vite from 5.4.20 to 5.4.21 (#3223) Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 5.4.20 to 5.4.21. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/v5.4.21/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v5.4.21/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-version: 5.4.21 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * MGMT-21625: Add Dual-Stack with Primary IPv6 Support (#3190) This PR adds support for IPv6-primary dual-stack clusters, enabling users to create dual-stack clusters where IPv6 is the primary IP stack (version-aware for OpenShift 4.12+) * sort OCP versions in create infra (#3171) Signed-off-by: Lior Soffer <liorsoffer1@gmail.com> * Update types/parsing for latest lightspeed API (#3226) * Update release job to use trusted-publishers flow (#3228) * Update setup-node config (#3232) * - MGMT-22057:Primary IPv6 should be Tech Preview (#3231) - MGMT-22045: Change style of Dualstack - IPv4 / IPv6 section headers in subnet dropdown - When we change the primary machine network IP to ipv6 we have to select the seconday machine network IP to ipv4 * MGMT-22080: Allow users to install Openshift AI as standalone operator in SNO clusters (#3234) * Release job: Configure Yarn for npm registry (#3235) * configure yarn before publish (#3239) * Pass OIDC token to yarn config (#3241) * MGMT-22047: Add apiVIP and ingressVIP for dual-stack ipv4 and ipv6 (#3246) * Add apiVIP and ingressVIP for dual-stack ipv4 and ipv6 * Add validations to ensure that users add primary and secondary apiVips and ingressVips when using dual-stack * Updating tests to dual-stack changes * Prevent InfraEnv creation on ABI to support OVE Late-Binding workflow (#3244) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * MGMT-22116: Add info about apiVIPs and ingressVIPs for dual-stack in Review and Create page (#3250) * Add info about apiVIPs and ingressVIPs for dual-stack in Review and Create page * Fixing review comments * MGMT-22119: Solving errors with SNO dual-stack (#3249) * Solving errors with SNO dual-stack * Solving errors when change between dual-stack and single-stack * MGMT-22165: Remove what's new link until the info appear in the chatbot (#3253) * Update logic to extract the tool response (#3255) * MGMT-17220: dual-stack with second machine network not populates (#3256) * MGMT-17220: dual-stack with second machine network not populates * Improving the code * Make the parsing compatible with new & old API (#3260) * MGMT-22047: solving errors with dual-stack (#3263) * MGMT-22047: solving errors with dual-stack 1. Changes in AvailableSubnetsControl.tsx so our corrective setFieldValue calls don’t validate immediately. This avoids heavy, repeated validations the moment you pick the first IPv6 option and should stop the freeze when secondary VIPs are IPv6 and empty. Specifically, set the third arg to false for: - Auto-select initialization of machineNetworks - Duplicate-primary fix that updates machineNetworks.1.cidr 2. Changes in AdvancesNetworkFields.tsx: -I found the infinite loop in AdvancedNetworkFields.tsx: when the primary machine network flips to IPv6, the effect was swapping clusterNetworks/serviceNetworks even when both entries had the same IP family, causing continuous reorders and a freeze. -I added guards so we only swap when both entries exist and have opposite families, and the first one mismatches the primary machine family. Lint is clean. * Solving issues in code * MGMT-21837: in YAML view in Static Network Configuration we add the radio buttons to change the form to another view (#3268) * late binding hosts to cluster in ABI (#3259) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * AGENT-1373: Rename feature gate to NoRegistryClusterInstall (#3267) Update the feature gate name from NoRegistryClusterOperations to NoRegistryClusterInstall. * AGENT-1352: Handle cluster reset with late binding (#3270) * late binding hosts to cluster in ABI Signed-off-by: Elay Aharoni <elayaha@gmail.com> * Handle cluster reset on ABI Signed-off-by: Elay Aharoni <elayaha@gmail.com> --------- Signed-off-by: Elay Aharoni <elayaha@gmail.com> * MGMT-22281: Dual stack seconday vips fields not mandatory (#3275) * Show TechPreview badge only in Primary Machine network when user chooses ipV6 IP (#3276) * add new fields to above sea level ABI (#3274) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * Revert "Handle cluster reset on ABI" (#3281) This reverts commit d35b70a. * add loki and logging operators (#3285) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * OCPBUGS-65657: Display OpenShift AI GPU validation message from API (#3279) Fix UI to show the friendly label and detailed message from the API instead of displaying the technical validation ID when OpenShift AI operator is selected without GPU support. * Bug fix: OVE Agent Installer UI: Red Hat OCP logo not displaying correctly (#3289) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * Adding TechPreview Budge for Assisted installer and agent (#3293) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * MGMT-20138 : Show 'Add hosts' tab for all cases (#3297) * Edit OWNERS file (#3172) * Bump js-yaml from 4.1.0 to 4.1.1 (#3264) Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 4.1.0 to 4.1.1. - [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md) - [Commits](nodeca/js-yaml@4.1.0...4.1.1) --- updated-dependencies: - dependency-name: js-yaml dependency-version: 4.1.1 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump zx from 7.2.3 to 8.8.5 (#3273) Bumps [zx](https://github.com/google/zx) from 7.2.3 to 8.8.5. - [Release notes](https://github.com/google/zx/releases) - [Commits](google/zx@7.2.3...8.8.5) --- updated-dependencies: - dependency-name: zx dependency-version: 8.8.5 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump mdast-util-to-hast from 13.2.0 to 13.2.1 (#3287) Bumps [mdast-util-to-hast](https://github.com/syntax-tree/mdast-util-to-hast) from 13.2.0 to 13.2.1. - [Release notes](https://github.com/syntax-tree/mdast-util-to-hast/releases) - [Commits](syntax-tree/mdast-util-to-hast@13.2.0...13.2.1) --- updated-dependencies: - dependency-name: mdast-util-to-hast dependency-version: 13.2.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * provides parameters for the GET /v2/operators/bundles route (#3306) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * Bump js-yaml from 4.1.0 to 4.1.1 (#3310) Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 4.1.0 to 4.1.1. - [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md) - [Commits](nodeca/js-yaml@4.1.0...4.1.1) --- updated-dependencies: - dependency-name: js-yaml dependency-version: 4.1.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fix Cluster summary styling (#3216) * remove external platforms field from below sea level UI (#3316) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * Openshift AI Bundle on SNO enables ODF and LVM which are uncompatible (#3320) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * change ABI above sea level iso size (#3322) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * MGMT-22465: Merge the contents of releases/v2.17-cim into master (#3319) * Fix patches when there were 0 nodes in hypershift nodepool (#3177) * Make OVN the default network type if the version is invalid (#3183) * MGMT-20076: Support external platform in Assisted-installer Kube API (#3151) * Add External platforms field * Restructure files related to CIM cluster deployment wizard * Create SelectFieldWithSearch component * Update '@openshift-console/dynamic-plugin-sdk' * CIM custom manifest step * CIM custom manifests review * Make 'Baremetal' the default external platform * Translations for 2.16-cim (#3225) * MGMT-21025: installing Two Node OpenShift with Arbiter (TNA) (#3224) * Clean up ClusterDetailsFormFields * Allow TNA arbiter in CIM * Set 'userManagedNetworking' as true with 'external' platform (#3245) * Tweak TNA-related strings in CIM (#3248) * Restrict platform options for SNO clusters (#3262) Signed-off-by: Lior Soffer <liorsoffer1@gmail.com> * Fix: Undefined OpenShift version producing an incorrect documentation link (#3271) Signed-off-by: Lior Soffer <liorsoffer1@gmail.com> * Fix bug - the option to remove host from the cluster disabled while the host is installing (#3219) Signed-off-by: Elay Aharoni <elayaha@gmail.com> * Implement user interface for the multiple SSH keys (#3292) Signed-off-by: Lior Soffer <liorsoffer1@gmail.com> * Edit OWNERS file (#3303) Co-authored-by: jgyselov <jgyselov@redhat.com> * Fix arbiter translation (#3304) Co-authored-by: jgyselov <jgyselov@redhat.com> * [releases/v2.17-cim] MGMT-22264: Add 'Labels' and 'GPU' columns to infra env host table (#3307) * Add GPUs column to infra agent table * Add labels column and filtering to infra agent table --------- Co-authored-by: jgyselov <jgyselov@redhat.com> * Prevent mass approve crash when hosts are still discovering (#3305) Co-authored-by: jgyselov <jgyselov@redhat.com> * Fix 'Required' translations (#3308) Co-authored-by: jgyselov <jgyselov@redhat.com> * Do not exclude hosts with SpecSyncError status from host selection during binding (#3309) Co-authored-by: jgyselov <jgyselov@redhat.com> * MGMT-22438: Handle empty labels in infra env host table (#3314) * Handle empty labels in infra env host table Signed-off-by: Lior Soffer <liorsoffer1@gmail.com> * format fix Signed-off-by: Lior Soffer <liorsoffer1@gmail.com> --------- Signed-off-by: Lior Soffer <liorsoffer1@gmail.com> * MGMT-19743: Icon from agent status available is confusing (#3220) * Fix bug - the option to remove host from the cluster disabled while the host is installing Signed-off-by: Elay Aharoni <elayaha@gmail.com> * icon from agent status Available is confusing Signed-off-by: Elay Aharoni <elayaha@gmail.com> --------- Signed-off-by: Elay Aharoni <elayaha@gmail.com> Co-authored-by: Julie Gyselova <jgyselov@redhat.com> --------- Signed-off-by: Lior Soffer <liorsoffer1@gmail.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com> Co-authored-by: Lior Soffer <liorsoffer1@gmail.com> Co-authored-by: Elay Aharoni <elayaha@gmail.com> Co-authored-by: OpenShift Cherrypick Robot <openshift-cherrypick-robot@redhat.com> --------- Signed-off-by: gitdallas <5322142+gitdallas@users.noreply.github.com> Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Lior Soffer <liorsoffer1@gmail.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com> Co-authored-by: Montse Ortega Gallart <ammont82@users.noreply.github.com> Co-authored-by: gitdallas <5322142+gitdallas@users.noreply.github.com> Co-authored-by: Pawan Pinjarkar <ppinjark@redhat.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: OpenShift Bot <openshift-bot@redhat.com> Co-authored-by: AOS Automation Release Team <noreply@redhat.com> Co-authored-by: Linoy Hadad <Linoyaslan@gmail.com> Co-authored-by: Lior Soffer <liorsoffer1@gmail.com> Co-authored-by: Rastislav Wagner <rawagner@redhat.com> Co-authored-by: Elay Aharoni <elayaha@gmail.com> Co-authored-by: Richard Su <rwsu@redhat.com> Co-authored-by: Yoav Schwammenthal <33420608+yoavsc0302@users.noreply.github.com> Co-authored-by: OpenShift Cherrypick Robot <openshift-cherrypick-robot@redhat.com>
3 participants
https://issues.redhat.com/browse/AGENT-1351
added the functionality for when a host is added to the infraEnv in ABI, it will be associated to the cluster created in the assisted installer and will be shown on the hosts table.
using this API:
curl -X POST
http://<RENDEZVOUS_IP>:3001/api/assisted-install/v2/infra-envs/<INFRA_ENV_ID>/hosts/<HOST_ID>/actions/bind
-H 'accept: application/json'
-H 'Content-Type: application/json'
-d "{"cluster_id":<CLUSTER_ID>}"
and polling the infraEnv hosts in interval to detect when a new host is genrated
Summary by CodeRabbit