Revert abi new flow for 2.49#3353
Revert abi new flow for 2.49#3353openshift-merge-bot[bot] merged 4 commits intoopenshift-assisted:releases/v2.49from
Conversation
This reverts commit 41a3210.
…hnology Preview spinner (openshift-assisted#3341)" This reverts commit d47ab5f.
📝 WalkthroughWalkthroughThis PR removes the disconnected-cluster feature surface: API/service registration methods, the optional-configurations wizard step and component, disconnected-related context/state and exports, related routing/footer props, and simplifies disconnected wizard steps to navigate without API calls. Additional small UI and validation tweaks included. Changes
Sequence Diagram(s)(omitted — changes are multiple-component but mainly removals/simplifications of internal flows; no new multi-component control flow introduced that benefits from a sequence diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
openshift-ci
bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-lib/lib/ocm/services/InfraEnvsService.ts (1)
14-33: Verify cache integrity whenisSingleClusterFeatureEnabled=true.The
isSingleClusterFeatureEnabledparameter is still actively passed to this function (e.g., fromuseInfraEnvIdhook) and the feature remains enabled in production code. When true, this creates a cache corruption risk:
- Line 22 calls the API with an empty string instead of
clusterId- The API may return infraEnvs not specific to the provided
clusterId- Line 25 caches these potentially unrelated infraEnvs under the current
clusterIdThis could cause incorrect infraEnv retrieval for affected clusters. The removed guard (
clusterId &&check before updating cache) was protecting against this scenario.Since this is a revert PR removing the disconnected cluster feature:
- Can
isSingleClusterFeatureEnabledstill betrueafter this revert?- If yes, restore the
clusterId &&guard on line 24 before caching- If no, remove the unused parameter entirely
🤖 Fix all issues with AI agents
In @libs/ui-lib/lib/common/components/ui/WizardFooter.tsx:
- Line 72: WizardFooter's Next button disabled logic was changed to use
isNextDisabled alone, which lets some callers (e.g., Day2StaticIP) allow clicks
while isSubmitting is true; ensure all callers that pass an isSubmitting flag
include it in their isNextDisabled calculation and/or have WizardFooter re-check
it. Update callers such as Day2StaticIP to incorporate isSubmitting into their
isNextDisabled (same pattern used by NetworkConfigurationForm), and if you
prefer a centralized safety net, restore a logical OR in WizardFooter (check
isNextDisabled || isSubmitting) so the Next button is always disabled during
submission; also verify the Reset button continues to use isSubmitting for its
disabled state and that isNextButtonLoading remains used only where loading UI
is required (CredentialsDownload).
In @libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx:
- Around line 33-34: Change the hardcoded downloadUrl constant to use the
correct ISO filename "agent.x86_64.iso" instead of "agent-ove.x86_64.iso"
(update the downloadUrl string in ReviewStep.tsx), remove the redundant
conditional guard `downloadUrl && saveAs(downloadUrl)` and call
`saveAs(downloadUrl)` directly (since downloadUrl is a constant), and make the
URL configurable by reading it from configuration or an environment variable
(falling back to the corrected default) so future mirror/version changes can be
managed without code edits.
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx (1)
53-53: Remove redundant conditional check.The check
downloadUrl &&is unnecessary sincedownloadUrlis a constant defined in this file (line 33) and will always be truthy.♻️ Simplify the download logic
- downloadUrl && saveAs(downloadUrl); + saveAs(downloadUrl);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
libs/ui-lib/lib/common/api/assisted-service/ClustersAPI.tslibs/ui-lib/lib/common/components/ui/WizardFooter.tsxlibs/ui-lib/lib/common/components/ui/formik/PullSecretField.tsxlibs/ui-lib/lib/ocm/components/Routes.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/VirtualIPControlGroup.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardFooter.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/constants.tslibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/index.tslibs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.tslibs/ui-lib/lib/ocm/hooks/useInfraEnvId.tslibs/ui-lib/lib/ocm/services/ClustersService.tslibs/ui-lib/lib/ocm/services/InfraEnvsService.ts
💤 Files with no reviewable changes (10)
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/VirtualIPControlGroup.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx
- libs/ui-lib/lib/ocm/services/ClustersService.ts
- libs/ui-lib/lib/common/api/assisted-service/ClustersAPI.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/index.ts
- libs/ui-lib/lib/common/components/ui/formik/PullSecretField.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts:69-69
Timestamp: 2025-10-19T17:22:52.502Z
Learning: CIM UI changes in the repository openshift-assisted/assisted-installer-ui (e.g., files under libs/ui-lib/lib/cim/) are handled separately by the CIM team and should be tracked via separate issues rather than being included in PRs for other UI components.
📚 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/ClusterWizardFooter.tsxlibs/ui-lib/lib/ocm/hooks/useInfraEnvId.tslibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsxlibs/ui-lib/lib/ocm/components/Routes.tsx
📚 Learning: 2025-12-17T09:08:18.279Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3319
File: libs/ui-lib/lib/cim/components/helpers/toAssisted.ts:199-201
Timestamp: 2025-12-17T09:08:18.279Z
Learning: In libs/ui-lib/lib/cim/components/helpers/toAssisted.ts, the platformType field on agentClusterInstall.spec is guaranteed to always contain a valid PlatformType value (when lowercased), making the type assertion safe without additional runtime validation.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardFooter.tsx
🧬 Code graph analysis (2)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardFooter.tsx (2)
libs/ui-lib/lib/common/features/featureGate.tsx (1)
useFeature(56-59)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)
🔇 Additional comments (5)
libs/ui-lib/lib/ocm/hooks/useInfraEnvId.ts (1)
58-63: Guard removal is justified and safe—the service method properly handles both enabled and disabled flag states.The
InfraEnvsService.getInfraEnvId()method accepts theisSingleClusterFeatureEnabledparameter as optional and uses it to control API behavior (passing either the clusterId or empty string to filter results), not to gate execution. Since the flag is still passed to the service on line 28, the method can handle both cases. The dependency array change is also correct—the flag is captured indirectly throughfindInfraEnvId's dependencies, so no duplication is needed.libs/ui-lib/lib/ocm/components/Routes.tsx (1)
39-39: No issues found with this route change.The route structure correctly defines
~newas a sibling route underassisted-installer/clusters, and all navigation references in the codebase use only the~newpath. The change aligns with the PR's objective to revert the ABI new flow.libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (1)
32-32: LGTM! Clean simplification of the wizard flow.The removal of async registration logic and direct use of
moveNextproperly simplifies the wizard step progression, aligning with the PR's goal of reverting the disconnected cluster feature.libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardFooter.tsx (1)
85-95: LGTM! Handlers properly simplified.The removal of disconnect-related API calls and state management, along with the streamlined cancel and reset handlers, correctly aligns with removing the disconnected cluster flows. The direct navigation and dialog invocation patterns are clean and maintainable.
libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts (1)
34-34: LGTM! Wizard step definitions properly updated.The removal of
'disconnected-optional-configurations'from the type union (line 34) and thedisconnectedStepsarray (lines 50-53) correctly simplifies the wizard flow. These changes are consistent with the PR's objective to revert the optional configurations feature.Also applies to: 50-53
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
ElayAharoni
force-pushed
the
revert-ABI-new-flow-for-2.49
branch
from
a177a99 to
ca3e8cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx (2)
52-55: Remove redundant conditional guard.Since
downloadUrlis a constant string literal, thedownloadUrl &&check is unnecessary—it will always be truthy.♻️ Suggested simplification
onNext={() => { - downloadUrl && saveAs(downloadUrl); + saveAs(downloadUrl); navigate('/cluster-list'); }}
98-111: Consider consistency between displayed values and downloadUrl.The description list shows hardcoded values (OpenShift version "4.20", architecture "x86_64", ISO size "approx. 43.5GB") that should stay in sync with the
downloadUrl. Currently they appear consistent (URL shows4.20.8/...x86_64.iso), but if the URL changes, these values could drift.Acceptable for this revert PR, but consider deriving these from a shared configuration in the future.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib/lib/common/config/constants.tslibs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
💤 Files with no reviewable changes (1)
- libs/ui-lib/lib/common/config/constants.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts:69-69
Timestamp: 2025-10-19T17:22:52.502Z
Learning: CIM UI changes in the repository openshift-assisted/assisted-installer-ui (e.g., files under libs/ui-lib/lib/cim/) are handled separately by the CIM team and should be tracked via separate issues rather than being included in PRs for other UI components.
📚 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/disconnected/ReviewStep.tsx
📚 Learning: 2025-09-02T08:03:57.204Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3151
File: libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts:25-25
Timestamp: 2025-09-02T08:03:57.204Z
Learning: In the openshift-assisted/assisted-installer-ui repository, non-English translations are handled separately from the main development process. When adding new translation keys to English locale files, it's expected that non-English locale files (es, fr, ja, ko, zh) may not immediately contain the corresponding translations, as these are managed through a different workflow.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
🧬 Code graph analysis (1)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)
🔇 Additional comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx (2)
29-29: LGTM on import change.The switch to
useNavigatefromreact-router-dom-v5-compatis consistent with the simplified navigation flow and aligns with React Router v6 patterns.
36-39: LGTM on simplified context usage.The component now only needs
moveBackfrom the wizard context, which aligns with the PR's goal of removing the complex deregistration workflow. Clean and focused.
openshift-ci
bot
added
lgtm
openshift-ci
bot
removed
the
approved
|
/retest |
|
/override ci/prow/okd-scos-images |
|
@rawagner: Overrode contexts on behalf of rawagner: ci/prow/okd-scos-images 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. |
openshift-ci
bot
added
the
approved
|
[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-merge-bot
bot
merged commit 264b50c
into
openshift-assisted:releases/v2.49
2 participants
Summary by CodeRabbit
Features Removed
Bug Fixes
UX Changes
✏️ Tip: You can customize this high-level summary in your review settings.