MGMT-20743 | [Staging] [UI] - Machine network CIDR value should be mandatory#3122
MGMT-20743 | [Staging] [UI] - Machine network CIDR value should be mandatory#3122asmasarw wants to merge 8 commits intoopenshift-assisted:masterfrom
Conversation
WalkthroughAdds a disconnected cluster-wizard flow (new steps, UI steps, and step mapping), exposes installDisconnected state in the wizard context/provider, moves wizard navigation (moveNext/moveBack) to function properties in context, updates ClusterDetails/ClusterDetailsForm to use context-sourced navigation and a disconnected Switch, refactors OcmOpenShiftVersion to accept optional props and children, and tightens a prefixLength Yup validation to disallow null. Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Welcome @asmasarw! It looks like this is your first PR to openshift-assisted/assisted-installer-ui 🎉 |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @asmasarw. Thanks for your PR. I'm waiting for a openshift-assisted member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
bot
added
the
size/S
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/FormViewNetworkWide/FormViewNetworkWideFields.tsx (1)
127-127: Nice wiring. Consider adding step=1 to enforce integer-only UX at the input level.This complements the validator and reduces the chance of entering decimals via the UI.
Outside the selected lines, update the Prefix Length input:
<OcmInputField name={`${fieldName}.prefixLength`} isRequired={true} data-testid={`${protocolVersion}-machine-network-prefix-length`} type={TextInputTypes.number} showErrorMessage={false} min={MIN_PREFIX_LENGTH} max={protocolVersion === ProtocolVersion.ipv4 ? MAX_PREFIX_LENGTH.ipv4 : MAX_PREFIX_LENGTH.ipv6} step={1} // enforce integer increments validate={handleValidatePrefixLength} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/FormViewNetworkWide/FormViewNetworkWideFields.tsx(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/FormViewNetworkWide/FormViewNetworkWideFields.tsx (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/FormViewNetworkWide/formViewNetworkWideValidationSchema.tsx (2)
MIN_PREFIX_LENGTH(26-26)MAX_PREFIX_LENGTH(27-30)
| const handleValidatePrefixLength = (value: string) => { | ||
| if (!value || value === '') { | ||
| return 'Prefix length is required'; | ||
| } | ||
| if (Number(value) < MIN_PREFIX_LENGTH) { | ||
| return `Prefix length must be at least ${MIN_PREFIX_LENGTH}`; | ||
| } | ||
| if (Number(value) > MAX_PREFIX_LENGTH[protocolVersion]) { | ||
| return `Prefix length must be at most ${MAX_PREFIX_LENGTH[protocolVersion]}`; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden prefix-length validation: handle NaN/decimals, trim input, and avoid enum-index pitfalls (also memoize).
Current checks allow non-numeric (e.g., "e") or decimal values (e.g., "24.5") to pass because Number('e') is NaN (comparisons are false) and 24.5 satisfies the min/max comparisons. Also, indexing MAX_PREFIX_LENGTH[protocolVersion] assumes protocolVersion is a string key; elsewhere you use a ternary for safety. Proposal below fixes all and uses a stable callback.
- const handleValidatePrefixLength = (value: string) => {
- if (!value || value === '') {
- return 'Prefix length is required';
- }
- if (Number(value) < MIN_PREFIX_LENGTH) {
- return `Prefix length must be at least ${MIN_PREFIX_LENGTH}`;
- }
- if (Number(value) > MAX_PREFIX_LENGTH[protocolVersion]) {
- return `Prefix length must be at most ${MAX_PREFIX_LENGTH[protocolVersion]}`;
- }
- return undefined;
- }
+ const handleValidatePrefixLength = React.useCallback((input: string | number) => {
+ const raw = String(input ?? '');
+ const trimmed = raw.trim();
+ if (!trimmed) {
+ return 'Prefix length is required';
+ }
+
+ const num = Number(trimmed);
+ if (!Number.isFinite(num)) {
+ return 'Prefix length must be a number';
+ }
+ if (!Number.isInteger(num)) {
+ return 'Prefix length must be an integer';
+ }
+
+ if (num < MIN_PREFIX_LENGTH) {
+ return `Prefix length must be at least ${MIN_PREFIX_LENGTH}`;
+ }
+ const max =
+ protocolVersion === ProtocolVersion.ipv4
+ ? MAX_PREFIX_LENGTH.ipv4
+ : MAX_PREFIX_LENGTH.ipv6;
+ if (num > max) {
+ return `Prefix length must be at most ${max}`;
+ }
+ return undefined;
+ }, [protocolVersion]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleValidatePrefixLength = (value: string) => { | |
| if (!value || value === '') { | |
| return 'Prefix length is required'; | |
| } | |
| if (Number(value) < MIN_PREFIX_LENGTH) { | |
| return `Prefix length must be at least ${MIN_PREFIX_LENGTH}`; | |
| } | |
| if (Number(value) > MAX_PREFIX_LENGTH[protocolVersion]) { | |
| return `Prefix length must be at most ${MAX_PREFIX_LENGTH[protocolVersion]}`; | |
| } | |
| return undefined; | |
| } | |
| const handleValidatePrefixLength = React.useCallback((input: string | number) => { | |
| const raw = String(input ?? ''); | |
| const trimmed = raw.trim(); | |
| if (!trimmed) { | |
| return 'Prefix length is required'; | |
| } | |
| const num = Number(trimmed); | |
| if (!Number.isFinite(num)) { | |
| return 'Prefix length must be a number'; | |
| } | |
| if (!Number.isInteger(num)) { | |
| return 'Prefix length must be an integer'; | |
| } | |
| if (num < MIN_PREFIX_LENGTH) { | |
| return `Prefix length must be at least ${MIN_PREFIX_LENGTH}`; | |
| } | |
| const max = | |
| protocolVersion === ProtocolVersion.ipv4 | |
| ? MAX_PREFIX_LENGTH.ipv4 | |
| : MAX_PREFIX_LENGTH.ipv6; | |
| if (num > max) { | |
| return `Prefix length must be at most ${max}`; | |
| } | |
| return undefined; | |
| }, [protocolVersion]); |
🤖 Prompt for AI Agents
In
libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/FormViewNetworkWide/FormViewNetworkWideFields.tsx
around lines 80–91, the prefix-length validator currently allows NaN and decimal
inputs and unsafely indexes MAX_PREFIX_LENGTH by protocolVersion; trim the
input, parse it as an integer (reject if parsing yields NaN or the original
trimmed string !== parsed integer string to catch decimals), then compare the
integer against MIN_PREFIX_LENGTH and a safe max resolved with the same pattern
used elsewhere (e.g., protocolVersion === 'v6' ? MAX_PREFIX_LENGTH_V6 :
MAX_PREFIX_LENGTH_V4) instead of direct enum indexing, return the appropriate
messages for empty/NaN/decimal/out-of-range, and memoize the validator with
useCallback/useMemo keyed by protocolVersion so it isn’t recreated
unnecessarily.
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
jgyselov
left a comment
jgyselov
left a comment
There was a problem hiding this comment.
Please, also rename the PR to follow the MGMT-xxxx: [jira title] format. It makes navigation between PRs and Jira tickets easier and allows the @openshift-ci-robot to trigger a procedure that links this PR in the corresponding Jira ticket (so you don't have to do that manually -- it's nice!).
| return getHumanizedSubnetRange(getAddressObject(cidr, protocolVersion)); | ||
| }, [value, protocolVersion, errorMessage]); | ||
|
|
||
| const handleValidatePrefixLength = (value: string) => { |
There was a problem hiding this comment.
While this technically does the job, I don't think it's the best solution here.
In the Assisted Installer UI project, we use Yup and Formik for form handling and validation. And you can generally assume that if there's a form, there is a Yup validation schema for it somewhere.
In the case of the static IP form (more specifically, the "form view", "network wide" static IP form), the Yup schema is defined in here and attached to the form via here.
From the networkWideValidationSchema object, you can navigate to getMachineNetworkValidationSchema where we already have a validation schema set up for the subnet prefix length.
If you play around with the prefix length input field on the master branch, you'll see that the validation for the minimum and maximum is working. So the question is, why does it not work for the required test?
My immediate theory is that required doesn't handle the prefixLength: number | '' type (defined in here) the way we wanted it to, so required might not be the best way to go here.
You will need to tweak the validation schema so that it handles the '' value the way we want it to. You should be able to do it all with Yup functions. Look at .when() and .test() and see if they are helpful.
|
@asmasarw: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
asmasarw
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asmasarw The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
bot
added
size/XS
asmasarw
force-pushed
the
feature/MGMT-20743
branch
from
99ef8cf to
7709896
Compare
openshift-ci
bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersion.tsx (2)
11-13: Type-safety: use ClusterCpuArchitecture instead of string and avoid unsafe cast later
clusterCpuArchitectureis typed asstringand later cast toClusterCpuArchitecture. Tighten the prop type to reflect the actual domain and drop the cast downstream.Apply this diff to the prop type:
- clusterCpuArchitecture?: string; + clusterCpuArchitecture?: ClusterCpuArchitecture;
31-37: Guard multi-arch text on presence of cpuArchitecture; drop the castIf
withMultiTextis true butclusterCpuArchitectureis undefined, we pass an undefined (casted) architecture into the helper. Make it explicit: only add multi-arch text whenclusterCpuArchitectureis available, and avoid the cast by leveraging the stricter prop typing.Apply this diff:
- {getOpenshiftVersionText({ + {getOpenshiftVersionText({ openshiftVersion, - cpuArchitecture: clusterCpuArchitecture as ClusterCpuArchitecture, + cpuArchitecture: clusterCpuArchitecture, versions, withPreviewText, - withMultiText, + withMultiText: Boolean(withMultiText && clusterCpuArchitecture), })}libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx (1)
167-177: Use i18n and disable switch in viewer mode
- The switch label should be translated (this file already uses
useTranslation()).- In viewer mode, prevent toggling by disabling the switch.
Apply this diff:
- {!isSingleClusterFeatureEnabled && ( + {!isSingleClusterFeatureEnabled && ( <GridItem> <Switch id="disconnected-install-switch" - label="I'm installing on a disconnected/air-gapped/secured environment" + label={t('ai:disconnectedInstallSwitchLabel')} isChecked={installDisconnected} - onChange={(_, checked) => setInstallDisconnected(checked)} + onChange={(_, checked) => setInstallDisconnected(checked)} + isDisabled={isViewerMode} ouiaId="DisconnectedInstall" /> </GridItem> )}Follow-up: add the
ai:disconnectedInstallSwitchLabelkey to the locale files.libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts (1)
32-34: New disconnected step IDs added — confirm interplay with wizardStepsOrder/isStepAfterThe new steps are not part of wizardStepsOrder. isStepAfter returns false for unknown steps, which is fine today (provider logic treats it as “not after”). Still, to avoid future surprises when comparing steps across flows, consider either:
- including disconnected steps in a separate order array and guarding isStepAfter usage by flow, or
- explicitly gating isStepAfter calls to only run for the connected flow.
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx (5)
35-37: Avoid hard-coded download URL; parameterize and verify CORS/availabilityHard-coding the ISO URL will age quickly and makes staging vs. prod switching harder. Also, cross-origin downloads may be blocked if the server is not configured correctly.
Consider making this configurable via an env/config constant and verify CORS and filename handling.
Apply this diff to parameterize the URL:
-const downloadUrl = - 'https://mirror.openshift.com/pub/cgw/assisted-installer-disconnected/latest/agent-ove.x86_64.iso'; +const downloadUrl = + process.env.REACT_APP_AA_DISCONNECTED_ISO_URL ?? + 'https://mirror.openshift.com/pub/cgw/assisted-installer-disconnected/latest/agent-ove.x86_64.iso';Verification steps:
- Confirm the final URL value resolves in your deployment (staging/prod).
- Manually test that clicking “Download ISO” initiates a download and is not blocked by CORS. If the blob filename is important, consider fetching as a Blob and passing it to saveAs with an explicit filename.
54-61: Navigation immediately after saveAs may race with downloadNavigating right after triggering the download can occasionally interfere in some browsers. If you see flakes, add a short defer or trigger navigation after onClick via setTimeout.
- onNext={() => { - downloadUrl && saveAs(downloadUrl); - navigate('/cluster-list'); - }} + onNext={() => { + if (downloadUrl) { + saveAs(downloadUrl); + } + // Defer navigation to avoid racing with the download + setTimeout(() => navigate('/cluster-list'), 100); + }}
63-75: Localize user-facing strings“Failed to load Review step” and “Review and download ISO” should use i18n like the rest of the UI.
- <WithErrorBoundary title="Failed to load Review step"> + <WithErrorBoundary title={t('ai:Failed to load Review step')}> ... - <Text component={TextVariants.h2}>Review and download ISO</Text> + <Text component={TextVariants.h2}>{t('ai:Review and download ISO')}</Text>You’ll need to import/use useTranslation in this file:
-} from '../../../../common'; +} from '../../../../common'; +import { useTranslation } from '../../../../common'; ... -const ReviewStep = () => { - const { moveBack } = useClusterWizardContext(); +const ReviewStep = () => { + const { t } = useTranslation(); + const { moveBack } = useClusterWizardContext();
75-91: Localize boot instructionsThese user-facing strings should go through i18n for consistency.
- <Alert isInline variant="info" title="Discovery ISO boot instructions"> + <Alert isInline variant="info" title={t('ai:Discovery ISO boot instructions')}> ... - <ListItem>Download ISO</ListItem> + <ListItem>{t('ai:Download ISO')}</ListItem> ... - Download or copy your pull secret from{' '} + {t('ai:Download or copy your pull secret from')}{' '} ... - Boot your cluster's machines from this ISO and follow instructions + {t("ai:Boot your cluster's machines from this ISO and follow instructions")} ... - Provide your pull secret inside the installation wizard when requested + {t('ai:Provide your pull secret inside the installation wizard when requested')}
92-101: Localize “List of available operators” and consider precomputing the operator map
- i18n for the alert title.
- Minor: precompute a Map by operatorKey to avoid linear search per item (low impact given the small list).
- <Alert isInline isExpandable variant="info" title="List of available operators"> + <Alert isInline isExpandable variant="info" title={t('ai:List of available operators')}>Optional precompute (outside render):
const opsByKey = React.useMemo(() => { const all = Object.values(opSpecs).flatMap((op) => op); return new Map(all.map((op) => [op.operatorKey, op])); }, [opSpecs]);Then:
- const operator = Object.values(opSpecs) - .flatMap((op) => op) - .find((op) => op.operatorKey === o); + const operator = opsByKey.get(o);libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (5)
43-44: Localize error boundary titleWrap the title in t(...) for consistency.
- <WithErrorBoundary title="Failed to load Basic step"> + <WithErrorBoundary title={t('ai:Failed to load Basic step')}>
49-55: Localize headingUse i18n for “Basic information”.
- <Text component={TextVariants.h2}>Basic information</Text> + <Text component={TextVariants.h2}>{t('ai:Basic information')}</Text>
58-64: Switch label should be localized; double-check onChange signature
- Localize the label.
- Verify the PatternFly Switch onChange signature in your version. If it’s (event, checked), your handler is correct. If it’s (checked, event), swap the args.
<Switch id="disconnected-install-switch" - label="I'm installing on a disconnected/air-gapped/secured environment" + label={t("ai:I'm installing on a disconnected/air-gapped/secured environment")} isChecked={installDisconnected} - onChange={(_, checked) => setInstallDisconnected(checked)} + onChange={(_, checked) => setInstallDisconnected(checked)} ouiaId="DisconnectedInstall" />If your PF version expects (checked, event), use:
onChange={(checked) => setInstallDisconnected(!!checked)}
68-74: Avoid assuming window.location origin for external linkIf OCP_RELEASES_PAGE is a relative path intended for a specific site (e.g., console.redhat.com), constructing it from window.location.origin can break in other environments. Prefer a fully qualified URL from config, or use URL(OCP_RELEASES_PAGE, base) with an explicit base.
- <OcmOpenShiftVersion openshiftVersion="4.19" withPreviewText withMultiText> - <ExternalLink href={`${window.location.origin}/${OCP_RELEASES_PAGE}`}> + <OcmOpenShiftVersion openshiftVersion="4.19" withPreviewText withMultiText> + <ExternalLink href={OCP_RELEASES_PAGE}> <span data-ouia-id="openshift-releases-link"> {t('ai:Learn more about OpenShift releases')} </span> </ExternalLink> </OcmOpenShiftVersion>Please confirm OCP_RELEASES_PAGE is an absolute URL in your config for staging/prod.
75-77: Hard-coded CPU architecture — consider deriving from state or a single source of truthIf possible, source the architecture from cluster context or a central config to avoid drift between steps (Basic vs. Review).
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (2)
106-107: Active steps aliasing — consider clearer naming and flow-guarded logicwizardStepIds is now an alias that may point to disconnectedSteps. Later, getWizardStepIds() is called (in useEffect) using this alias, which can inadvertently derive “connected” step lists from the disconnected base. Prefer passing connectedWizardStepIds to getWizardStepIds and only using the alias for navigation.
Proposed change (outside this hunk; illustrative):
// inside useEffect: const firstStepIds = getWizardStepIds( connectedWizardStepIds, // base should be the connected set staticIpInfo?.view, customManifestsStepEnabled, isSingleClusterFeatureEnabled, );
273-276: useMemo dependency: setInstallDisconnected not neededReact’s state setter is stable; including it in deps is unnecessary noise. Safe to remove.
}, [ wizardStepIds, currentStepId, infraEnv, customManifestsStep, wizardPerPage, isSingleClusterFeatureEnabled, clearAlerts, uiSettings, updateUISettings, - installDisconnected, - setInstallDisconnected, + installDisconnected, connectedWizardStepIds, ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersion.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/FormViewNetworkWide/formViewNetworkWideValidationSchema.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx(6 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts(4 hunks)
💤 Files with no reviewable changes (2)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/FormViewNetworkWide/formViewNetworkWideValidationSchema.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetails.tsx
🧰 Additional context used
🧬 Code Graph Analysis (6)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/BasicStep.tsx (4)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)libs/ui-lib/lib/common/components/ui/DeveloperPreview.tsx (1)
DeveloperPreview(8-20)libs/ui-lib/lib/common/config/docs_links.ts (1)
OCP_RELEASES_PAGE(84-84)libs/ui-lib/lib/common/components/ui/StaticTextField.tsx (1)
StaticTextField(80-92)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (2)
libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts (1)
ClusterWizardStepsType(21-34)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx (5)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(137-526)libs/ui-lib/lib/common/components/ui/DeveloperPreview.tsx (1)
DeveloperPreview(8-20)libs/ui-lib/lib/common/config/docs_links.ts (1)
PULL_SECRET_INFO_LINK(41-41)libs/ui-lib/lib/common/config/constants.ts (1)
singleClusterOperators(346-358)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (1)
libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts (2)
ClusterWizardStepsType(21-34)disconnectedSteps(50-53)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx (1)
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(27-33)
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersion.tsx (3)
libs/ui-lib/lib/common/components/ui/StaticTextField.tsx (1)
StaticTextField(80-92)libs/ui-lib/lib/common/components/clusterConfiguration/utils.ts (1)
getOpenshiftVersionText(60-68)libs/ui-lib/lib/common/types/cpuArchitecture.ts (1)
ClusterCpuArchitecture(3-3)
🔇 Additional comments (10)
libs/ui-lib/lib/ocm/components/clusterWizard/NewClusterWizard.tsx (2)
9-18: LGTM: step mapping looks correct and minimalClean switch-based mapping to disconnected steps with a sensible default to ClusterDetails.
21-25: Verify: is the "Machine network CIDR mandatory" validation included in this PR?Short summary: I located where Machine Network CIDR is enforced (validation schemas and static-IP validators) but did not find corresponding edits to those validation files in this branch — my git diff against origin/master had a merge-base error so the changed-files check may be inconclusive. Please confirm whether the mandatory-CIDR change is part of this PR and, if so, where.
Files to check / where validation lives:
- libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts — machineNetworksValidationSchema (enforces machineNetworks shape)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts — references machineNetworksValidationSchema for the Networking form
- libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/FormViewNetworkWide/formViewNetworkWideValidationSchema.tsx — static-IP network-wide validation (machineNetwork rules)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/FormViewHosts/formViewHostsValidationSchema.tsx — host-specific static-IP validation (references machine network CIDR)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/staticIp/components/YamlView/yamlViewValidationSchema.tsx — YAML static-IP validation parsing machineNetworks
Requested action: if the PR intends to make Machine network CIDR mandatory, include the corresponding changes to the schemas above (or point me to the exact file(s) in this PR); otherwise please update the PR title/description to reflect that it only changes wizard rendering/navigation.
libs/ui-lib/lib/ocm/components/clusterWizard/constants.ts (1)
15-16: LGTM: added disconnected step labelsThe new labels align with the added step IDs and the disconnected flow.
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (2)
10-12: LGTM: navigation callbacks as function-valued propertiesSwitching
moveBack/moveNextto() => voidproperties is clear and consistent with typical context usage patterns.
21-23: Context provider wiring verified
All checks confirminstallDisconnectedand its setter are correctly initialized, exposed, and used to switch between connected and disconnected step sequences.• In ClusterWizardContextProvider:
–const [installDisconnected, setInstallDisconnected] = React.useState(false);
– Context value includes bothinstallDisconnectedandsetInstallDisconnected
–wizardStepIdsswitches betweenconnectedWizardStepIdsanddisconnectedStepsbased oninstallDisconnected
– Setter resets the wizard page when toggled• In wizardTransition.ts:
–export const disconnectedSteps = ['disconnected-basic','disconnected-review'];• In consumer components (BasicStep, ClusterDetailsForm, NewClusterWizard):
–installDisconnectedandsetInstallDisconnectedare pulled from context and wired to the UI switchNo further changes required.
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterDetailsForm.tsx (1)
65-66: LGTM: sourcing navigation and flags from contextMoving
moveNextand the newinstallDisconnectedstate into context reduces prop drilling and aligns with the wizard’s global flow management.libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts (3)
50-53: disconnectedSteps export — LGTMThe explicit sequence for the disconnected flow is clear and minimal. No concerns.
261-263: Empty validations for disconnected steps — OK for Developer PreviewUsing buildEmptyValidationsMap keeps navigation unblocked for the new steps. If you plan to gate progression later (e.g., require inputs), wire appropriate validations here.
276-278: Wiring validations into wizardStepsValidationsMap — LGTMThe disconnected steps are properly represented in the global validations map.
libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx (1)
91-95: Separate connected step IDs vs. disconnected flow state — naming LGTMStoring connectedWizardStepIds separately is a good call. This avoids mutating the disconnectedSteps array. No issues here.
| setInstallDisconnected: (enabled: boolean) => { | ||
| setInstallDisconnected(enabled); | ||
| if (enabled) { | ||
| onSetCurrentStepId(disconnectedSteps[0]); | ||
| } else { | ||
| connectedWizardStepIds?.length && onSetCurrentStepId(connectedWizardStepIds[0]); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Toggling to connected flow when IDs are not ready can leave currentStepId invalid
When disabling disconnected mode, connectedWizardStepIds may still be undefined. In that case, currentStepId remains a disconnected step and navigation functions will fail (indexOf returns -1).
Compute the connected steps (and set them) before switching, and always set a valid current step.
Apply this diff:
setInstallDisconnected: (enabled: boolean) => {
setInstallDisconnected(enabled);
if (enabled) {
onSetCurrentStepId(disconnectedSteps[0]);
} else {
- connectedWizardStepIds?.length && onSetCurrentStepId(connectedWizardStepIds[0]);
+ // Build the connected step list deterministically and move to its first step
+ const staticIpInfo = infraEnv ? getStaticIpInfo(infraEnv) : undefined;
+ const nextSteps = getWizardStepIds(
+ connectedWizardStepIds,
+ staticIpInfo?.view,
+ customManifestsStep,
+ isSingleClusterFeatureEnabled,
+ );
+ setWizardStepIds(nextSteps);
+ onSetCurrentStepId(nextSteps[0]);
}
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setInstallDisconnected: (enabled: boolean) => { | |
| setInstallDisconnected(enabled); | |
| if (enabled) { | |
| onSetCurrentStepId(disconnectedSteps[0]); | |
| } else { | |
| connectedWizardStepIds?.length && onSetCurrentStepId(connectedWizardStepIds[0]); | |
| } | |
| }, | |
| setInstallDisconnected: (enabled: boolean) => { | |
| setInstallDisconnected(enabled); | |
| if (enabled) { | |
| onSetCurrentStepId(disconnectedSteps[0]); | |
| } else { | |
| // Build the connected step list deterministically and move to its first step | |
| const staticIpInfo = infraEnv ? getStaticIpInfo(infraEnv) : undefined; | |
| const nextSteps = getWizardStepIds( | |
| connectedWizardStepIds, | |
| staticIpInfo?.view, | |
| customManifestsStep, | |
| isSingleClusterFeatureEnabled, | |
| ); | |
| setWizardStepIds(nextSteps); | |
| onSetCurrentStepId(nextSteps[0]); | |
| } | |
| }, |
| <DescriptionList isHorizontal> | ||
| <DescriptionListGroup> | ||
| <DescriptionListTerm>OpenShift version</DescriptionListTerm> | ||
| <DescriptionListDescription>4.19</DescriptionListDescription> | ||
| </DescriptionListGroup> | ||
| <DescriptionListGroup> | ||
| <DescriptionListTerm>CPU architecture</DescriptionListTerm> | ||
| <DescriptionListDescription>x86_64</DescriptionListDescription> | ||
| </DescriptionListGroup> | ||
| <DescriptionListGroup> | ||
| <DescriptionListTerm>ISO size</DescriptionListTerm> | ||
| <DescriptionListDescription>approx. 35GB</DescriptionListDescription> | ||
| </DescriptionListGroup> | ||
| </DescriptionList> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incorrect ISO size; avoid hard-coded, potentially misleading values
“approx. 35GB” is very likely incorrect for a discovery ISO and may confuse users. Either remove the size or derive it dynamically when known.
<DescriptionList isHorizontal>
<DescriptionListGroup>
- <DescriptionListTerm>OpenShift version</DescriptionListTerm>
- <DescriptionListDescription>4.19</DescriptionListDescription>
+ <DescriptionListTerm>{t('ai:OpenShift version')}</DescriptionListTerm>
+ <DescriptionListDescription>4.19</DescriptionListDescription>
</DescriptionListGroup>
<DescriptionListGroup>
- <DescriptionListTerm>CPU architecture</DescriptionListTerm>
- <DescriptionListDescription>x86_64</DescriptionListDescription>
+ <DescriptionListTerm>{t('ai:CPU architecture')}</DescriptionListTerm>
+ <DescriptionListDescription>x86_64</DescriptionListDescription>
</DescriptionListGroup>
- <DescriptionListGroup>
- <DescriptionListTerm>ISO size</DescriptionListTerm>
- <DescriptionListDescription>approx. 35GB</DescriptionListDescription>
- </DescriptionListGroup>Optionally, if you can resolve the actual size, display it with a proper unit formatter.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <DescriptionList isHorizontal> | |
| <DescriptionListGroup> | |
| <DescriptionListTerm>OpenShift version</DescriptionListTerm> | |
| <DescriptionListDescription>4.19</DescriptionListDescription> | |
| </DescriptionListGroup> | |
| <DescriptionListGroup> | |
| <DescriptionListTerm>CPU architecture</DescriptionListTerm> | |
| <DescriptionListDescription>x86_64</DescriptionListDescription> | |
| </DescriptionListGroup> | |
| <DescriptionListGroup> | |
| <DescriptionListTerm>ISO size</DescriptionListTerm> | |
| <DescriptionListDescription>approx. 35GB</DescriptionListDescription> | |
| </DescriptionListGroup> | |
| </DescriptionList> | |
| <DescriptionList isHorizontal> | |
| <DescriptionListGroup> | |
| <DescriptionListTerm>{t('ai:OpenShift version')}</DescriptionListTerm> | |
| <DescriptionListDescription>4.19</DescriptionListDescription> | |
| </DescriptionListGroup> | |
| <DescriptionListGroup> | |
| <DescriptionListTerm>{t('ai:CPU architecture')}</DescriptionListTerm> | |
| <DescriptionListDescription>x86_64</DescriptionListDescription> | |
| </DescriptionListGroup> | |
| </DescriptionList> |
🤖 Prompt for AI Agents
In libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/ReviewStep.tsx
around lines 102 to 115, the ISO size is hard-coded as "approx. 35GB" which is
incorrect; remove the hard-coded DescriptionListGroup or replace it with a
conditional render that uses a passed-in isoSize value (or a lookup) and a unit
formatter. Specifically, stop rendering the ISO size block when isoSize is
unknown, accept/derive the real size via props or async metadata, format it with
a units helper (e.g., bytes -> GB) and only render the DescriptionListGroup if
isoSize is defined and properly formatted.
4 participants
Summary by CodeRabbit
Bug Fixes
New Features