MGMT-20076: Support external platform in Assisted-installer Kube API#3151
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jgyselov: This pull request references MGMT-20076 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 epic to target the "4.20.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. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds a "Custom manifests" wizard step with ConfigMap selection, validation, watch hooks, patch submission and review; introduces hooks/types/models for ConfigMaps and AgentClusterInstall manifests refs; refactors many ClusterDeployment step modules to named exports/index barrels; refactors OpenShift version selection to a Formik-driven searchable select; adds External Platforms dropdown and many i18n keys plus typo fixes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Wizard as ClusterDeployment Wizard
participant CustomStep as Custom Manifests Step
participant Hook as useConfigMaps
participant K8s as Kubernetes API
participant ACI as AgentClusterInstall (API)
participant Review as Review Step
User->>Wizard: navigate to "Custom manifests"
Wizard->>CustomStep: render (pass agentClusterInstall)
CustomStep->>Hook: watch ConfigMaps(namespace)
Hook->>K8s: subscribe/list ConfigMaps
K8s-->>Hook: return data/updates
Hook-->>CustomStep: data or error
User->>CustomStep: select ConfigMaps & edit
CustomStep->>CustomStep: validate names & uniqueness
User->>CustomStep: submit
CustomStep->>ACI: k8sPatch /spec/manifestsConfigMapRefs (via appendPatch)
ACI-->>Wizard: patch result
Wizard->>Review: proceed to Review step
Review->>Hook: watch ConfigMaps(namespace)
Hook->>K8s: get data
Review-->>User: render ReviewConfigMapsTable
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ 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
approved
|
@jgyselov: This pull request references MGMT-20076 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 epic to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@jgyselov: This pull request references MGMT-20076 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 epic to target the "4.20.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: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
libs/ui-lib/lib/cim/components/Agent/AgentStatus.tsx (1)
44-48: Guard against undefined validationsInfo to avoid runtime error.Object.values(status.validationsInfo) will throw when validationsInfo is undefined.
Apply:
- const showValidationsRunning = - isDay2 && - Object.values(status.validationsInfo).some((valInfo) => - valInfo.some((v) => ['pending', 'error', 'failure'].includes(v.status)), - ); + const showValidationsRunning = Boolean( + isDay2 && + Object.values(status.validationsInfo ?? {}).some((valInfo) => + (valInfo ?? []).some((v) => ['pending', 'error', 'failure'].includes(v.status)), + ), + );libs/ui-lib/lib/cim/components/ClusterDeployment/components/AdditionalNTPSourcesDialogToggle.tsx (1)
9-12: Fix NTP sources copy and update translation keysReplace the i18n key to remove the extra space and improve phrasing:
- {t( - "ai:NTP sources can be edited in Host's Infrastructure environment. After updating the NTP sources, make sure to restart the host.", - )} + {t( + "ai:NTP sources can be edited in the host's Infrastructure environment. After updating the NTP sources, restart the host.", + )}Then update the key in all locale files (libs/locales/lib/{en,zh,ko,ja,es,fr}/translation.json) to match the new string, preserving each translation value.
libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx (1)
5-5: Cancel currently wipes user selection; restore previous value insteadCancel should revert to the value at modal open, not clear it. Otherwise users can lose an existing selection unintentionally.
Apply:
-import { useFormikContext } from 'formik'; +import { useField, useFormikContext } from 'formik'; @@ - const { setFieldValue } = useFormikContext<ClusterDetailsValues>(); + const { setFieldValue } = useFormikContext<ClusterDetailsValues>(); + // Snapshot value at mount/open to restore on cancel + const [{ value: initialValue }] = useField<string | null>('customOpenshiftSelect'); + const initialValueRef = React.useRef(initialValue); @@ - const onCancel = () => { - setFieldValue('customOpenshiftSelect', null); - setOpenshiftVersionModalOpen(false); - }; + const onCancel = () => { + setFieldValue('customOpenshiftSelect', initialValueRef.current); + setOpenshiftVersionModalOpen(false); + };Also consider local (modal) state for the selection and committing only on “Select” for clearer UX, but the above keeps current Formik-driven approach.
Also applies to: 22-25, 33-45
libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ACMClusterDeploymentDetailsStep.tsx (1)
68-73: Formik should enableReinitialize to reflect async prop changesinitialValues/validationSchema depend on ocpVersions and usedClusterNames (derived from props). Without enableReinitialize, Formik won’t update after those props load/change.
Apply:
return ( <Formik initialValues={initialValues} + enableReinitialize validate={getRichTextValidation(validationSchema)} innerRef={formRef} onSubmit={noop} >libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsStep.tsx (1)
131-136: Make error messaging robust.Casting error to string often yields “[object Object]”. Prefer extracting message safely.
- addAlert({ - title: t('ai:Failed to save ClusterDeployment'), - message: error as string, - }); + addAlert({ + title: t('ai:Failed to save ClusterDeployment'), + message: error instanceof Error ? error.message : String(error), + });libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscoveryStep.tsx (1)
176-181: Fix step id and typo in user-facing text.Wrong step id breaks ValidationSection context; also “occured” -> “occurred”.
Apply:
- <ValidationSection currentStepId={'hosts-selection'} hosts={[]}> - <Alert variant={AlertVariant.danger} title={t('ai:An error occured')} isInline> + <ValidationSection currentStepId={'hosts-discovery'} hosts={[]}> + <Alert variant={AlertVariant.danger} title={t('ai:An error occurred')} isInline>libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (1)
63-75: Fix stale default selection: missing deps in useEffect.The default version won’t update when versions/customItem change. Include both in deps and drop the eslint-disable.
- React.useEffect(() => { + React.useEffect(() => { let defaultVersion = versions.find((item) => item.default); if (customOpenshiftSelect && customItem) { defaultVersion = customItem; } else if (customOpenshiftSelect) { defaultVersion = versions.find((item) => item.value === customOpenshiftSelect); } setCurrent(defaultVersion?.label || ''); setValue(defaultVersion?.value || ''); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [customOpenshiftSelect]); + }, [customOpenshiftSelect, customItem, versions, setValue]);libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx (3)
174-184: Always clear Formik submitting state on errors in onEditRoleIf onEditRoleInit throws, isSubmitting stays true and the footer remains disabled.
Apply:
const onEditRole = React.useCallback( async (agent: AgentK8sResource, role: string | undefined) => { setNextRequested(false); setShowClusterErrors(false); - setSubmitting(true); - const response = await onEditRoleInit?.(agent, role); - setSubmitting(false); - return response; + setSubmitting(true); + try { + return await onEditRoleInit?.(agent, role); + } finally { + setSubmitting(false); + } }, [onEditRoleInit, setSubmitting], );
255-263: ValidationSection uses the wrong step idcurrentStepId should match this step; using cluster-details may hide relevant validations.
- <ValidationSection currentStepId={'cluster-details'} hosts={[]}> + <ValidationSection currentStepId={'hosts-selection'} hosts={[]}>
321-332: Show selection validation for both manual and auto-select modesOnly selectedHostIds errors are shown; autoSelectedHostIds errors are invisible to the user.
- {showFormErrors && errors.selectedHostIds && touched.selectedHostIds && ( + {showFormErrors && + (errors.selectedHostIds || errors.autoSelectedHostIds) && + (touched.selectedHostIds || touched.autoSelectedHostIds) && ( <GridItem> <Alert variant={AlertVariant.danger} title={t('ai:Provided cluster configuration is not valid')} isInline > - {errors.selectedHostIds} + {errors.selectedHostIds || errors.autoSelectedHostIds} </Alert> </GridItem> )}
🧹 Nitpick comments (45)
libs/ui-lib/package.json (1)
5-5: SDK 1.0.0 bump: verify Console compatibility and consider peering the SDK.
- Confirm that all imports (e.g., K8sResourceCommon) still resolve under @openshift-console/dynamic-plugin-sdk@1.0.0 and that the OpenShift Console version you target provides this API at runtime.
- Consider making the SDK a peerDependency (and devDependency for build) to avoid bundling multiple copies in consuming plugins.
Optional diff to peer the SDK:
@@ "dependencies": { "@openshift-assisted/locales": "workspace:*", "@openshift-assisted/types": "workspace:*", - "@openshift-console/dynamic-plugin-sdk": "1.0.0", "@patternfly/patternfly": "5.2.0", @@ "devDependencies": { + "@openshift-console/dynamic-plugin-sdk": "1.0.0", @@ "peerDependencies": { + "@openshift-console/dynamic-plugin-sdk": "^1.0.0", "@reduxjs/toolkit": "^1.9.1",libs/ui-lib/lib/cim/components/ClusterDeployment/use-networking-formik.ts (1)
24-25: Switch to shared IPv4 defaults looks fine; ensure IPv6/dual-stack still initialize correctly.If clusters can be IPv6-only or dual-stack, verify getNetworkInitialValues with CLUSTER_DEFAULT_NETWORK_SETTINGS_IPV4 doesn’t regress those flows (e.g., wrong defaults when cluster has no CIDRs yet).
Also applies to: 82-83
libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts (1)
60-61: manifestsConfigMapRefs added — validate CRD field/version; consider a named type.
- Confirm the ACAI CRD version you target includes spec.manifestsConfigMapRefs and that UI gracefully handles clusters without it.
- Minor: introduce a ConfigMapRef type for reuse/readability.
Optional diff:
@@ -export type AgentClusterInstallK8sResource = K8sResourceCommon & { +type ConfigMapRef = { name: string }; +export type AgentClusterInstallK8sResource = K8sResourceCommon & { spec?: { @@ - manifestsConfigMapRefs?: { name: string }[]; + manifestsConfigMapRefs?: ConfigMapRef[]; };libs/ui-lib/lib/cim/hooks/types.tsx (1)
3-9: Constrain list vs single-watch props at the type level.Prevent accidental misuse by encoding isList in the types.
-export type K8sWatchHookListProps = Omit< - WatchK8sResource, - 'groupVersionKind' | 'name' | 'isList' -> | null; +export type K8sWatchHookListProps = + | (Omit<WatchK8sResource, 'groupVersionKind' | 'name' | 'isList'> & { isList?: true }) + | null; -export type K8sWatchHookProps = Pick<WatchK8sResource, 'name' | 'namespace' | 'isList'> | null; +export type K8sWatchHookProps = + | (Pick<WatchK8sResource, 'name' | 'namespace'> & { isList?: false }) + | null;libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx (1)
13-27: Avoid broad eslint-disable; keep hooks unconditional without disables.Refactor to compute the watched resource first and call consoleWatch once; no need to disable any rule.
-export const useK8sWatchResource = <R extends K8sResourceCommon | K8sResourceCommon[]>( +export const useK8sWatchResource = <R extends K8sResourceCommon | K8sResourceCommon[]>( resource: Pick<WatchK8sResource, 'groupVersionKind'>, props?: Omit<WatchK8sResource, 'groupVersionKind'> | null, ): WatchK8sResult<R> => { - // eslint-disable-next-line - const [data, loaded, error] = consoleWatch<R>( - props !== null - ? { - ...resource, - ...(props || {}), - } - : null, - ); + const watchArg = + props !== null + ? { + ...resource, + ...(props || {}), + } + : null; + const [data, loaded, error] = consoleWatch<R>(watchArg); return [data, error ? true : loaded, error as unknown]; };libs/ui-lib/lib/cim/components/ClusterDeployment/components/AdditionalNTPSourcesDialogToggle.tsx (1)
4-4: Track the TODO or implement via hostValidationFailureHints.If desired, I can draft a small helper that maps failure IDs to actionable hints and wire it here.
libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx (1)
29-42: Optional: internationalize visible stringsTitle, field label, and button texts are hardcoded. Wrap in t(...) for consistency with the rest of the UI.
libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx (1)
5-14: Consider widening props typing to future‑proof filteringIf you expect label/field selectors or other watch options later, define K8sWatchHookProps as Omit<WatchK8sResource, 'groupVersionKind'> | null to pass through any supported console watch fields.
Example (in libs/ui-lib/lib/cim/hooks/types.tsx):
export type K8sWatchHookProps = Omit<WatchK8sResource, 'groupVersionKind'> | null;libs/ui-lib/lib/cim/types/models.tsx (1)
1-1: Prefer stable package-root importAvoid deep path to reduce breakage risk on SDK internals.
Apply:
-import { K8sModel } from '@openshift-console/dynamic-plugin-sdk/lib/api/common-types'; +import { K8sModel } from '@openshift-console/dynamic-plugin-sdk';libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionAdvanced.tsx (1)
28-35: Simplify component genericsAll imports already use named exports. Remove the generic
<T extends FormValues>from the component and switch to a fixedFormValuesin the Formik context:-export const ClusterDeploymentHostsSelectionAdvanced = <T extends FormValues>({ +export const ClusterDeploymentHostsSelectionAdvanced = ({ … - const { values } = useFormikContext<T>(); + const { values } = useFormikContext<FormValues>();Then drop the explicit generic arguments at the two usage sites:
- libs/ui-lib/lib/cim/components/ClusterDeployment/ScaleUpForm.tsx line 75
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelection.tsx line 87
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (2)
6-6: PatternFly v5: include DropdownList for consistencyWrap items with DropdownList to match PF v5 composables used elsewhere.
Apply:
-import { Dropdown, DropdownItem, FormGroup, MenuToggle } from '@patternfly/react-core'; +import { Dropdown, DropdownItem, DropdownList, FormGroup, MenuToggle } from '@patternfly/react-core';And:
- > - {options} - </Dropdown> + > + <DropdownList>{options}</DropdownList> + </Dropdown>
27-34: Optional: remove preventDefault on itemsDropdownItem defaults to a button; preventDefault isn’t needed and can interfere with event handling in some cases.
Apply:
- selected={platform === value} - onClick={(e: React.MouseEvent) => e.preventDefault()} + selected={platform === value}libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx (3)
39-47: Name reflects plurality but value is singular.
The variable holds a single item (or undefined). Rename for clarity.Apply:
- const updatedSelectOptions = React.useMemo(() => { + const customItem = React.useMemo(() => { @@ - return undefined; - }, [allVersions, customOpenshiftSelect, selectOptions]); + return undefined; + }, [allVersions, customOpenshiftSelect, selectOptions]);Follow-up change at Line 66 below.
53-55: Stabilize handler to reduce child re-renders.
Wrap in useCallback.- const showOpenshiftVersionModal = () => { - setIsOpenshiftVersionModalOpen(true); - }; + const showOpenshiftVersionModal = React.useCallback(() => { + setIsOpenshiftVersionModalOpen(true); + }, []);
64-67: Confirm isInOcm is a boolean, not a function.
If it’s a function, call it; otherwise leave as-is. Also update the prop usage to the renamed customItem.showReleasesLink={isInOcm} showOpenshiftVersionModal={showOpenshiftVersionModal} - customItem={updatedSelectOptions} + customItem={customItem}If needed:
- showReleasesLink={isInOcm} + showReleasesLink={isInOcm()}libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (3)
23-29: Localize the “Developer preview release” suffix.
Use i18n and include t in deps.- children: - version.supportLevel === 'beta' - ? version.label + ' - ' + 'Developer preview release' - : version.label, + children: + version.supportLevel === 'beta' + ? `${version.label} - ${t('ai:Developer preview release')}` + : version.label, @@ - [versions], + [versions, t],
48-55: Localize the “No results found” empty state.
Also avoid hardcoding quotes around the query.- newSelectOptions = [ - { - isDisabled: true, - children: `No results found for "${filterValue}"`, - value: 'no results', - }, - ]; + newSelectOptions = [ + { + isDisabled: true, + children: t('ai:No results found for "{{filterValue}}"', { filterValue }), + value: 'no results', + }, + ];Remember to include t in the effect deps:
- }, [filterValue, initialSelectOptions]); + }, [filterValue, initialSelectOptions, t]);
59-75: Guard divider logic for non-version values.
Prevent false dividers when options don’t match x.y.z.- const selectOptionsWithDividers = newSelectOptions.map((option, index) => { - const match = (option.value as string).match(/\d+\.(\d+)\.\d+/); - const y = match ? match[1] : null; - - const previousY = - index > 0 - ? ((newSelectOptions[index - 1].value as string).match(/\d+\.(\d+)\.\d+/) || [])[1] - : null; - - return { - ...option, - showDivider: previousY !== null && y !== previousY, - }; - }); + const selectOptionsWithDividers = newSelectOptions.map((option, index) => { + const y = + typeof option.value === 'string' + ? ((option.value as string).match(/^\d+\.(\d+)\.\d+/) || [])[1] ?? null + : null; + const previousY = + index > 0 && typeof newSelectOptions[index - 1].value === 'string' + ? ((newSelectOptions[index - 1].value as string).match(/^\d+\.(\d+)\.\d+/) || [])[1] ?? + null + : null; + return { + ...option, + showDivider: y !== null && previousY !== null && y !== previousY, + }; + });libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsStep.tsx (2)
67-84: Reinitialize form when inputs change (versions, cluster).initialValues is memoized with an empty dependency array, so changes to ocpVersions/cluster won’t update the form. Either depend on those values or enable reinitialize on Formik.
- const initialValues = React.useMemo( - () => { + const initialValues = React.useMemo(() => { const initValues = getClusterDetailsInitialValues({ managedDomains: [], // not supported cluster, ocpVersions, }); const ocpVersion = ocpVersions.find( (ocpVersion) => ocpVersion.value === initValues.openshiftVersion, ); return { ...initValues, networkType: getNetworkType(ocpVersion), }; - }, - [], // eslint-disable-line react-hooks/exhaustive-deps - ); + }, [cluster, ocpVersions]);And enable reinitialize:
- <Formik + <Formik initialValues={initialValues} + enableReinitialize validate={getRichTextValidation(validationSchema)}
147-148: Localize the step header.Use t(...) for consistency with the rest of the UI.
- <ClusterWizardStepHeader>Cluster Details</ClusterWizardStepHeader> + <ClusterWizardStepHeader>{t('ai:Cluster details')}</ClusterWizardStepHeader>libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx (4)
88-97: Minor: avoid Object.values on an array.configMaps is already an array; Object.values adds noise.
- const fuse = React.useMemo( - () => - new Fuse(Object.values(configMaps), { + const fuse = React.useMemo( + () => + new Fuse(configMaps, { includeScore: true, ignoreLocation: true, threshold: 0.3, keys: ['value.name'], }),
199-210: Avoid O(n·m) finds when rendering details.Precompute a name→ConfigMap map to prevent repeated .find per item.
- <FieldArray name="configMaps"> - {({ remove }) => - values.configMaps.map((configMap, index) => ( - <ConfigMapDetail - index={index} - key={configMap.name} - configMapName={configMap.name} - configMap={data.find((item) => item.metadata?.name === configMap.name)} - onRemove={() => remove(index)} - isLoading={!loaded} - /> - )) - } - </FieldArray> + <FieldArray name="configMaps"> + {({ remove }) => { + const byName = new Map( + (data || []).map((cm) => [cm.metadata?.name as string, cm]), + ); + return values.configMaps.map((configMap, index) => ( + <ConfigMapDetail + index={index} + key={configMap.name} + configMapName={configMap.name} + configMap={byName.get(configMap.name)} + onRemove={() => remove(index)} + isLoading={!loaded} + /> + )); + }} + </FieldArray>
78-86: Localize placeholder strings.These user-visible strings should go through t(...).
- return `${values.configMaps.length} config map${values.configMaps.length !== 1 ? 's' : ''} selected`; + return t('ai:{{count}} config map selected', { + count: values.configMaps.length, + }); ... - return 'No config map selected'; + return t('ai:No config map selected');
145-158: Name collision with project’s WizardFooter.You’re using PatternFly’s composable WizardFooter, but this repo also ships a WizardFooter component. Consider aliasing to avoid confusion.
- <WizardFooter + <WizardFooter activeStep={activeStep}Or import as:
-import { ..., WizardFooter, useWizardContext, useWizardFooter, ... } from '@patternfly/react-core'; +import { ..., WizardFooter as PFWizardFooter, useWizardContext, useWizardFooter, ... } from '@patternfly/react-core';libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx (1)
37-46: Strengthen cross-configMap uniqueness check.Current check passes if all current names exist but can misread counts. Use an exact count comparison.
- (value) => - // check if another config map has any manifests with the same names as this one - values.configMaps - .flatMap(({ manifestNames }) => manifestNames) - .filter((v) => value?.includes(v)).length === value?.length, + (value) => { + if (!value) return true; + const all = values.configMaps.flatMap(({ manifestNames }) => manifestNames || []); + return value.every((name) => all.filter((n) => n === name).length === 1); + },libs/ui-lib/lib/common/components/ui/formik/SelectFieldWithSearch.tsx (6)
192-201: Accurate toggle label.Label says “Multi …” even for single-select.
Apply:
- <MenuToggle - variant="typeahead" - aria-label="Multi typeahead checkbox menu toggle" + <MenuToggle + variant="typeahead" + aria-label={isMultiSelect ? 'Multi typeahead menu toggle' : 'Typeahead menu toggle'}
247-264: Add a key to the fragment and remove unnecessary ref.Missing key produces React warnings; ref={null} is unnecessary.
Apply:
- selectOptions.map((option, index) => ( - <> + selectOptions.map((option, index) => ( + <React.Fragment key={`row-${String(option.value ?? index)}`}> {option.showDivider && <Divider component="li" key={`divider-${index}`} />} <SelectOption hasCheckbox={isMultiSelect && !option.isAriaDisabled} isSelected={ isMultiSelect && Array.isArray(value) ? value.some((v) => isEqual(v, option.value)) : value === option.value } key={`option-${index}`} isFocused={focusedItemIndex === index} - ref={null} {...option} /> - </> + </React.Fragment> ))
239-239: Only mark listbox as multiselectable when applicable.Currently always true.
Apply:
- <SelectList isAriaMultiselectable id="select-typeahead-listbox"> + <SelectList isAriaMultiselectable={isMultiSelect} id={`${baseId}-listbox`}>
225-231: Remove explicit role override.Select sets appropriate roles; role="menu" conflicts with combobox/listbox semantics.
Apply:
- <Select - role="menu" + <Select
225-275: Expose validation state and disabled/readOnly props like other Formik fields.For parity with SelectField.tsx, consider showing Formik error (touched+error) and passing isDisabled/isReadOnly to controls. This improves UX consistency.
I can draft a minimal diff aligning FormGroup + FormHelperText behavior with SelectField.tsx if you want.
169-175: Multi-select robustness: ensure array shape.If initial Formik value isn’t an array, first multi-select pick will store a scalar. Either default initialValues[name] to [] for multi-select or coerce:
Apply:
- if (isMultiSelect && Array.isArray(value)) { - setValue( + if (isMultiSelect) { + const current = Array.isArray(value) ? value : []; + setValue( - value.some((v) => isEqual(v, val)) - ? value.filter((v) => !isEqual(v, val)) - : [...(value as unknown[]), val], + current.some((v) => isEqual(v, val)) + ? current.filter((v) => !isEqual(v, val)) + : [...current, val], );libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (2)
98-103: Avoid duplicate “Custom releases” entry.If customItem also exists in items, it will appear twice. Guard against duplicates.
- customItem && ( + customItem && !items.some(({ value }) => value === customItem.value) && ( <DropdownGroup label="Custom releases" key="custom-releases"> <DropdownItem key={customItem.value} id={customItem.value} value={customItem.value}> {customItem.label} </DropdownItem> </DropdownGroup> ),
154-162: Dropdown state toggling bug risk.onOpenChange provides the next open state; toggling with !isOpen can desync on outside clicks/esc.
- onOpenChange={() => setOpen(!isOpen)} + onOpenChange={(open) => setOpen(!!open)}libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx (2)
17-23: Faster filtering with Set and early bailouts.Replace includes over arrays with a Set for O(1) lookups; skip empty refs.
- const customManifests = React.useMemo(() => { - const maps = configMaps.filter((configMap) => - agentClusterInstall.spec?.manifestsConfigMapRefs - ?.flatMap((ref) => ref.name) - .includes(configMap.metadata?.name || ''), - ); + const customManifests = React.useMemo(() => { + const refNames = agentClusterInstall.spec?.manifestsConfigMapRefs?.map((r) => r.name) || []; + if (!refNames.length) return []; + const wanted = new Set(refNames); + const maps = configMaps.filter( + (configMap) => !!configMap.metadata?.name && wanted.has(configMap.metadata.name), + );
24-33: Stable row keys.Use ConfigMap name in rowId to avoid jitter when order changes.
- .map((map, index) => [ + .map((map, index) => [ { - rowId: `config-map-${index}`, + rowId: `config-map-${map.metadata?.name as string}`, cells: [{ title: map.metadata?.name as string, isExpanded: true, level: 1, posInset: 1 }], }, - ...Object.keys(map.data || {}).map((key) => ({ - rowId: `config-map-${index}-${key}`, + ...Object.keys(map.data || {}).map((key) => ({ + rowId: `config-map-${map.metadata?.name as string}-${key}`,libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx (3)
37-43: Incorrect Formik useField generics for.valid.The field value is boolean, not an object. Fix the type param.
- const [, { error: isValidError }] = useField<{ name: string; valid: boolean }>( - `configMaps.${index}.valid`, - ); + const [, { error: isValidError }] = useField<boolean>(`configMaps.${index}.valid`);
110-115: Incorrect Formik useField generics for.manifestNames.This field holds string[], not an object.
- const [, { error: manifestNamesError }] = useField<{ name: string; valid: boolean }>( - `configMaps.${index}.manifestNames`, - ); + const [, { error: manifestNamesError }] = useField<string[]>( + `configMaps.${index}.manifestNames`, + );
169-176: Improve removal button accessibility.Include the ConfigMap name in aria-label for screen readers.
- <Button - aria-label="remove config map" + <Button + aria-label={`remove config map ${configMapName}`} style={{ visibility: showRemoveButton ? 'visible' : 'hidden' }} variant="plain" onClick={onRemove} >libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx (1)
154-157: Show “Custom manifests” step only when relevant (External platform).Reduces noise for non-External flows.
- <WizardStep name={stepNames['custom-manifests']} id={'custom-manifests'}> - <ClusterDeploymentCustomManifestsStep agentClusterInstall={agentClusterInstall} /> - </WizardStep> + {agentClusterInstall.spec?.platformType === 'External' && ( + <WizardStep name={stepNames['custom-manifests']} id={'custom-manifests'}> + <ClusterDeploymentCustomManifestsStep + agentClusterInstall={agentClusterInstall} + /> + </WizardStep> + )}libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx (3)
86-100: Yup array schema stylePrefer array().of(Yup.string()) for clarity and future Yup compatibility.
- autoSelectedHostIds: values.autoSelectHosts - ? Yup.array(Yup.string()).min(values.hostCount).max(values.hostCount) - : Yup.array(Yup.string()), + autoSelectedHostIds: values.autoSelectHosts + ? Yup.array().of(Yup.string()).min(values.hostCount).max(values.hostCount) + : Yup.array().of(Yup.string()), - selectedHostIds: values.autoSelectHosts - ? Yup.array(Yup.string()) + selectedHostIds: values.autoSelectHosts + ? Yup.array().of(Yup.string()) : isSNOCluster - ? Yup.array(Yup.string()) + ? Yup.array().of(Yup.string()) .min(1, t('ai:Please select one host for the cluster.')) .max(1, t('ai:Please select one host for the cluster.')) // TODO(jtomasek): replace this with Yup.array().length() after updating Yup - : Yup.array(Yup.string()).min(3, t('ai:Please select at least 3 hosts for the cluster.')), + : Yup.array().of(Yup.string()).min(3, t('ai:Please select at least 3 hosts for the cluster.')),
215-231: Avoid recomputing agent statusesCompute once and reuse.
- const agentStatuses = selectedAgents.map( + const statuses = selectedAgents.map( (agent) => getWizardStepAgentStatus(agent, 'hosts-selection', t).status.key, ); - if ( - agentStatuses.some((status) => - ['disconnected', 'disabled', 'error', 'insufficient', 'cancelled'].includes(status), - ) - ) { + if (statuses.some((s) => + ['disconnected', 'disabled', 'error', 'insufficient', 'cancelled'].includes(s), + )) { setNextRequested(false); - } else if ( - !!selectedAgents.length && - selectedAgents.every( - (agent) => getWizardStepAgentStatus(agent, 'hosts-selection', t).status.key === 'known', - ) - ) { + } else if (!!selectedAgents.length && statuses.every((s) => s === 'known')) { setShowClusterErrors(true); if (canNextFromHostSelectionStep(agentClusterInstall, selectedAgents)) { void goToNextStep(); } }
370-376: Consider enableReinitialize for FormikInitial values derive from K8s resources; enabling reinit keeps the form in sync when props change.
return ( <Formik initialValues={initialValues} + enableReinitialize validationSchema={validationSchema} onSubmit={handleSubmit} >Would this cause unwanted resets in your flow? If yes, disregard.
libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostDiscoveryTable.tsx (1)
61-67: Use functional state updates for selectionPrevents stale closures when rapidly selecting/deselecting.
- const onSelect = (host: Host, isSelected: boolean) => { - if (isSelected) { - setSelectedHostIDs([...selectedHostIDs, host.id]); - } else { - setSelectedHostIDs(selectedHostIDs.filter((sa) => sa !== host.id)); - } - }; + const onSelect = (host: Host, isSelected: boolean) => { + setSelectedHostIDs((prev) => + isSelected ? [...prev, host.id] : prev.filter((sa) => sa !== host.id), + ); + };libs/locales/lib/en/translation.json (2)
200-203: Config map wording: align terminologyDecide on “ConfigMap” (Kubernetes resource) vs “config map” and apply consistently across these new strings.
Happy to batch-update once you confirm the preferred style.
Also applies to: 245-251, 776-776
476-479: Duplicate/overlapping keys for external platform phrasingThree similar strings will drift. Prefer one canonical phrase and reuse.
Proposed canonical: “Integrate with external platforms”.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (56)
libs/locales/lib/en/translation.json(15 hunks)libs/ui-lib/lib/cim/components/Agent/AgentStatus.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx(5 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/ScaleUpForm.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ACMClusterDeploymentDetailsStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDetailsFormFields.tsx(4 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/components/AdditionalNTPSourcesDialogToggle.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/components/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostDiscoveryTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscovery.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscoveryStep.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelection.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionAdvanced.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionBasic.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/index.ts(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentHostsNetworkTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingForm.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx(4 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/use-networking-formik.ts(2 hunks)libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/DetailsForm.tsx(3 hunks)libs/ui-lib/lib/cim/components/helpers/toAssisted.ts(2 hunks)libs/ui-lib/lib/cim/hooks/index.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/types.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx(1 hunks)libs/ui-lib/lib/cim/types/index.ts(1 hunks)libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts(1 hunks)libs/ui-lib/lib/cim/types/models.tsx(1 hunks)libs/ui-lib/lib/cim/utils.ts(2 hunks)libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx(1 hunks)libs/ui-lib/lib/common/components/clusterConfiguration/index.ts(1 hunks)libs/ui-lib/lib/common/components/clusterWizard/clusterDetailsValidation.ts(1 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx(3 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx(4 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx(4 hunks)libs/ui-lib/lib/common/components/ui/formik/SelectFieldWithSearch.tsx(1 hunks)libs/ui-lib/lib/common/components/ui/formik/index.tsx(1 hunks)libs/ui-lib/lib/ocm/components/Routes.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx(3 hunks)libs/ui-lib/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (20)
libs/ui-lib/lib/cim/components/ClusterDeployment/use-networking-formik.ts (2)
libs/ui-lib/lib/cim/components/ClusterDeployment/networkConfigurationValidation.ts (1)
getNetworkInitialValues(35-56)libs/ui-lib/lib/common/config/constants.ts (1)
CLUSTER_DEFAULT_NETWORK_SETTINGS_IPV4(273-277)
libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts (1)
ClusterDeploymentWizardStepsType(42-47)
libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx (1)
ReviewConfigMapsTable(7-70)
libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx (5)
libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizardContext.tsx (1)
ClusterDeploymentWizardContext(8-11)libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx (1)
useConfigMaps(5-14)libs/ui-lib/lib/common/components/ui/WizardFooter.tsx (1)
WizardFooter(40-132)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx (1)
ConfigMapDetail(134-199)libs/ui-lib/lib/cim/components/ClusterDeployment/components/ValidationSection.tsx (1)
ValidationSection(18-41)
libs/ui-lib/lib/cim/components/helpers/toAssisted.ts (1)
libs/types/assisted-installer-service.d.ts (1)
PlatformType(2456-2456)
libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx (2)
libs/ui-lib/lib/cim/hooks/types.tsx (1)
K8sWatchHookProps(8-8)libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx (1)
useK8sWatchResource(13-27)
libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx (3)
libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts (1)
AgentClusterInstallK8sResource(17-81)libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx (1)
useConfigMaps(5-14)libs/ui-lib/lib/common/components/ui/table/utils.ts (1)
genericTableRowKey(15-15)
libs/ui-lib/lib/ocm/components/Routes.tsx (1)
libs/ui-lib/lib/common/features/featureGate.tsx (1)
FeatureListType(6-8)
libs/ui-lib/lib/common/components/ui/formik/SelectFieldWithSearch.tsx (1)
libs/ui-lib/lib/common/components/ui/formik/types.ts (1)
FieldProps(16-38)
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (2)
libs/types/assisted-installer-service.d.ts (1)
PlatformType(2456-2456)libs/ui-lib/lib/common/components/ui/StaticTextField.tsx (1)
StaticField(23-57)
libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx (1)
libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)
libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts (1)
ClusterDeploymentHostSelectionStepProps(110-115)
libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingForm.tsx (1)
libs/ui-lib/lib/common/config/constants.ts (1)
CLUSTER_DEFAULT_NETWORK_SETTINGS_IPV4(273-277)
libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx (4)
libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts (1)
AgentClusterInstallK8sResource(17-81)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx (2)
CustomManifestFormType(24-30)ConfigMapForm(32-247)libs/ui-lib/lib/cim/utils.ts (1)
appendPatch(12-20)libs/ui-lib/lib/cim/types/models.tsx (1)
AgentClusterInstallModel(3-12)
libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsStep.tsx (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts (1)
ClusterDeploymentDetailsStepProps(85-89)
libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ACMClusterDeploymentDetailsStep.tsx (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/index.ts (1)
ACMClusterDeploymentDetailsStep(18-18)
libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (2)
libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (1)
HelperTextType(24-24)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx (2)
libs/ui-lib/lib/ocm/components/clusterWizard/OpenShiftVersionsContext.tsx (1)
useOpenShiftVersionsContext(125-131)libs/ui-lib/lib/ocm/components/clusterConfiguration/OpenshiftVersionHelperText.tsx (1)
getOpenshiftVersionHelperText(23-62)
libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx (1)
libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (1)
OpenShiftSelectWithSearch(14-91)
libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx (1)
ClusterDeploymentCustomManifestsStep(12-119)
⏰ 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: lint
- GitHub Check: format
...ib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx
Show resolved
Hide resolved
jgyselov
force-pushed
the
cim_external_platforms_implementation
branch
from
aa9a72d to
9f119eb
Compare
|
@jgyselov: This pull request references MGMT-20076 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 epic 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelection.tsx (1)
87-94: Invalid JSX generic usage (TypeScript).JSX cannot supply type parameters to components. This will not compile.
Apply:
- {!autoSelectHosts && ( - <ClusterDeploymentHostsSelectionAdvanced<ClusterDeploymentHostsSelectionValues> + {!autoSelectHosts && ( + <ClusterDeploymentHostsSelectionAdvanced availableAgents={availableAgents} onEditRole={onEditRole} onHostSelect={onHostSelect} onSetInstallationDiskId={onSetInstallationDiskId} cpuArchitecture={cpuArchitecture} />If the component is truly generic, type it at declaration; callers shouldn’t pass generics in JSX.
libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (2)
23-26: Localize the beta suffix textHardcoded “Developer preview release” bypasses i18n. Use
t()so locales can translate it.- version.supportLevel === 'beta' - ? version.label + ' - ' + 'Developer preview release' - : version.label, + version.supportLevel === 'beta' + ? `${version.label} - ${t('ai:Developer preview release')}` + : version.label,
51-53: Localize the “No results” messageExpose the string to translations and keep interpolation.
- children: `No results found for "${filterValue}"`, + children: t('ai:No results found for "{{value}}"', { value: filterValue }),
♻️ Duplicate comments (1)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
33-33: Type flip to string | null — align consumersChanging customOpenshiftSelect to string | null is the right direction. Please ensure no callers still expect an object with .value.
Run:
#!/bin/bash # Find lingering object-typed usages rg -nP "customOpenshiftSelect\\?\\.\s*value|customOpenshiftSelect:\s*OpenshiftVersionOptionType" -g "*.{ts,tsx}"
🧹 Nitpick comments (12)
libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/types.ts (1)
25-26: Validation schema omission confirmedExcluding customOpenshiftSelect from the Yup schema is consistent with it being a derived/aux value. If there’s a case where a custom version must be validated, consider a conditional schema later.
libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx (1)
121-133: Guard version parsing and arch list (optional hardening)Minor: version?.cpuArchitectures may be undefined; also parsing minor via split is brittle.
Proposed:
- const isMulti = version?.cpuArchitectures?.[0] === CpuArchitecture.MULTI; - - const highlyAvailableSupported = toNumber(version?.version?.split('.')?.[1]) >= 18; + const archs = version?.cpuArchitectures ?? []; + const isMulti = archs.includes(CpuArchitecture.MULTI); + const minor = toNumber(version?.version?.split('.')?.[1]); + const highlyAvailableSupported = Number.isFinite(minor) && minor >= 18; return [ - (isMulti ? cpuArchitectures : version?.cpuArchitectures) as SupportedCpuArchitecture[], + (isMulti ? cpuArchitectures : (archs as SupportedCpuArchitecture[])), highlyAvailableSupported && !isMulti, ];libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx (2)
71-77: Encode URL segments and localize the remove button label.
- Use encodeURIComponent for name/namespace in the href.
- Localize the aria-label via i18n.
@@ - <ExternalLink - href={`/k8s/ns/${configMap?.metadata?.namespace || ''}/configmaps/${ - configMap?.metadata?.name || '' - }/form`} - > + <ExternalLink + href={`/k8s/ns/${encodeURIComponent(configMap?.metadata?.namespace || '')}/configmaps/${encodeURIComponent(configMap?.metadata?.name || '')}/form`} + > @@ export const ConfigMapDetail = ({ @@ isLoading: boolean; }): { @@ - const [isExpanded, setIsExpanded] = React.useState(false); + const [isExpanded, setIsExpanded] = React.useState(false); const [showRemoveButton, setShowRemoveButton] = React.useState(false); + const { t } = useTranslation(); @@ - <Button - aria-label="remove config map" + <Button + aria-label={t('ai:Remove')} style={{ visibility: showRemoveButton ? 'visible' : 'hidden' }} variant="plain" onClick={onRemove} >Also applies to: 169-176, 145-146
80-87: Guard rendering of very large values.Long manifest contents can bloat the DOM and hurt performance. Consider truncation with a “Show more” toggle or a max-height with scroll.
I can provide a small wrapper that collapses values over N characters.
libs/locales/lib/en/translation.json (1)
200-203: Terminology consistency: “ConfigMap” vs “config map”.Kubernetes resource name is “ConfigMap.” Consider aligning keys/values for consistency (UI copy can still be sentence-case where needed).
I can prep a follow-up keys/values sweep if desired.
Also applies to: 245-251, 302-303
libs/ui-lib/lib/cim/components/ClusterDeployment/wizardTransition.ts (1)
102-103: Extract a dedicated validation map forcustom-manifestsReplace the mapping at
wizardTransition.ts:102–103so thatcustom-manifestsuses its ownWizardStepValidationMapinstead of reusingnetworkingStepValidationsMap:@@ wizardTransition.ts const networkingStepValidationsMap: WizardStepValidationMap = { … }; +const customManifestsStepValidationsMap: WizardStepValidationMap = { + cluster: { groups: [], validationIds: [] }, + host: { allowedStatuses: ['known', 'disabled'], groups: [], validationIds: [] }, + softValidationIds: [], +}; export const wizardStepsValidationsMap = { networking: networkingStepValidationsMap, - 'custom-manifests': networkingStepValidationsMap, + 'custom-manifests': customManifestsStepValidationsMap, review: reviewStepValidationsMap, };Optionally add a
canNextFromCustomManifestsStepwrapper for symmetry:+export const canNextFromCustomManifestsStep = ( + agentClusterInstall: AgentClusterInstallK8sResource, + agents: AgentK8sResource[], +) => canNextFromClusterDeploymentWizardStep(agentClusterInstall, agents, 'custom-manifests');libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelection.tsx (1)
71-78: Optional: onChange signature sanity-check.Ensure SwitchField’s onChange signature matches onAutoSelectChange; otherwise wrap it to match (e.g., (_e, checked) => onAutoSelectChange(checked)).
libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx (3)
85-85: Set alert variant to dangerExplicitly mark the alert as danger to ensure consistent styling.
- addAlert({ title: error.message || t('ai:An error occurred while starting installation.') }); + addAlert({ + title: error.message || t('ai:An error occurred while starting installation.'), + variant: AlertVariant.danger, + });
151-156: Return null instead of undefined; add testId for E2E targetingMinor JSX cleanup and test hook.
- {!!agentClusterInstall.spec?.manifestsConfigMapRefs?.length ? ( + {!!agentClusterInstall.spec?.manifestsConfigMapRefs?.length ? ( <DetailItem title={t('ai:Custom manifests')} - value={<ReviewConfigMapsTable agentClusterInstall={agentClusterInstall} />} + value={<ReviewConfigMapsTable agentClusterInstall={agentClusterInstall} />} + testId="custom-manifests" /> - ) : undefined} + ) : null}
119-124: Avoid trailing dot when baseDomain is emptyPrevents rendering like "name." when baseDomain is missing.
- value={`${clusterDeployment.metadata?.name || ''}.${ - clusterDeployment.spec?.baseDomain || '' - }`} + value={[clusterDeployment.metadata?.name, clusterDeployment.spec?.baseDomain] + .filter(Boolean) + .join('.')}libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (2)
32-35: Optional: derive selectOptions with useMemo (remove effect + extra state)
selectOptionsis pure derived state frominitialSelectOptionsandfilterValue. UsinguseMemoreduces complexity and re-renders.- const [filterValue, setFilterValue] = React.useState<string>(''); - const [selectOptions, setSelectOptions] = - React.useState<SelectOptionProps[]>(initialSelectOptions); + const [filterValue, setFilterValue] = React.useState<string>(''); ... - React.useEffect(() => { - let newSelectOptions: SelectOptionProps[] = initialSelectOptions; + const selectOptions = React.useMemo<SelectOptionProps[]>(() => { + let newSelectOptions: SelectOptionProps[] = initialSelectOptions; @@ - if (!newSelectOptions.length) { + if (!newSelectOptions.length) { newSelectOptions = [ { isDisabled: true, - children: `No results found for "${filterValue}"`, + children: t('ai:No results found for "{{value}}"', { value: filterValue }), value: 'no results', }, ]; } } - const selectOptionsWithDividers = newSelectOptions.map((option, index) => { - const match = (option.value as string).match(/\d+\.(\d+)\.\d+/); - const y = match ? match[1] : null; - - const previousY = - index > 0 - ? ((newSelectOptions[index - 1].value as string).match(/\d+\.(\d+)\.\d+/) || [])[1] - : null; - - return { - ...option, - showDivider: previousY !== null && y !== previousY, - }; - }); - - setSelectOptions(selectOptionsWithDividers); - }, [filterValue, initialSelectOptions]); + const getMinor = (v: string) => v.match(/^\d+\.(\d+)\./)?.[1]; + return newSelectOptions.map((option, index) => { + const y = getMinor(String(option.value)); + const previousY = index > 0 ? getMinor(String(newSelectOptions[index - 1].value)) : undefined; + return { ...option, showDivider: previousY != null && y != null && y !== previousY }; + }); + }, [filterValue, initialSelectOptions, t]);Also applies to: 38-76
9-12: Consistent OpenShift capitalization in type nameUse “OpenShift” casing for the props type to match the component and brand.
-type OpenshiftSelectWithSearchProps = { +type OpenShiftSelectWithSearchProps = { versions: OpenshiftVersionOptionType[]; getHelperText?: HelperTextType; }; -export const OpenShiftSelectWithSearch: React.FunctionComponent<OpenshiftSelectWithSearchProps> = ({ +export const OpenShiftSelectWithSearch: React.FunctionComponent<OpenShiftSelectWithSearchProps> = ({ versions, getHelperText, -}: OpenshiftSelectWithSearchProps) => { +}: OpenShiftSelectWithSearchProps) => {Also applies to: 14-17
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (62)
libs/locales/lib/en/translation.json(15 hunks)libs/ui-lib/lib/cim/components/Agent/AgentStatus.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx(5 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/ScaleUpForm.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ACMClusterDeploymentDetailsStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx(3 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDetailsFormFields.tsx(5 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/components/AdditionalNTPSourcesDialogToggle.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/components/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostDiscoveryTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscovery.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscoveryStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx(3 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelection.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionAdvanced.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionBasic.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/index.ts(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentHostsNetworkTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingForm.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingStep.tsx(3 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx(6 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/use-networking-formik.ts(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/wizardTransition.ts(2 hunks)libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/DetailsForm.tsx(3 hunks)libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/types.ts(1 hunks)libs/ui-lib/lib/cim/components/helpers/toAssisted.ts(2 hunks)libs/ui-lib/lib/cim/components/modals/EditProxyModal.tsx(1 hunks)libs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsx(1 hunks)libs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/index.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/types.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx(1 hunks)libs/ui-lib/lib/cim/types/index.ts(1 hunks)libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts(1 hunks)libs/ui-lib/lib/cim/types/models.tsx(1 hunks)libs/ui-lib/lib/cim/utils.ts(2 hunks)libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx(1 hunks)libs/ui-lib/lib/common/components/clusterConfiguration/index.ts(1 hunks)libs/ui-lib/lib/common/components/clusterWizard/clusterDetailsValidation.ts(1 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx(3 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx(4 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx(4 hunks)libs/ui-lib/lib/common/components/ui/formik/SelectFieldWithSearch.tsx(1 hunks)libs/ui-lib/lib/common/components/ui/formik/index.tsx(1 hunks)libs/ui-lib/lib/ocm/components/Routes.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx(3 hunks)libs/ui-lib/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- libs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsx
- libs/ui-lib/lib/cim/components/modals/EditProxyModal.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/components/AdditionalNTPSourcesDialogToggle.tsx
- libs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/index.tsx
🚧 Files skipped from review as they are similar to previous changes (46)
- libs/ui-lib/lib/cim/components/ClusterDeployment/components/index.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/networking/index.tsx
- libs/ui-lib/lib/cim/hooks/index.tsx
- libs/ui-lib/lib/common/components/clusterConfiguration/index.ts
- libs/ui-lib/package.json
- libs/ui-lib/lib/cim/components/Agent/AgentStatus.tsx
- libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts
- libs/ui-lib/lib/common/components/ui/formik/index.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/index.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/index.tsx
- libs/ui-lib/lib/cim/utils.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/ScaleUpForm.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx
- libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts
- libs/ui-lib/lib/ocm/components/Routes.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/review/index.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/index.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentHostsNetworkTable.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/index.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscovery.tsx
- libs/ui-lib/lib/common/components/clusterWizard/clusterDetailsValidation.ts
- libs/ui-lib/lib/cim/components/helpers/toAssisted.ts
- libs/ui-lib/lib/cim/types/models.tsx
- libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscoveryStep.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingStep.tsx
- libs/ui-lib/lib/common/components/ui/formik/SelectFieldWithSearch.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingForm.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionBasic.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsStep.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx
- libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx
- libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/DetailsForm.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ACMClusterDeploymentDetailsStep.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostDiscoveryTable.tsx
- libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/use-networking-formik.ts
- libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx
- libs/ui-lib/lib/cim/hooks/types.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDetailsFormFields.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionAdvanced.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx (1)
libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)
libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (2)
libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (1)
HelperTextType(24-24)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)
libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx (1)
ReviewConfigMapsTable(7-70)
⏰ 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: circular-deps
- GitHub Check: unit-tests
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (14)
libs/ui-lib/lib/cim/types/index.ts (1)
5-5: Re-export looks goodAdding models to the barrel is fine and unblocks downstream imports.
libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/types.ts (1)
11-11: Consistent value type for customOpenshiftSelectGood switch to string | null; matches the dropdown’s ID-based flow.
libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx (4)
11-18: Import path consolidation to types barrelLooks correct and keeps the surface clean. Ensure the barrel re-exports remain stable to avoid circulars.
24-31: Common types import updates look consistentImports align with the refactor to named exports from ../../../../common.
85-85: Typo fix in alert title“occurred” correction is good; user-facing copy matters.
95-95: Named export safe—no default imports remain
Confirmed viargsearch that all references (e.g. in ACMClusterDeploymentDetailsStep.tsx) use a named import. No further changes needed.libs/locales/lib/en/translation.json (3)
88-92: Typo fix confirmed (“occurred”).The “occured” → “occurred” corrections look good.
14-16: Pluralization pair looks correct.Matches existing pattern; usage with t('ai:{{count}} entry', { count }) will auto-select plural.
332-333: External platform strings added.Labels for external platforms and “No platform integration” look consistent.
Also applies to: 602-603, 637-638, 934-935
libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts (1)
47-48: New wizard step type added.Union extended with 'custom-manifests'—OK and consistent with the flow.
libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx (2)
11-19: Imports reorg LGTMThe path rewrites and barrel usage look consistent with the refactor.
Also applies to: 27-43
55-55: No default imports detected — named export usage confirmed
All imports of ClusterDeploymentReviewStep use the named form ({ ClusterDeploymentReviewStep }); no lingering default imports remain.libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (2)
59-71: Confirm SelectFieldWithSearch supportsshowDivideron optionsIf
SelectFieldWithSearchdoesn’t readshowDivider, this logic is dead code. If it does, consider typing an extension (e.g.,type SelectOptionWithDivider = SelectOptionProps & { showDivider?: boolean }) to make intent clear.
78-89: Formik wiring and UX defaults look goodNice consolidation to a single Formik-controlled field with searchable select and sensible helper text fallback.
libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx
Outdated
Show resolved
Hide resolved
jgyselov
force-pushed
the
cim_external_platforms_implementation
branch
from
9f119eb to
2c13948
Compare
|
@jgyselov: This pull request references MGMT-20076 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 epic 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. |
jgyselov
force-pushed
the
cim_external_platforms_implementation
branch
from
2c13948 to
05c5ad0
Compare
|
@jgyselov: This pull request references MGMT-20076 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 epic 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx (1)
256-263: Wrong step id passed to ValidationSection.Should be 'hosts-selection', not 'cluster-details'.
Apply:
- const errorsSection = ( - <ValidationSection currentStepId={'cluster-details'} hosts={[]}> + const errorsSection = ( + <ValidationSection currentStepId={'hosts-selection'} hosts={[]}>libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx (1)
121-133: Potential undefined cpuArchitectures; add safe fallback and safer minor parsingIf version is not found, cpuArchitectures can be undefined and break consumers. Also guard minor parsing.
const [cpuArchitectures, allowHighlyAvailable] = React.useMemo(() => { - const cpuArchitectures = [CpuArchitecture.x86, CpuArchitecture.ARM, CpuArchitecture.s390x]; + const defaultArchs = [CpuArchitecture.x86, CpuArchitecture.ARM, CpuArchitecture.s390x]; const version = allVersions.find( (ver) => ver.value === values.openshiftVersion || ver.version === forceOpenshiftVersion, ); - const isMulti = version?.cpuArchitectures?.[0] === CpuArchitecture.MULTI; + const isMulti = version?.cpuArchitectures?.[0] === CpuArchitecture.MULTI; - const highlyAvailableSupported = toNumber(version?.version?.split('.')?.[1]) >= 18; + const minorMatch = version?.version?.match(/^\d+\.(\d+)\./); + const minor = minorMatch ? toNumber(minorMatch[1]) : NaN; + const highlyAvailableSupported = Number.isFinite(minor) && minor >= 18; return [ - (isMulti ? cpuArchitectures : version?.cpuArchitectures) as SupportedCpuArchitecture[], + (isMulti ? defaultArchs : (version?.cpuArchitectures ?? defaultArchs)) as SupportedCpuArchitecture[], highlyAvailableSupported && !isMulti, ]; }, [allVersions, forceOpenshiftVersion, values.openshiftVersion]);libs/ui-lib/lib/cim/components/helpers/toAssisted.ts (1)
135-185: Add missingplatformfield toClustertype
Updateexport interface Clusterinlibs/types/assisted-installer-service.d.ts(around line 110) to includeplatform: { type: PlatformType }so it matches the
platformusage intoAssisted.ts.libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (1)
63-74: Effect missing deps may cause stale default/version labelRe-run when
versionsorcustomItemchange (common when data loads async).- }, [customOpenshiftSelect]); + }, [customOpenshiftSelect, versions, customItem]);
♻️ Duplicate comments (5)
libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (2)
20-22: Formik field typing alignedIncluding null in the field type matches schema.
61-73: Divider logic can misfire (null/undefined capture + loose regex)Undefined previousY passes the null check; stray dividers appear. Anchor regex and require both captures to exist.
- const selectOptionsWithDividers = newSelectOptions.map((option, index) => { - const match = (option.value as string).match(/\d+\.(\d+)\.\d+/); - const y = match ? match[1] : null; - - const previousY = - index > 0 - ? ((newSelectOptions[index - 1].value as string).match(/\d+\.(\d+)\.\d+/) || [])[1] - : null; - - return { - ...option, - showDivider: previousY !== null && y !== previousY, - }; - }); + const getMinor = (v: string) => v.match(/^\d+\.(\d+)\./)?.[1] ?? null; + const selectOptionsWithDividers = newSelectOptions.map((option, index) => { + const y = getMinor(String(option.value)); + const previousY = index > 0 ? getMinor(String(newSelectOptions[index - 1].value)) : null; + return { ...option, showDivider: previousY !== null && y !== null && y !== previousY }; + });libs/locales/lib/en/translation.json (1)
88-91: Typo fixes LGTM“occured” → “occurred” and new config maps error key added.
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (2)
55-60: Use fieldId (not name) for FormGroup association and accessibilitySwap name->fieldId so the label associates with the control; keep the id consistent with the toggle.
- <FormGroup - isInline - label={t('ai:Integrate with external partner platforms')} - isRequired - name={'platform'} - > + <FormGroup + isInline + label={t('ai:Integrate with external partner platforms')} + isRequired + fieldId="platform" + >
64-66: Add an id to the toggle and link it to fieldIdImproves a11y and testing; pairs with fieldId="platform".
- <MenuToggle ref={toggleRef} className="pf-v5-u-w-100" onClick={() => setIsOpen(!isOpen)}> + <MenuToggle + id="platform" + ref={toggleRef} + className="pf-v5-u-w-100" + onClick={() => setIsOpen(!isOpen)} + >
🧹 Nitpick comments (13)
libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx (2)
198-214: Shadowed variable name ‘errors’ inside onNext().It shadows Formik’s
errorsfrom context; rename for clarity.- const onNext = React.useCallback(async () => { + const onNext = React.useCallback(async () => { if (!showFormErrors) { setShowFormErrors(true); - const errors = await validateForm(); + const formErrors = await validateForm(); setTouched( - Object.keys(errors).reduce((acc, curr) => { + Object.keys(formErrors).reduce((acc, curr) => { acc[curr] = true; return acc; }, {} as Record<string, boolean>), ); - if (Object.keys(errors).length) { + if (Object.keys(formErrors).length) { return; } }
215-239: Avoid recomputing agent statuses; compute once.Minor perf/readability win.
React.useEffect(() => { if (nextRequested && !isSubmitting) { - const agentStatuses = selectedAgents.map( - (agent) => getWizardStepAgentStatus(agent, 'hosts-selection', t).status.key, - ); + const agentStatuses = selectedAgents.map((agent) => + getWizardStepAgentStatus(agent, 'hosts-selection', t).status.key, + ); if ( agentStatuses.some((status) => ['disconnected', 'disabled', 'error', 'insufficient', 'cancelled'].includes(status), ) ) { setNextRequested(false); } else if ( !!selectedAgents.length && - selectedAgents.every( - (agent) => getWizardStepAgentStatus(agent, 'hosts-selection', t).status.key === 'known', - ) + agentStatuses.every((status) => status === 'known') ) { setShowClusterErrors(true); if (canNextFromHostSelectionStep(agentClusterInstall, selectedAgents)) { void goToNextStep(); } } }libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx (1)
154-156: Custom Manifests step integration complete; optional conditional rendering for External only
Constants.ts and wizardTransition.ts already include ‘custom-manifests’. To reduce noise, you can conditionally render this step in ClusterDeploymentWizard.tsx for External clusters:- <WizardStep name={stepNames['custom-manifests']} id="custom-manifests"> - <ClusterDeploymentCustomManifestsStep agentClusterInstall={agentClusterInstall} /> - </WizardStep> + {agentClusterInstall?.spec?.platformType === 'External' && ( + <WizardStep name={stepNames['custom-manifests']} id="custom-manifests"> + <ClusterDeploymentCustomManifestsStep agentClusterInstall={agentClusterInstall} /> + </WizardStep> + )}libs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsx (2)
109-121: Progress should reflect approvals, not total agentsWhen many agents are already approved, the progress percentage is misleading. Also avoid dividing by zero and update progress after each successful approval.
const onClick = async () => { setError(undefined); - let i = 0; + const totalToApprove = agents.filter((a) => !a.spec.approved).length; + if (totalToApprove === 0) { + onClose(); + return; + } + let done = 0; try { for (const agent of agents) { if (!agent.spec.approved) { - setProgress((100 * (i + 1)) / agents.length); - await onApprove(agent); + await onApprove(agent); + done += 1; + setProgress((100 * done) / totalToApprove); } - i++; } setProgress(null); onClose();
171-174: Drop eslint-disable by wrapping the async handlerAvoid disabling no-misused-promises on the button.
- {/* eslint-disable-next-line @typescript-eslint/no-misused-promises */} - <Button onClick={onClick} isDisabled={progress !== null}> + <Button onClick={() => void onClick()} isDisabled={progress !== null}> {t('ai:Approve all hosts')} </Button>libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (2)
26-29: Localize “Developer preview release”Use i18n instead of hardcoding.
- version.supportLevel === 'beta' - ? version.label + ' - ' + 'Developer preview release' + version.supportLevel === 'beta' + ? `${version.label} - ${t('ai:Developer preview release')}` : version.label,
35-36: Type the extra showDivider fieldSelectOptionProps doesn’t define showDivider. Extend locally for correctness.
+type OptionWithDivider = SelectOptionProps & { showDivider?: boolean }; - const [selectOptions, setSelectOptions] = - React.useState<SelectOptionProps[]>(initialSelectOptions); + const [selectOptions, setSelectOptions] = + React.useState<OptionWithDivider[]>(initialSelectOptions as OptionWithDivider[]); @@ - let newSelectOptions: SelectOptionProps[] = initialSelectOptions; + let newSelectOptions: OptionWithDivider[] = initialSelectOptions as OptionWithDivider[];Also applies to: 61-77
libs/locales/lib/en/translation.json (1)
257-258: Use the localized “Developer preview release” in UIKey exists; ensure UI uses it (see OpenShiftSelectWithSearch).
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (2)
27-35: Remove per-item preventDefault; let Dropdown handle selectionPreventing default here is unnecessary and can interfere with selection semantics.
- selected={platform === value} - onClick={(e: React.MouseEvent) => e.preventDefault()} + selected={platform === value}
46-54: Defensive fallback for undefined value in read-only modeAvoid rendering an empty field if
valueis unset by falling back to “No platform integration”.- {platforms[value]} + {platforms[value] ?? platforms.baremetal}libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (3)
76-90: Insert dividers only between version groups (avoid leading/trailing dividers)Current logic appends a Divider to every group (including the first). Build a flat list and add dividers between groups only.
- const parsedVersionsForItems = getParsedVersions(items); - let lastY: string | undefined = ''; - const dropdownItems = parsedVersionsForItems.parsedVersions.map(({ y, versions }) => { - const items = versions.map(({ value, label }) => ( - <DropdownItem key={value} id={value} value={value}> - {label} - </DropdownItem> - )); - - if (lastY !== null && y !== lastY) { - items.push(<Divider key={`separator-${y || ''}`} />); - } - lastY = y; - return items; - }); + const parsedVersionsForItems = getParsedVersions(items); + const dropdownItems = parsedVersionsForItems.parsedVersions.flatMap(({ y, versions }, idx) => { + const groupItems = versions.map(({ value, label }) => ( + <DropdownItem key={value} id={value} value={value}> + {label} + </DropdownItem> + )); + return idx > 0 ? [<Divider key={`separator-${y || ''}`} />, ...groupItems] : groupItems; + });
92-104: Localize group labelsUse i18n for consistency.
- dropdownItems.length && ( - <DropdownGroup label="Latest releases" key="latest-releases"> + dropdownItems.length && ( + <DropdownGroup label={t('ai:Latest releases')} key="latest-releases"> @@ - customItem && ( - <DropdownGroup label="Custom releases" key="custom-releases"> + customItem && ( + <DropdownGroup label={t('ai:Custom releases')} key="custom-releases">Remember to add the new keys to locales.
145-146: Match new HelperTextType signaturePass null when empty to align with
(value: string | null).- const helperText = getHelperText && getHelperText(field.value); + const helperText = getHelperText && getHelperText(field.value || null);
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (42)
libs/locales/lib/en/translation.json(15 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx(5 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx(3 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDetailsFormFields.tsx(5 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscoveryStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx(3 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingStep.tsx(3 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx(6 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/wizardTransition.ts(2 hunks)libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/DetailsForm.tsx(3 hunks)libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/types.ts(1 hunks)libs/ui-lib/lib/cim/components/helpers/toAssisted.ts(2 hunks)libs/ui-lib/lib/cim/components/modals/EditProxyModal.tsx(1 hunks)libs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsx(1 hunks)libs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/index.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/types.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx(1 hunks)libs/ui-lib/lib/cim/types/index.ts(1 hunks)libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts(1 hunks)libs/ui-lib/lib/cim/types/models.tsx(1 hunks)libs/ui-lib/lib/cim/utils.ts(2 hunks)libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx(1 hunks)libs/ui-lib/lib/common/components/clusterWizard/clusterDetailsValidation.ts(1 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx(3 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx(5 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx(4 hunks)libs/ui-lib/lib/common/components/ui/formik/SelectFieldWithSearch.tsx(1 hunks)libs/ui-lib/lib/common/components/ui/formik/index.tsx(1 hunks)libs/ui-lib/lib/ocm/components/Routes.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OpenshiftVersionHelperText.tsx(1 hunks)libs/ui-lib/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/ui-lib/lib/cim/components/modals/EditProxyModal.tsx
🚧 Files skipped from review as they are similar to previous changes (29)
- libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts
- libs/ui-lib/lib/cim/types/index.ts
- libs/ui-lib/lib/cim/hooks/types.tsx
- libs/ui-lib/lib/cim/hooks/index.tsx
- libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/types.ts
- libs/ui-lib/lib/common/components/ui/formik/index.tsx
- libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts
- libs/ui-lib/lib/cim/utils.ts
- libs/ui-lib/lib/ocm/components/Routes.tsx
- libs/ui-lib/package.json
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/index.tsx
- libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts
- libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx
- libs/ui-lib/lib/common/components/clusterWizard/clusterDetailsValidation.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/wizardTransition.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx
- libs/ui-lib/lib/common/components/clusterWizard/types.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx
- libs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsx
- libs/ui-lib/lib/common/components/ui/formik/SelectFieldWithSearch.tsx
- libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/DetailsForm.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx
- libs/ui-lib/lib/cim/types/models.tsx
- libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscoveryStep.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-02T08:14:13.681Z
Learnt from: jgyselov
PR: openshift-assisted/assisted-installer-ui#3151
File: libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx:24-36
Timestamp: 2025-09-02T08:14:13.681Z
Learning: In ExternalPlatformsDropdown, "baremetal" is the correct platform type value for the "No platform integration" option and should be selectable as the default, while "none" is filtered out as it's unused/duplicate.
Applied to files:
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx
📚 Learning: 2025-09-02T08:03:57.186Z
Learnt from: jgyselov
PR: openshift-assisted/assisted-installer-ui#3151
File: libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts:25-25
Timestamp: 2025-09-02T08:03:57.186Z
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/locales/lib/en/translation.json
🧬 Code graph analysis (5)
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (2)
libs/types/assisted-installer-service.d.ts (1)
PlatformType(2456-2456)libs/ui-lib/lib/common/components/ui/StaticTextField.tsx (1)
StaticField(23-57)
libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (3)
libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (1)
HelperTextType(24-24)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
ClusterDetailsValues(19-36)
libs/ui-lib/lib/cim/components/helpers/toAssisted.ts (1)
libs/types/assisted-installer-service.d.ts (1)
PlatformType(2456-2456)
libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts (1)
ClusterDeploymentHostSelectionStepProps(111-116)
libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx (1)
ClusterDeploymentCustomManifestsStep(12-121)
⏰ 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: tests
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (17)
libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingStep.tsx (4)
13-20: Consolidated common imports — verify barrel re-exports and naming collisionsThe shift to '../../../../common' looks consistent with the refactor. Please verify that this barrel re-exports all of useAlerts, ClusterWizardStepHeader, FormikAutoSave, getFormikErrorFields, clusterFieldLabels, and Alerts, and that there’s no naming collision between PF’s Alert and our Alerts component in downstream consumers.
203-205: Typo fix LGTMSwitched to “An error occurred” — correct spelling and improved UX.
26-34: Import path reorg verified — all symbols are exported and no stale local imports remain
215-221: ClusterDeploymentNetworkingStep already a named export—no import updates needed
All references import it as a named export (import { ClusterDeploymentNetworkingStep } from './networking') andnetworking/index.tsxre-exports it viaexport *; there are no default imports to convert.Likely an incorrect or invalid review comment.
libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx (3)
13-34: Import path refactor looks good.Centralizing imports through common barrels and local indices improves cohesion and API clarity.
258-258: Typo fix “occurred” is correct.Good catch on the user-facing string.
337-339: Verify migration to named export
No default imports ofClusterDeploymentHostSelectionStepdetected; confirm that thelibs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/index.tsbarrel file re-exports the renamed component.libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx (2)
11-16: Move to named step imports — good modularization.
57-60: No changes needed: startIndex is 1-based
PatternFly React Wizard’sstartIndexprop is 1-based (defaults to 1); passingstartIndex={4}correctly targets the host step (patternfly.org).libs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsx (1)
124-126: Typo fix LGTMCorrected copy matches the new i18n key.
libs/ui-lib/lib/ocm/components/clusterConfiguration/OpenshiftVersionHelperText.tsx (2)
25-26: Signature change to string | null is correctMatches HelperTextType across the UI. No behavioral concerns.
23-41: No callers passundefinedforselectedVersionValue. All usages pass astring | nullvalue—no changes needed.libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx (1)
95-160: Named export imports verified: All occurrences importClusterDeploymentDetailsFormusing named syntax.libs/locales/lib/en/translation.json (1)
747-748: New helper texts LGTMMatches updated UI for searchable selects.
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (1)
24-36: LGTM: options list excludes 'none' but keeps 'baremetal' as defaultMatches the intended UX (“No platform integration” => 'baremetal').
libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDetailsFormFields.tsx (1)
92-100: Good: single customItem selection logic is consistent
additionalSelectOptioncorrectly guards against duplicates and feeds the dropdown.libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (1)
98-104: LGTM: single custom item rendering is cleanThe simplified “Custom releases” rendering removes unnecessary grouping complexity.
libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDetailsFormFields.tsx
Show resolved
Hide resolved
|
@jgyselov: This pull request references MGMT-20076 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 epic 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. |
|
@jgyselov: This pull request references MGMT-20076 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 epic 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. |
jgyselov
force-pushed
the
cim_external_platforms_implementation
branch
from
05c5ad0 to
10923fb
Compare
|
@jgyselov: This pull request references MGMT-20076 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 epic 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: 7
♻️ Duplicate comments (3)
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (1)
9-15: Platform options and defaults LGTMIncluding 'baremetal' as the selectable default and filtering only 'none' matches the intended UX. Keeping a label for 'none' helps display legacy values.
Also applies to: 24-26
libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (2)
38-38: Guard helperText invocation and align nullability
Use optional chaining and pass a null-coalesced value.- const helperText = getHelperText && getHelperText(value, true); + const helperText = getHelperText?.(value ?? null, true);
61-77: Fix divider computation (null/undefined bug) and anchor regexCurrent logic can set showDivider when either side doesn’t match the version regex (e.g., for "no_results"). Anchor the regex and ensure both captures are non-null before comparing.
- const selectOptionsWithDividers = newSelectOptions.map((option, index) => { - const match = (option.value as string).match(/\d+\.(\d+)\.\d+/); - const y = match ? match[1] : null; - - const previousY = - index > 0 - ? ((newSelectOptions[index - 1].value as string).match(/\d+\.(\d+)\.\d+/) || [])[1] - : null; - - return { - ...option, - showDivider: previousY !== null && y !== previousY, - }; - }); + const getMinor = (v: string) => v.match(/^\d+\.(\d+)\./)?.[1] ?? null; + const selectOptionsWithDividers = newSelectOptions.map((option, index) => { + const y = getMinor(String(option.value)); + const previousY = index > 0 ? getMinor(String(newSelectOptions[index - 1].value)) : null; + return { + ...option, + showDivider: previousY != null && y != null && y !== previousY, + }; + });
🧹 Nitpick comments (16)
libs/ui-lib/lib/cim/types/models.tsx (3)
1-1: Avoid deep import; use stable SDK entrypoint.Prevents breakage on SDK internals refactors.
-import { K8sModel } from '@openshift-console/dynamic-plugin-sdk/lib/api/common-types'; +import { K8sModel } from '@openshift-console/dynamic-plugin-sdk';
3-12: Mark as CRD and add a stable id.Not required for patching, but improves consistency with other models.
export const AgentClusterInstallModel: K8sModel = { label: 'AgentClusterInstall', apiVersion: 'v1beta1', plural: 'agentclusterinstalls', abbr: 'ACI', namespaced: true, kind: 'AgentClusterInstall', labelPlural: 'AgentClusterInstalls', apiGroup: 'extensions.hive.openshift.io', + crd: true, + id: 'agentclusterinstalls', };
1-12: Use .ts (no JSX present).Rename to models.ts for clarity and to tighten TSX processing scope.
libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx (2)
8-11: Add explicit group: '' for core/v1 GVK.OpenShift Console SDK typically expects group to be an empty string for core resources. Being explicit avoids subtle watch mismatches.
groupVersionKind: { + group: '', kind: 'ConfigMap', version: 'v1', },
1-1: Nit: use .ts instead of .tsx.No JSX here; .ts speeds builds and improves editor signals.
libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/DetailsForm.tsx (1)
40-48: Graceful fallback when version not in allVersionsIf
allVersionshasn’t loaded yet (or the value is unknown), consider synthesizing a display item so the dropdown still shows the user’s value.Apply:
if ( values.customOpenshiftSelect && !selectOptions.some((option) => option.value === values.customOpenshiftSelect) ) { - return allVersions.find((version) => version.value === values.customOpenshiftSelect); + const found = allVersions.find((v) => v.value === values.customOpenshiftSelect); + return ( + found ?? { value: values.customOpenshiftSelect, label: values.customOpenshiftSelect } + ); } return undefined;libs/ui-lib/lib/common/components/clusterWizard/clusterDetailsValidation.ts (1)
78-79: Defaulting to 'baremetal' is correct; consider normalizing legacy 'none' valuesGood alignment with the ExternalPlatformsDropdown behavior and prior product decision. Optionally coerce legacy
platform.type === 'none'to'baremetal'to avoid persisting the deprecated state.- platform: cluster?.platform?.type || 'baremetal', + platform: + cluster?.platform?.type && cluster.platform.type !== 'none' + ? cluster.platform.type + : 'baremetal',libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (2)
55-66: Use fieldId instead of name on FormGroup and link toggle via idImproves accessibility and consistency with the rest of the form controls.
- <FormGroup - isInline - label={t('ai:Integrate with external partner platforms')} - isRequired - name={'platform'} - > + <FormGroup + isInline + fieldId="platform" + label={t('ai:Integrate with external partner platforms')} + isRequired + > <Dropdown isOpen={isOpen} toggle={(toggleRef) => ( - <MenuToggle ref={toggleRef} className="pf-v5-u-w-100" onClick={() => setIsOpen(!isOpen)}> + <MenuToggle + id="platform" + ref={toggleRef} + className="pf-v5-u-w-100" + onClick={() => setIsOpen(!isOpen)} + > {value ? platforms[value] : t('ai:Integrate with external partner platforms')} </MenuToggle> )}
46-53: StaticField fallback to handle undefined/unknown valueGuard against unexpected
valueto avoid renderingundefined.- {platforms[value]} + {platforms[value] ?? t('ai:No platform integration')}libs/locales/lib/en/translation.json (1)
88-92: Grammar fixes LGTM; keep punctuation consistentGreat catch on “occurred”. Consider adding a period to the config maps error for consistency with adjacent messages.
- "ai:An error occurred while fetching config maps": "An error occurred while fetching config maps", + "ai:An error occurred while fetching config maps": "An error occurred while fetching config maps.",libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (1)
25-29: Internationalize the beta suffixHard-coded "Developer preview release" should be translated.
- children: - version.supportLevel === 'beta' - ? version.label + ' - ' + 'Developer preview release' - : version.label, + children: + version.supportLevel === 'beta' + ? `${version.label} - ${t('ai:Developer preview release')}` + : version.label,libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx (1)
154-156: Show the Custom Manifests step only when relevant (External platform)If the step is intended only for External platform, consider gating visibility for non-External to reduce noise. If it must remain visible, OK to ignore.
Would you like a follow-up patch to conditionally include the step when platformType === 'External'?
libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx (4)
107-110: Harden platform lookup to avoid undefined renderingIf platformType is unexpected, the lookup can return undefined. Add a safe fallback.
- const platform = - getPlatforms(t)[ - (agentClusterInstall?.spec?.platformType?.toLowerCase() || 'baremetal') as PlatformType - ]; + const platformStr = agentClusterInstall?.spec?.platformType?.toLowerCase(); + const platforms = getPlatforms(t); + const platform = platforms[(platformStr ?? 'baremetal') as PlatformType] ?? platforms.baremetal;
141-145: Label clarity for platform row“External partner platform” may confuse when the value is “No platform integration”. Consider a neutral label like “Platform” or “Platform integration”. Also consider hiding the row when value is “No platform integration” if that’s the UX intent.
162-168: Conditional render: prefer null over undefinedReturn null instead of undefined for React children; also a short-circuit read improves readability.
- {!!agentClusterInstall.spec?.manifestsConfigMapRefs?.length ? ( - <DetailItem - title={t('ai:Custom manifests')} - value={<ReviewConfigMapsTable agentClusterInstall={agentClusterInstall} />} - /> - ) : undefined} + {!!agentClusterInstall.spec?.manifestsConfigMapRefs?.length && ( + <DetailItem + title={t('ai:Custom manifests')} + value={<ReviewConfigMapsTable agentClusterInstall={agentClusterInstall} />} + /> + )}
87-87: i18n key consistency for error messagesTwo different keys/strings are used: a contextual message (“…while starting installation.”) and a generic “An error occurred”. Ensure the i18n catalog intentionally has both and that punctuation/style is consistent across the app.
Also applies to: 199-199
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (63)
libs/locales/lib/en/translation.json(15 hunks)libs/ui-lib/lib/cim/components/Agent/AgentStatus.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx(5 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/ScaleUpForm.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ACMClusterDeploymentDetailsStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx(3 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDetailsFormFields.tsx(5 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/components/AdditionalNTPSourcesDialogToggle.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/components/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostDiscoveryTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscovery.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscoveryStep.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx(3 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelection.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionAdvanced.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionBasic.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/index.ts(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentHostsNetworkTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingForm.tsx(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingStep.tsx(3 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/networking/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx(8 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/review/index.tsx(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts(1 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/use-networking-formik.ts(2 hunks)libs/ui-lib/lib/cim/components/ClusterDeployment/wizardTransition.ts(2 hunks)libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/DetailsForm.tsx(3 hunks)libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/types.ts(1 hunks)libs/ui-lib/lib/cim/components/helpers/toAssisted.ts(2 hunks)libs/ui-lib/lib/cim/components/modals/EditProxyModal.tsx(1 hunks)libs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsx(1 hunks)libs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/index.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/types.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx(1 hunks)libs/ui-lib/lib/cim/types/index.ts(1 hunks)libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts(1 hunks)libs/ui-lib/lib/cim/types/models.tsx(1 hunks)libs/ui-lib/lib/cim/utils.ts(2 hunks)libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx(1 hunks)libs/ui-lib/lib/common/components/clusterConfiguration/index.ts(1 hunks)libs/ui-lib/lib/common/components/clusterWizard/clusterDetailsValidation.ts(1 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx(3 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx(5 hunks)libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx(4 hunks)libs/ui-lib/lib/common/components/ui/formik/SelectFieldWithSearch.tsx(1 hunks)libs/ui-lib/lib/common/components/ui/formik/index.tsx(1 hunks)libs/ui-lib/lib/ocm/components/Routes.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OpenshiftVersionHelperText.tsx(1 hunks)libs/ui-lib/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (51)
- libs/ui-lib/lib/cim/types/k8s/agent-cluster-install.ts
- libs/ui-lib/lib/cim/types/index.ts
- libs/ui-lib/lib/cim/hooks/index.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/components/AdditionalNTPSourcesDialogToggle.tsx
- libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/types.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/components/index.tsx
- libs/ui-lib/lib/cim/components/modals/EditSSHKeyModal.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/networking/index.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/review/index.tsx
- libs/ui-lib/lib/common/components/clusterWizard/types.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/types.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapDetail.tsx
- libs/ui-lib/lib/cim/components/Agent/AgentStatus.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentHostsNetworkTable.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/index.tsx
- libs/ui-lib/lib/common/components/ui/formik/index.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/index.tsx
- libs/ui-lib/lib/cim/utils.ts
- libs/ui-lib/package.json
- libs/ui-lib/lib/common/components/ui/OpenShiftVersionModal.tsx
- libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingStep.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/index.tsx
- libs/ui-lib/lib/cim/components/modals/EditProxyModal.tsx
- libs/ui-lib/lib/cim/components/helpers/toAssisted.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/use-networking-formik.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionBasic.tsx
- libs/ui-lib/lib/ocm/components/Routes.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscoveryStep.tsx
- libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/index.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsStep.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelectionAdvanced.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ACMClusterDeploymentDetailsStep.tsx
- libs/ui-lib/lib/cim/components/modals/MassApproveAgentModal.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostSelectionStep.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ConfigMapForm.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/wizardTransition.ts
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostsDiscovery.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/networking/ClusterDeploymentNetworkingForm.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/ScaleUpForm.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/index.ts
- libs/ui-lib/lib/common/components/ui/formik/SelectFieldWithSearch.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOpenShiftVersionSelect.tsx
- libs/ui-lib/lib/cim/hooks/types.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostSelection/ClusterDeploymentHostsSelection.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/hostDiscovery/ClusterDeploymentHostDiscoveryTable.tsx
- libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDetailsFormFields.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T08:14:13.681Z
Learnt from: jgyselov
PR: openshift-assisted/assisted-installer-ui#3151
File: libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx:24-36
Timestamp: 2025-09-02T08:14:13.681Z
Learning: In ExternalPlatformsDropdown, "baremetal" is the correct platform type value for the "No platform integration" option and should be selectable as the default, while "none" is filtered out as it's unused/duplicate.
Applied to files:
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx
🧬 Code graph analysis (5)
libs/ui-lib/lib/cim/hooks/useConfigMaps.tsx (2)
libs/ui-lib/lib/cim/hooks/types.tsx (1)
K8sWatchHookProps(8-8)libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx (1)
useK8sWatchResource(13-27)
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (2)
libs/types/assisted-installer-service.d.ts (1)
PlatformType(2456-2456)libs/ui-lib/lib/common/components/ui/StaticTextField.tsx (1)
StaticField(23-57)
libs/ui-lib/lib/common/components/ui/OpenShiftSelectWithSearch.tsx (3)
libs/ui-lib/lib/common/components/ui/OpenShiftVersionDropdown.tsx (1)
HelperTextType(24-24)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
ClusterDetailsValues(19-36)
libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx (1)
libs/ui-lib/lib/cim/components/ClusterDeployment/customManifests/ClusterDeploymentCustomManifestsStep.tsx (1)
ClusterDeploymentCustomManifestsStep(12-121)
libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx (3)
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx (1)
getPlatforms(9-15)libs/types/assisted-installer-service.d.ts (1)
PlatformType(2456-2456)libs/ui-lib/lib/cim/components/ClusterDeployment/review/ReviewConfigMapsTable.tsx (1)
ReviewConfigMapsTable(7-70)
⏰ 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 (10)
libs/ui-lib/lib/cim/types/models.tsx (2)
3-12: LGTM: model fields look correct for ACI.Plural, group, version, namespaced, and kind align with AgentClusterInstall usage for k8sPatch.
1-12: All imports, re-exports, and usages of AgentClusterInstallModel are correct—no deep imports of K8sModel found.libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/DetailsForm.tsx (2)
73-73: Prop rename usage LGTM
customItem={additionalSelectOption}aligns with the updatedOpenShiftVersionDropdownAPI.
11-11: Import path move validated:BaseDnsHelperTextremains exported inlibs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDetailsFormFields.tsx(line 35) and there’s no circular import involving Hypershift.libs/ui-lib/lib/ocm/components/clusterConfiguration/OpenshiftVersionHelperText.tsx (1)
25-41: All call sites updated to usestring | null; noundefinedpassedlibs/ui-lib/lib/common/components/clusterConfiguration/index.ts (1)
12-12: Approve barrel export; no circular dependencies detected
Barrel export ofExternalPlatformsDropdownis correct and a project-wide import scan shows no cycles referencingclusterConfiguration.libs/locales/lib/en/translation.json (1)
245-251: New keys for manifests and external platforms LGTMLabels read well and match the new UI components and flow.
Also applies to: 332-334, 603-603, 638-638, 935-935
libs/ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx (1)
95-95: All imports updated to named export. No default imports remain.libs/ui-lib/lib/cim/components/ClusterDeployment/review/ClusterDeploymentReviewStep.tsx (2)
57-64: No default imports ofClusterDeploymentReviewStepfound; named export change has no impact.
11-11: Import paths and module resolution verified
All relative imports (../wizardTransition,../constants,../helpers) resolve to existing files; the scoped types import (@openshift-assisted/types/assisted-installer-service) resolves via Yarn workspace in node_modules; no tsconfigpathsor ESLintimport/resolverchanges needed; Jest moduleNameMapper already covers internal UI package alias.
libs/ui-lib/lib/cim/components/ClusterDeployment/ClusterDeploymentWizard.tsx
Show resolved
Hide resolved
.../ui-lib/lib/cim/components/ClusterDeployment/clusterDetails/ClusterDeploymentDetailsForm.tsx
Show resolved
Hide resolved
libs/ui-lib/lib/cim/components/Hypershift/HostedClusterWizard/DetailsStep/DetailsForm.tsx
Show resolved
Hide resolved
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx
Show resolved
Hide resolved
libs/ui-lib/lib/common/components/clusterConfiguration/ExternalPlatformsDropdown.tsx
Show resolved
Hide resolved
openshift-merge-robot
added
the
needs-rebase
|
@jgyselov: This pull request references MGMT-20076 which is a valid jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
jgyselov
force-pushed
the
cim_external_platforms_implementation
branch
from
10923fb to
e4fdc91
Compare
openshift-merge-robot
removed
the
needs-rebase
rawagner
left a comment
rawagner
left a comment
There was a problem hiding this comment.
Looks great!
I have a few comments which just shuffle the code around, but generally the existing logic is correct. We can address my comments in a followup if you prefer.
| configMaps: { | ||
| name: string; | ||
| valid: boolean; | ||
| manifestNames?: string[]; | ||
| }[]; |
There was a problem hiding this comment.
there is no need to have such a complicated structure. The Formik state can be just
| configMaps: { | |
| name: string; | |
| valid: boolean; | |
| manifestNames?: string[]; | |
| }[]; | |
| configMaps: string[] //name of CM |
| const validationSchema = Yup.lazy((values: CustomManifestFormType) => | ||
| Yup.object<CustomManifestFormType>({ | ||
| configMaps: Yup.array() | ||
| .of( | ||
| Yup.object().shape({ | ||
| name: Yup.string().required(t('ai:Config map name is required')), | ||
| valid: Yup.boolean().isTrue(t('ai:Config map not found')), | ||
| manifestNames: Yup.array() | ||
| .of(Yup.string()) | ||
| .test( | ||
| 'unique-manifests', | ||
| t( | ||
| 'ai:Manifest names in each config map should be unique across all referenced config maps.', | ||
| ), | ||
| (value) => | ||
| // check if another config map has any manifests with the same names as this one | ||
| values.configMaps | ||
| .flatMap(({ manifestNames }) => manifestNames) | ||
| .filter((v) => value?.includes(v)).length === value?.length, | ||
| ), | ||
| }), | ||
| ) | ||
| .min( | ||
| agentClusterInstall.spec?.platformType === 'External' ? 1 : 0, | ||
| t('ai:At least one config map is required'), | ||
| ), | ||
| }), | ||
| ); |
There was a problem hiding this comment.
We should centralize the validation logic here (ie valid field is set elsewhere which is not great and there is also no need to have a separate manifestNames in Formik state - generally anything that can be calculated, should not be stored in state).
After changing the CustomManifestFormType.configMaps to a simple string[], we could load the CMs in this component
const [cms, loaded, error] = useConfigMaps({
namespace: agentClusterInstall.metadata?.namespace,
isList: true,
});and then change the validation schema to (Im quite sure there will be some issue in my code, but it just demonstrates the idea ):
| const validationSchema = Yup.lazy((values: CustomManifestFormType) => | |
| Yup.object<CustomManifestFormType>({ | |
| configMaps: Yup.array() | |
| .of( | |
| Yup.object().shape({ | |
| name: Yup.string().required(t('ai:Config map name is required')), | |
| valid: Yup.boolean().isTrue(t('ai:Config map not found')), | |
| manifestNames: Yup.array() | |
| .of(Yup.string()) | |
| .test( | |
| 'unique-manifests', | |
| t( | |
| 'ai:Manifest names in each config map should be unique across all referenced config maps.', | |
| ), | |
| (value) => | |
| // check if another config map has any manifests with the same names as this one | |
| values.configMaps | |
| .flatMap(({ manifestNames }) => manifestNames) | |
| .filter((v) => value?.includes(v)).length === value?.length, | |
| ), | |
| }), | |
| ) | |
| .min( | |
| agentClusterInstall.spec?.platformType === 'External' ? 1 : 0, | |
| t('ai:At least one config map is required'), | |
| ), | |
| }), | |
| ); | |
| const validationSchema = Yup.lazy((values: CustomManifestFormType) => { | |
| const cmManifests = values.configMaps.reduce((acc, name) => { | |
| const cm = cms.find(({ metadata }) => metadata?.name === name); | |
| if (cm) { | |
| Object.keys(cm.data || {}).forEach((k) => { | |
| if (acc[k]) { | |
| acc[k]++; | |
| } else { | |
| acc[k] = 1; | |
| } | |
| }); | |
| } | |
| return acc; | |
| }, {} as { [key: string]: number }); | |
| return Yup.object<CustomManifestFormType>({ | |
| configMaps: Yup.array() | |
| .of( | |
| Yup.string() | |
| .required(t('ai:ConfigMap name is required')) | |
| .test('exists', 'ai:ConfigMap not found', (name) => { | |
| return cms.some(({ metadata }) => metadata?.name === name); | |
| }) | |
| .test('unique-manifests', 'ai:Manifests are not unique', (name) => { | |
| const cm = cms.find(({ metadata }) => metadata?.name === name); | |
| if (!cm) { | |
| return true; | |
| } | |
| return !Object.keys(cm.data || {}).some((k) => cmManifests[k] > 1); | |
| }), | |
| ) | |
| .min( | |
| agentClusterInstall.spec?.platformType === 'External' ? 1 : 0, | |
| t('ai:At least one config map is required'), | |
| ), | |
| }); | |
| }); |
which ensures that our validation logic is in one place
| onSubmit={onSubmit} | ||
| validationSchema={validationSchema} | ||
| > | ||
| <ConfigMapForm namespace={agentClusterInstall.metadata?.namespace as string} /> |
There was a problem hiding this comment.
if you load the CMs in this component, you could handle the loading/error state before rendering ConfigMapForm - which makes it a bit simpler IMO
| <DescriptionListGroup> | ||
| <DescriptionListTerm>{t('ai:Namespace')}</DescriptionListTerm> | ||
| <DescriptionListDescription> | ||
| {configMap?.metadata?.namespace} | ||
| </DescriptionListDescription> | ||
| </DescriptionListGroup> |
There was a problem hiding this comment.
Im not sure we need this as the Namespace is always the same as ACI
| React.useEffect(() => { | ||
| if (data && loaded && !isError) { | ||
| setConfigMaps( | ||
| data.map((configMap) => ({ | ||
| value: { | ||
| name: configMap.metadata?.name as string, | ||
| valid: true, | ||
| manifestNames: Object.keys(configMap.data || {}), | ||
| }, | ||
| children: configMap.metadata?.name as string, | ||
| })), | ||
| ); | ||
| } | ||
| }, [data, isError, loaded]); |
There was a problem hiding this comment.
no need for useEffect & useState. Just use useMemo
| const placeholder = React.useMemo(() => { | ||
| if (values.configMaps.length > 0) { | ||
| return `${values.configMaps.length} config map${ | ||
| values.configMaps.length !== 1 ? 's' : '' | ||
| } selected`; | ||
| } | ||
|
|
||
| return 'No config map selected'; | ||
| }, [values.configMaps]); |
| const submittingText = React.useMemo(() => { | ||
| if (isSubmitting) { | ||
| return t('ai:Saving changes...'); | ||
| } | ||
| return undefined; | ||
| }, [isSubmitting, t]); |
There was a problem hiding this comment.
no need to memoize this separately, just move it inside const footer = React.useMemo(...
| setFilterValue={setFilter} | ||
| placeholder={placeholder} | ||
| helperText={t( | ||
| 'ai:Select config maps from the list or use the type ahead to narrow down the list.', |
There was a problem hiding this comment.
I'd change all occurrences of config map/config maps to ConfigMap/ConfigMaps to properly reference the name of k8s resource. It is a standard in OCP Console
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgyselov, 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 3ed763a
into
openshift-assisted:releases/v2.16-cim
|
/cherry-pick master |
|
@jgyselov: #3151 failed to apply on top of branch "master": 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. |
* 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>
* 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>
5 participants
https://issues.redhat.com/browse/MGMT-20076
console pr: stolostron/console#4953
Changing the external platform:
external_platforms.mp4
Adding custom manifests:
adding_config_maps.mp4
With "external" platform:
Missing config map:
Config maps with data of same name:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation