NO-ISSUE: First step in ABI is Operators too#2899
NO-ISSUE: First step in ABI is Operators too#2899openshift-merge-bot[bot] merged 2 commits intoopenshift-assisted:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rawagner: This pull request explicitly references no 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. |
WalkthroughThe change modifies the cluster wizard's transition logic by updating the Changes
Sequence Diagram(s)sequenceDiagram
participant UI as User Interface
participant WZ as getClusterWizardFirstStep
participant Step as Step Resolver
UI->>WZ: Request initial wizard step (with state & staticIpInfo)
Note right of WZ: Removed conditional flag check
WZ->>Step: Return 'operators'
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
libs/ui-lib/lib/cim/components/helpers/versions.tsOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@openshift-assisted/eslint-config" to extend from. Please check that the name of the config is correct. The config "@openshift-assisted/eslint-config" was referenced from the config file in "/libs/ui-lib/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@openshift-assisted/eslint-config" to extend from. Please check that the name of the config is correct. The config "@openshift-assisted/eslint-config" was referenced from the config file in "/libs/ui-lib/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. libs/ui-lib/lib/common/components/clusterDetail/ClusterProgressItems.tsxOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the config "@openshift-assisted/eslint-config" to extend from. Please check that the name of the config is correct. The config "@openshift-assisted/eslint-config" was referenced from the config file in "/libs/ui-lib/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
bot
added
approved
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
size/XXL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx (1)
1-64: Consider adding i18n support for user-facing text.The descriptive text in the bundle specifications is currently hardcoded. For better internationalization support, consider extracting these strings to a localization system, especially if this UI will be used in multiple languages.
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
72-75: Consider avoiding redundant data in types.You already have
cluster.idinside theclusterproperty, so specifying bothclusterIdandclustermay be redundant. IfclusterIdis only used as a unique identifier, consider usingcluster.iddirectly.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (1)
136-146: Enhance error handling for parent operator lookupThe current implementation assumes that
parentOperator?.operatorNamewill have a corresponding entry inopSpecs. Add a fallback to prevent potential runtime errors.- parentOperatorName = opSpecs[parentOperator.operatorName]?.title || parentOperator.operatorName; + parentOperatorName = opSpecs[parentOperator.operatorName]?.title || parentOperator.operatorName || 'another operator';libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
87-104: Add error handling for invalid operator keysWhile there's a check for undefined
opSpecs[operatorKey], it might be helpful to log a warning when this occurs to help identify configuration issues.if (!opSpecs[operatorKey]) { + console.warn(`Operator specification not found for key: ${operatorKey}`); return null; }libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (2)
69-73: Consider handling other feature states.
Right now, the code checks if'LVM'is'supported'only. If a feature can return'techpreview','unsupported', etc., confirm that such cases won’t cause unintended behavior.
100-100: Improve readability when listing multiple failed operators.
If more than two operators fail, the string'operator1 and operator2 and operator3'can become awkward. Consider using a more natural-sounding sentence format (e.g., commas and a final "and").libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
70-353: Operator specs look comprehensive but could be further modularized.
There is repetition in theDescriptionandRequirementsproperties across many operators. Consider creating shared helper components or a standardized dictionary to reduce duplication and improve maintainability.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (1)
28-35: Fix typo in alert title.
Minor spelling error: “bunles” should be “bundles.”- title: 'Failed to fetch operator bunles', + title: 'Failed to fetch operator bundles',libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
58-67: Consider deduplicating bundle operators.
Repeatedly pushing operator dependencies may lead to duplicates. A quick refactor can return a unique set of operators:- return bundleOperators; + return [...new Set(bundleOperators)];
69-100: Fix the function name typo for better readability.
RenamegetNewBudleOperators→getNewBundleOperators.-export const getNewBudleOperators = ( +export const getNewBundleOperators = (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx(1 hunks)libs/ui-lib/lib/cim/components/helpers/versions.ts(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx(3 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(1 hunks)libs/ui-lib/lib/common/config/constants.ts(1 hunks)libs/ui-lib/lib/common/selectors/clusterSelectors.ts(1 hunks)libs/ui-lib/lib/common/types/clusters.ts(1 hunks)libs/ui-lib/lib/common/utils.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts(0 hunks)libs/ui-lib/lib/ocm/components/clusters/utils.ts(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(7 hunks)libs/ui-lib/lib/ocm/components/utils.ts(0 hunks)libs/ui-lib/lib/ocm/services/OperatorsService.tsx(1 hunks)
💤 Files with no reviewable changes (37)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx
- libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx
- libs/ui-lib/lib/ocm/components/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusters/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx
✅ Files skipped from review due to trivial changes (2)
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css
- libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx
🧰 Additional context used
🧬 Code Graph Analysis (9)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (6)
libs/ui-lib/lib/ocm/store/slices/current-cluster/selectors.ts (2)
selectIsCurrentClusterSNO(7-8)selectCurrentClusterPermissionsState(9-17)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(62-68)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFieldId(15-18)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
getNewOperators(33-56)
libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx (2)
libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(70-353)
libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx (5)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(70-353)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/TableSummaryExpandable.tsx (1)
TableSummaryExpandable(4-26)libs/ui-lib/lib/common/components/ui/table/utils.ts (1)
genericTableRowKey(15-15)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (3)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
ClusterOperatorProps(72-75)libs/types/assisted-installer-service.d.ts (1)
Bundle(89-109)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)
libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (2)
libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(70-353)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (5)
libs/ui-lib/lib/common/utils.ts (1)
getMajorMinorVersion(110-113)libs/ui-lib/lib/common/types/featureSupportLevel.ts (1)
FeatureId(8-15)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/config/constants.ts (22)
OPERATOR_NAME_MTV(302-302)OPERATOR_NAME_AMD_GPU(312-312)OPERATOR_NAME_LSO(298-298)OPERATOR_NAME_AUTHORINO(311-311)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_FENCE_AGENTS_REMEDIATION(316-316)OPERATOR_NAME_KMM(313-313)OPERATOR_NAME_KUBE_DESCHEDULER(318-318)OPERATOR_NAME_MCE(301-301)OPERATOR_NAME_NMSTATE(310-310)OPERATOR_NAME_NODE_FEATURE_DISCOVERY(303-303)OPERATOR_NAME_NODE_HEALTHCHECK(314-314)OPERATOR_NAME_NODE_MAINTENANCE(317-317)OPERATOR_NAME_NVIDIA_GPU(304-304)OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_OPENSHIFT_AI(308-308)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_PIPELINES(305-305)OPERATOR_NAME_SELF_NODE_REMEDIATION(315-315)OPERATOR_NAME_SERVERLESS(307-307)OPERATOR_NAME_SERVICEMESH(306-306)OPERATOR_NAME_LVM(300-300)libs/ui-lib/lib/common/config/docs_links.ts (22)
MTV_LINK(140-141)getLsoLink(155-158)AUTHORINO_OPERATOR_LINK(153-153)CNV_LINK(91-91)FENCE_AGENTS_REMEDIATION_LINK(114-115)KMM_LINK(93-94)NODE_HEALTHCHECK_LINK(108-109)getMceDocsLink(98-101)getNmstateLink(160-163)getNodeFeatureDiscoveryLink(165-168)getNvidiaGpuLink(170-173)ODF_REQUIREMENTS_LINK(82-83)ODF_LINK(96-96)OPENSHIFT_AI_REQUIREMENTS_LINK(85-86)OPENSHIFT_AI_LINK(143-144)OSC_REQUIREMENTS_LINK(88-89)OSC_LINK(146-146)PIPELINES_OPERATOR_LINK(175-176)SELF_NODE_REMEDIATION_LINK(111-112)SERVERLESS_OPERATOR_LINK(183-184)getServiceMeshLink(178-181)getLvmsDocsLink(103-106)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (4)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(70-353)libs/ui-lib/lib/common/config/constants.ts (5)
OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_LVM(300-300)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_OPENSHIFT_AI(308-308)libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
isSNO(59-60)libs/ui-lib/lib/common/types/cpuArchitecture.ts (2)
architectureData(52-72)SupportedCpuArchitecture(40-43)
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (4)
libs/ui-lib/lib/common/types/ui-settings.ts (1)
UISettingsValues(1-6)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
selectOlmOperators(40-44)
🔇 Additional comments (65)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx (4)
5-11: Well-structured type definition for bundle specifications.The
BundleSpectype is clear and provides good flexibility with its optional properties. The type captures the necessary information for defining bundle requirements and compatibility constraints.
13-29: Good implementation of virtualization bundle requirements.The virtualization bundle specification clearly outlines hardware requirements and resource needs for both control plane and worker nodes. This information will be valuable for users to understand the prerequisites before enabling this feature.
30-47: Clear specification for NVIDIA GPU-based OpenShift AI.The bundle properly defines compatibility constraints (
incompatibleBundles), hardware requirements, and includes an important note about secure boot configuration. The documentation link helps users find more detailed information.
48-64: Well-defined AMD GPU-based OpenShift AI specification.Similar to the NVIDIA variant, this bundle correctly specifies compatibility constraints, hardware requirements, and secure boot considerations specific to AMD GPUs. The consistent structure between related bundles is good for maintainability.
libs/ui-lib/lib/common/utils.ts (1)
110-113: Function relocated for better reusability.The
getMajorMinorVersionfunction has been moved from a component-specific file to this common utility file, making it accessible throughout the codebase. This is a good refactoring choice that improves code organization.libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
46-53: Parameter type broadened for flexibility.Changing the
searchOperatorparameter type fromExposedOperatorNametostringmakes this function more versatile. This change aligns with the broader refactoring to handle operators more dynamically rather than using hardcoded enum values.libs/ui-lib/lib/cim/components/helpers/versions.ts (1)
11-11: Using imported utility function.Now importing
getMajorMinorVersionfrom the common utils module instead of defining it locally. This change supports the centralization of utility functions and ensures consistent version parsing across the codebase.libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx (1)
31-31:Details
✅ Verification successful
Always display operators table.
The conditional rendering of operators has been removed in favor of always displaying the
ReviewOperatorsTable. This simplifies the component logic, but ensure that theReviewOperatorsTablecomponent itself handles empty or undefinedmonitoredOperatorsgracefully.
🏁 Script executed:
#!/bin/bash # Check if ReviewOperatorsTable handles empty monitoredOperators # Look for null/undefined checks in ReviewOperatorsTable component echo "Checking how ReviewOperatorsTable handles empty monitoredOperators:" rg -A 5 "ReviewOperatorsTable" libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ # Check the implementation of ReviewOperatorsTable echo "Checking ReviewOperatorsTable implementation:" fd "ReviewOperatorsTable" --exec cat {} \;Length of output: 5100
Verification Successful: Operators Table Implementation Approved
The
<ReviewOperatorsTable>component is now always rendered via<ReviewOperatorsTable cluster={cluster} />inReviewSummary.tsx, and its internal logic correctly handles scenarios with empty or undefinedmonitoredOperatorsby returningnullwhen no valid rows are generated.
- In
ReviewSummary.tsx, the conditional rendering is removed, ensuring the operators table is always invoked.- In
ReviewOperatorsTable.tsx, the component filters the operators and returnsnullif there are no valid entries, which handles empty states gracefully.libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx (3)
21-22: Imports look good.These new imports correctly bring in the functions needed for feature support levels and operator specifications.
86-88: Effective use of memoization and contextual feature checks.The usage of
React.useMemoto deriveopSpecsbased on theLVMfeature level ensures optimal performance and readability.
100-100: Graceful fallback for operator name.Falling back to
operator.nameif the spec title is unavailable prevents runtime errors and improves user experience.libs/ui-lib/lib/common/types/clusters.ts (1)
61-63: Simplified operator selection model.Switching to
selectedBundlesandselectedOperatorsarrays offers more flexibility than multiple boolean flags.libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx (4)
3-7: New imports integrated properly.These imports add the necessary utilities for row keys, expandable tables, operator specs, and feature support checks.
10-31: Robust filtered mapping approach.Leveraging
cluster.monitoredOperators?.filterwith a check againstopSpecshandles undefined operators gracefully and ensures only valid operators are rendered.
33-35: Intuitive early return for empty rows.Returning
nullif no operators exist avoids rendering an empty table, improving UX.
38-57: Compact and well-structured table output.Wrapping the table with
TableSummaryExpandableorganizes the operators review under a single, expandable section, enhancing readability and maintainability.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (5)
35-73: Well implemented function for generating bundle labelsThe
getBundleLabelfunction is well-structured and provides a good user experience by displaying bundle information with interactive elements. The implementation correctly handles conditional rendering of descriptions and documentation links.
97-109: Good handling of incompatible bundlesThe logic for detecting incompatible bundles and providing clear error messages is well implemented. This helps provide immediate feedback to users when they select bundles that cannot be installed together.
111-125: Well implemented selection logicThe
onSelectfunction properly updates both the selected bundles and operators, ensuring that the form state remains consistent. The use ofgetNewBudleOperatorsis appropriate for handling the complex logic of updating operators based on bundle selection.
182-204:Details
❓ Verification inconclusive
Review filtering logic based on feature flag
The component filters bundles to only show 'virtualization' when
isSingleClusterFeatureEnabledis true. Make sure this aligns with the intended behavior of the feature flag. The current implementation will significantly restrict the available bundles when this feature is enabled.
🏁 Script executed:
#!/bin/bash # Check if the feature flag is referenced elsewhere for similar filtering rg "isSingleClusterFeatureEnabled.*filter" -A 3 -B 3 --iglob "*.{ts,tsx}"Length of output: 75
Action Required: Confirm the Bundles Filtering Behavior
The current implementation filters bundles to show only the “virtualization” bundle when
isSingleClusterFeatureEnabledis enabled, which may overly restrict the available options. Since our initial search for similar filtering patterns returned no results, please manually verify that this behavior is intentional and aligns with the overall design/specifications for the single-cluster feature.
- File:
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(Lines 182-204)- Snippet:
{( isSingleClusterFeatureEnabled ? bundles.filter((b) => b.id === 'virtualization') : bundles ).map((bundle) => ( <GalleryItem key={bundle.id}> <BundleCard bundle={bundle} bundles={bundles} preflightRequirements={preflightRequirements} /> </GalleryItem> ))}- Action: Confirm via a manual review or cross-reference with product requirements that restricting bundles to only “virtualization” under the feature flag is the desired behavior.
91-94:Details
❓ Verification inconclusive
Consider handling potential undefined operator specs
The current implementation might cause runtime errors if an operator doesn't have a corresponding spec in the
featureSupportLevelDataobject.
🏁 Script executed:
#!/bin/bash # Check if there are any bundle operators without corresponding specs echo "Searching for potential missing operator specs..." rg -U 'bundle\.operators.*\n.*\.some\(' -A 2 | grep -o 'opSpecs\[.*\]' | grep -o '\[.*\]' | tr -d '[]' > /tmp/accessed_operators.txt # Find all defined operator specs rg -o 'OPERATOR_NAME_[A-Z_]+' libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx | sort -u > /tmp/defined_operators.txt echo "Checking if all accessed operators are defined..." for op in $(cat /tmp/accessed_operators.txt); do if ! grep -q "$op" /tmp/defined_operators.txt; then echo "WARNING: Operator '$op' is accessed but may not have a defined spec" fi doneLength of output: 461
Handle Potential Undefined Operator Specs
Our verification indicates that all operator names currently accessed via
opSpecs[operator].featureIdhave corresponding specs defined. However, to future-proof the code against runtime errors when new operators might be introduced without a defined spec, please consider adding a defensive check (e.g., ensuring thatopSpecs[operator]is defined before accessing itsfeatureId).
- Location:
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(lines 91–94)libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (2)
19-19: Good upgrade to new feature support level hookThe change from the previous support level handling to
useNewFeatureSupportLevelis appropriate and aligns with other components in the codebase. The addition of the LVMS support check enhances the component's ability to display the correct operator specifications.Also applies to: 22-22, 86-88
100-100: Improved operator name lookupUsing
opSpecsto determine operator names is a more consistent approach and aligns with the rest of the codebase. This change ensures that operator names are displayed consistently across different components.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (4)
33-60: Well-designed requirements displayThe
OperatorRequirementscomponent effectively displays additional hardware requirements needed for each operator. The conditional rendering based on available requirements is well-implemented, providing clear information to users.
61-101: Comprehensive requirements presentation with good fallbackThe component handles the display of requirements effectively, using a popover for detailed information. The fallback when no requirements are needed is a good user experience touch.
148-155: Comprehensive disabled state handlingThe component thoroughly handles various cases where an operator should be disabled, providing clear explanations to the user through tooltips. This enhances usability by making it clear why certain options aren't available.
156-201: Well-structured form control with accessibility considerationsThe checkbox implementation includes proper accessibility attributes such as
aria-describedbyand uses consistent IDs generated withgetFieldId. The component also includes comprehensive descriptions and tooltips for better user understanding.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (3)
40-55: Good API error handling patternThe component properly handles API errors when fetching supported operators, displaying an alert with the error message and ensuring the loading state is properly updated regardless of the outcome.
60-73: Efficient operator sorting and filteringThe sorting and filtering logic for operators is well-implemented and properly memoized. The conditional filtering based on the feature flag ensures that only relevant operators are displayed.
80-85: Clear and descriptive expandable section labelThe label for the expandable section effectively communicates both the total number of operators and how many are currently selected, providing users with important context at a glance.
libs/ui-lib/lib/ocm/services/OperatorsService.tsx (2)
1-1: No issues found with the updated import.
It cleanly references the needed entities from../../common.
8-43:Details
✅ Verification successful
Ensure removed methods are not referenced elsewhere.
SincegetOLMOperatorswas completely removed, please confirm that no references to this method remain in the codebase or documentation.Run this script to verify:
🏁 Script executed:
#!/bin/bash # Search for any references to the removed 'getOLMOperators' in the repository rg "getOLMOperators"Length of output: 20
Verification Complete: No references to "getOLMOperators" found in the repository.
After re-verifying with an expanded, case-insensitive search across the codebase (including potential documentation files), there were no matches for "getOLMOperators". This confirms that the removed method is not referenced anywhere and no further action is required.
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
15-16: Imports appear consistent with usage.
These newly introduced imports (useNewFeatureSupportLevel,getOperatorSpecs) fit the usage below and do not appear to introduce any issues.libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
62-69: Type definitions are clear.
DefiningOperatorSpecwith optionalDescriptionandRequirementsis well-structured and helps keep operator details organized.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (7)
1-2: New imports look valid.
The usage ofStackandStackItemfrom PatternFly is appropriate for layout.
17-21: State management is straightforward.
The boolean and array states are cleanly initialized.
42-44: Loading state is handled cleanly.
Returning<LoadingState />early simplifies the component.
49-49: Step header usage is appropriate.
No issues with the<ClusterWizardStepHeader>title.
52-53: OperatorsBundle usage.
The layout and property passing toOperatorsBundleare clear.
54-60: OperatorsSelect usage.
Props are aligned well, and hooking uppreflightRequirementsappears correct.
63-63: Component end looks good.
No further issues found here.libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (8)
7-11: Use of operator name constants
These lines add references to the operator name constants (e.g.,OPERATOR_NAME_CNV,OPERATOR_NAME_LVM, etc.), which centralizes naming and helps avoid typos.
20-20: New import for getOperatorSpecs
AddinggetOperatorSpecsimport tidily supports fetching operator details in a dynamic way, aligning with the approach used elsewhere.
49-51: Dynamic ODF operator title
Fetching the ODF operator title fromopSpecsunifies operator naming and avoids hardcoding. Good use of optional chaining to handle missing data.
55-55: Refined messages for ODF unavailability
UsingoperatorTitlein these return statements clarifies the reasoning behind feature unavailability under different conditions (ARM, SNO, etc.).Also applies to: 58-58, 61-61, 64-64
78-79: Applying getOperatorSpecs for CNV
Similar approach for CNV ensures consistency across operators. Nicely done.
108-109: Retrieving LVM operator labeling
Switching toopSpecsfor the LVM title maintains alignment with the broader dynamic labeling strategy.
128-131: OSC operator disabled reason
LeveraginggetOperatorSpecsto incorporate dynamic operator titles for OSC simplifies and standardizes messages.
302-302: OpenShift AI unavailability logic
These lines properly reference the AI operator title under various unsupported conditions (ARM, SNO, etc.). The localized message pattern is consistent with the rest of the file.Also applies to: 303-303, 306-306, 309-309, 312-312
libs/ui-lib/lib/common/config/constants.ts (1)
320-330: Updated singleClusterOperators array
The addition ofOPERATOR_NAME_ODF,OPERATOR_NAME_LSO, andOPERATOR_NAME_KUBE_DESCHEDULERextends the array for single-cluster usage. Ensure that these operators are indeed intended for single-cluster logic.libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (13)
5-5: Importing Cluster type
Importing theClusterinterface clarifies typing for the form and ensures correct usage of the cluster object.
10-11: Importing selectOlmOperators and UISettingsValues
These imports support the newly introduced logic for filtered OLM operators and UI settings.
19-19: Importing ClustersService
Bringing inClustersServiceis essential for updating cluster data after form submission.
24-30: New getOperatorsInitialValues function
Refactoring the initial form values to includeuiSettingsandselectOlmOperators(cluster)neatly consolidates logic for default operator selections and bundles.
38-39: Formik destructuring
Extractingerrors,touched,isSubmitting,isValid, andsubmitFormfrom Formik context simplifies usage and keeps the form logic clean.
50-51: Async handleNext ensures form submission
AwaitingsubmitForm()before navigation avoids race conditions, guaranteeing updates persist before moving forward.
67-67: Invoking handleNext in onNext
Wrapping thehandleNext()call in an arrow function is consistent with the broader approach and clarifies intent.
73-73: OperatorsStep component usage
PassingclusterIdandclusterdirectly gives the child component the data it needs, aligning with your new dynamic operator logic.
80-80: Retrieving uiSettings from wizard context
PullinguiSettingsalongsideupdateUISettingsintegrates form data with the wizard's broader state, maintaining consistency.
83-83: Asynchronous handleSubmit
Defining an async submit function accommodates server updates gracefully, preventing partial form states.
86-88: Building enabledOperators
Mappingvalues.selectedOperatorsto the server-expected structure centralizes logic and avoids accidental mismatches.
95-95: Storing selected bundles to UI settings
Updating UI settings withselectedBundlesensures subsequent wizard steps or features can access curated operator selections.
109-109: Formik initialization with new getOperatorsInitialValues
Directly supplying the derived initial form state helps keep the form in sync with cluster data and user preferences.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
2-5: Ensure imports are consistently used.
No major concerns here. Just verify thatPreflightHardwareRequirementsandOperatorSpecare correctly handled throughout the codebase, especially with potentially undefinedpreflightRequirements.
7-31:Details
✅ Verification successful
Check for unintended circular dependencies in recursion.
This recursive approach should work fine because of the!dependencies.includes(dep)guard. However, please confirm that there's no pathological scenario where a cycle in operator dependencies could slip through.
🏁 Script executed:
#!/bin/bash # Description: Searching for any potential cyclical references in operator dependency definitions # Assumption: Operator definitions are stored in code under "operators" references rg -A 5 "operators"Length of output: 65672
Circular Dependency Check Verified
After analyzing the recursion logic in
getOperatorDependenciesand reviewing how dependencies are added only once using the!dependencies.includes(dep)guard, there’s no evidence that a cyclic dependency would lead to infinite recursion—even in cases where an operator might depend (directly or indirectly) on itself. The grep search across operator-related files didn’t reveal any operator configuration that might inadvertently introduce a pathological cycle.• The guard ensures each dependency is processed only once.
• A hypothetical cycle (e.g., A → B, B → A) is safely truncated since a dependency isn’t added a second time.No further changes are required at this point.
libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (1)
84-84: Remove unused parameter from function call.The
openshiftVersionparameter has been removed from theOperatorsPopovercomponent signature but is still being passed when the component is used inOperatorsProgressItem. This parameter should be removed from the function call.- <OperatorsPopover operators={operators} openshiftVersion={openshiftVersion}> + <OperatorsPopover operators={operators}>Also applies to: 124-124
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
55-60:⚠️ Potential issueVersion comparison may lead to incorrect results.
The
parseFloatfunction will truncate trailing zeros, causing potential comparison issues. For example,parseFloat('4.10')yields4.1, causing'4.10' < '4.9'to be evaluated incorrectly.-export const isOCPVersionEqualsOrMore = ( - openshiftVersion: string, - ocpVersionToCompare: string, -): boolean => { - return parseFloat(getMajorMinorVersion(openshiftVersion)) >= parseFloat(ocpVersionToCompare); -}; +export const isOCPVersionEqualsOrMore = ( + openshiftVersion: string, + ocpVersionToCompare: string, +): boolean => { + const [majorA, minorA] = getMajorMinorVersion(openshiftVersion).split('.'); + const [majorB, minorB] = ocpVersionToCompare.split('.'); + return ( + Number(majorA) > Number(majorB) || + (Number(majorA) === Number(majorB) && Number(minorA) >= Number(minorB)) + ); +};libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
49-49:⚠️ Potential issuePotential runtime error when accessing operator spec properties.
The code doesn't check if
opSpecs[dep]exists before accessing itsnotStandaloneproperty, which could lead to runtime errors if a dependent operator doesn't have a corresponding spec.Apply this diff to add a safety check:
- .filter((dep) => opSpecs[dep].notStandalone) + .filter((dep) => opSpecs[dep]?.notStandalone)
🧹 Nitpick comments (8)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (2)
103-201: Extract complex logic to improve maintainability.The OperatorCheckbox component handles several responsibilities and contains complex logic, particularly for determining the disabled reason. Consider extracting some of this logic into separate helper functions.
For example, you could extract the disabled reason logic:
+const getDisabledReason = ( + isInBundle: boolean, + notStandalone: boolean | undefined, + parentOperatorName: string, + featureId: string, + getFeatureDisabledReason: (id: string) => string | undefined +) => { + if (isInBundle) { + return 'This operator is part of a bundle and cannot be deselected.'; + } + if (notStandalone) { + return 'This operator cannot be installed as a standalone'; + } + if (parentOperatorName) { + return `This operator is a dependency of ${parentOperatorName}`; + } + return getFeatureDisabledReason(featureId); +}; const OperatorCheckbox = ({ // ...props }) => { // ...existing code - const disabledReason = isInBundle - ? 'This operator is part of a bundle and cannot be deselected.' - : notStandalone - ? 'This operator cannot be installed as a standalone' - : parentOperatorName - ? `This operator is a dependency of ${parentOperatorName}` - : getFeatureDisabledReason(featureId); + const disabledReason = getDisabledReason( + isInBundle, + notStandalone, + parentOperatorName, + featureId, + getFeatureDisabledReason + ); // ...rest of the component };
42-45: Follow React component naming convention for props.The
Requirementsprop uses PascalCase, which typically indicates a React component. However, this is unusual for a prop name. Consider renaming it to camelCase (requirements) to follow common React prop naming conventions.const OperatorRequirements = ({ operatorId, preflightRequirements, - Requirements, + requirements: Requirements, cluster, }: { cluster: Cluster; operatorId: string; preflightRequirements: PreflightHardwareRequirements | undefined; - Requirements?: React.ComponentType<{ + requirements?: React.ComponentType<{ cluster: Cluster; }>; }) => { // ... }; const OperatorCheckbox = ({ // ...props notStandalone, Description, - Requirements, + requirements: Requirements, openshiftVersion, preflightRequirements, }: { // ... }Also applies to: 103-120
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (2)
40-55: Move function declaration outside useEffect.Defining the
fetchSupportedOperatorsfunction inside the useEffect hook can lead to unnecessary re-creation of the function. It's better to define it outside or use useCallback.+ const fetchSupportedOperators = React.useCallback(async () => { + try { + const fetchedOperators = await OperatorsService.getSupportedOperators(); + setSupportedOperators(fetchedOperators); + } catch (error) { + handleApiError(error, () => + addAlert({ title: 'Failed to fetch operators', message: getApiErrorMessage(error) }), + ); + } finally { + setIsLoading(false); + } + }, [addAlert]); React.useEffect(() => { - const fetchSupportedOperators = async () => { - try { - const fetchedOperators = await OperatorsService.getSupportedOperators(); - setSupportedOperators(fetchedOperators); - } catch (error) { - handleApiError(error, () => - addAlert({ title: 'Failed to fetch operators', message: getApiErrorMessage(error) }), - ); - } finally { - setIsLoading(false); - } - }; void fetchSupportedOperators(); - }, [addAlert]); + }, [fetchSupportedOperators]);
60-73: Simplify filtering logic for better readability.The filtering logic inside the useMemo hook is a bit complex. Consider extracting it into a separate function for better readability.
+ const filterOperatorsByFeature = ( + operatorKey: string, + isSingleClusterFeatureEnabled: boolean + ) => { + if (!isSingleClusterFeatureEnabled) { + return true; + } + return singleClusterOperators.includes(operatorKey); + }; const operators = React.useMemo(() => { return supportedOperators .sort((a, b) => { const aTitle = opSpecs[a]?.title || a; const bTitle = opSpecs[b]?.title || b; return aTitle.localeCompare(bTitle); }) - .filter((op) => { - if (!isSingleClusterFeatureEnabled) { - return true; - } - return singleClusterOperators.includes(op); - }); + .filter((op) => filterOperatorsByFeature(op, isSingleClusterFeatureEnabled)); }, [isSingleClusterFeatureEnabled, supportedOperators, opSpecs]);libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
110-110: Fix typo in Authorino description."Lightweigth" should be "Lightweight".
- Lightweigth external authorization service for tailor-made Zero Trust API security.{' '} + Lightweight external authorization service for tailor-made Zero Trust API security.{' '}libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (1)
30-30: Fix typo in error message."operator bunles" should be "operator bundles".
- title: 'Failed to fetch operator bunles', + title: 'Failed to fetch operator bundles',libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (1)
108-110: Consider handling potential undefined title more explicitly.While unlikely given the implementation of
getOperatorSpecs(), you might want to provide a more descriptive fallback for clarity.- const operatorLabel = opSpecs[OPERATOR_NAME_LVM]?.title; + const operatorLabel = opSpecs[OPERATOR_NAME_LVM]?.title || 'Logical Volume Manager';libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
69-100: Comprehensive bundle operators management.The
getNewBudleOperatorsfunction provides a robust mechanism for managing the addition and removal of operators based on bundle selection. The approach of determining which operators are still needed by other bundles prevents the unintended removal of shared dependencies.There is a typo in the function name
getNewBudleOperators(missing 'n' in 'Bundle').-export const getNewBudleOperators = ( +export const getNewBundleOperators = (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx(1 hunks)libs/ui-lib/lib/cim/components/helpers/versions.ts(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx(3 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(1 hunks)libs/ui-lib/lib/common/config/constants.ts(1 hunks)libs/ui-lib/lib/common/selectors/clusterSelectors.ts(1 hunks)libs/ui-lib/lib/common/types/clusters.ts(1 hunks)libs/ui-lib/lib/common/utils.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusters/utils.ts(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(7 hunks)libs/ui-lib/lib/ocm/components/utils.ts(0 hunks)libs/ui-lib/lib/ocm/services/OperatorsService.tsx(1 hunks)
💤 Files with no reviewable changes (35)
- libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusters/utils.ts
- libs/ui-lib/lib/ocm/components/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/ui-lib/lib/common/utils.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx
- libs/ui-lib/lib/common/selectors/clusterSelectors.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx
- libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/cim/components/helpers/versions.ts
🧰 Additional context used
🧬 Code Graph Analysis (10)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)
libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (2)
libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(70-353)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (3)
libs/ui-lib/lib/common/utils.ts (1)
getMajorMinorVersion(110-113)libs/ui-lib/lib/common/types/featureSupportLevel.ts (1)
FeatureId(8-15)libs/ui-lib/lib/common/config/constants.ts (22)
OPERATOR_NAME_MTV(302-302)OPERATOR_NAME_AMD_GPU(312-312)OPERATOR_NAME_LSO(298-298)OPERATOR_NAME_AUTHORINO(311-311)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_FENCE_AGENTS_REMEDIATION(316-316)OPERATOR_NAME_KMM(313-313)OPERATOR_NAME_KUBE_DESCHEDULER(318-318)OPERATOR_NAME_MCE(301-301)OPERATOR_NAME_NMSTATE(310-310)OPERATOR_NAME_NODE_FEATURE_DISCOVERY(303-303)OPERATOR_NAME_NODE_HEALTHCHECK(314-314)OPERATOR_NAME_NODE_MAINTENANCE(317-317)OPERATOR_NAME_NVIDIA_GPU(304-304)OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_OPENSHIFT_AI(308-308)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_PIPELINES(305-305)OPERATOR_NAME_SELF_NODE_REMEDIATION(315-315)OPERATOR_NAME_SERVERLESS(307-307)OPERATOR_NAME_SERVICEMESH(306-306)OPERATOR_NAME_LVM(300-300)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (7)
libs/types/assisted-installer-service.d.ts (2)
Bundle(89-109)PreflightHardwareRequirements(2419-2428)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
OperatorSpec(62-68)getOperatorSpecs(70-353)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx (1)
bundleSpecs(13-64)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/ocm/store/slices/current-cluster/selectors.ts (1)
selectIsCurrentClusterSNO(7-8)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
getNewBudleOperators(69-100)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (3)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
ClusterOperatorProps(72-75)libs/types/assisted-installer-service.d.ts (1)
Bundle(89-109)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)
libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx (5)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(70-353)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/TableSummaryExpandable.tsx (1)
TableSummaryExpandable(4-26)libs/ui-lib/lib/common/components/ui/table/utils.ts (1)
genericTableRowKey(15-15)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (7)
libs/types/assisted-installer-service.d.ts (3)
Cluster(110-398)PreflightHardwareRequirements(2419-2428)Bundle(89-109)libs/ui-lib/lib/ocm/store/slices/current-cluster/selectors.ts (2)
selectIsCurrentClusterSNO(7-8)selectCurrentClusterPermissionsState(9-17)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(62-68)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFieldId(15-18)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
getNewOperators(33-56)
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (4)
libs/ui-lib/lib/common/types/ui-settings.ts (1)
UISettingsValues(1-6)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
selectOlmOperators(40-44)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (4)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(70-353)libs/ui-lib/lib/common/config/constants.ts (5)
OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_LVM(300-300)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_OPENSHIFT_AI(308-308)libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
isSNO(59-60)libs/ui-lib/lib/common/types/cpuArchitecture.ts (2)
architectureData(52-72)SupportedCpuArchitecture(40-43)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
libs/types/assisted-installer-service.d.ts (2)
PreflightHardwareRequirements(2419-2428)Bundle(89-109)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(62-68)
🔇 Additional comments (34)
libs/ui-lib/lib/common/types/clusters.ts (1)
61-64: LGTM: Simplified operator values typeThe new
OperatorsValuestype provides a more flexible and dynamic approach to operator selection, using string arrays instead of multiple boolean flags. This change will make operator management more maintainable.libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
72-75: Enhanced interface provides more flexibilityProviding access to the entire
clusterobject rather than just specific properties gives components more flexibility to access cluster data as needed. This aligns well with the architectural changes to operator management in the codebase.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (3)
35-73: Well-implemented utility functionThe
getBundleLabelfunction effectively generates bundle labels with appropriate documentation links and operator details inside a popover, making the UI more informative.
75-172: Comprehensive bundle card implementation with proper edge case handlingThe
BundleCardcomponent effectively handles several edge cases:
- Unsupported operators detection (lines 91-93)
- Single Node OpenShift compatibility (lines 103-104)
- Bundle conflicts with informative tooltips (lines 97-108)
The selection logic correctly updates both bundles and operators (lines 111-125).
174-208: Good feature-aware bundle filteringThe
OperatorsBundlecomponent correctly filters bundles based on feature flags and provides a clean gallery layout for bundle selection.libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx (3)
9-31: Improved operator table implementationThe refactored row generation logic now properly filters operators based on the presence of specifications from
getOperatorSpecs. UsinguseMemofor this computation optimizes performance.
33-35: Good practice: early return for empty dataAdding an early return when there are no operators to display is a good practice that avoids rendering an empty table.
37-58: Enhanced table presentationUsing
TableSummaryExpandableimproves the user experience by providing a collapsible section for operators, which is especially useful when there are many items to display.libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (3)
19-22: Import changes look good.The changes to import
useNewFeatureSupportLevelinstead of the previous feature support level hook and adding thegetOperatorSpecsimport align well with the refactoring being done across the codebase to centralize operator specifications.
86-88: Feature support level implementation looks good.The usage of
useNewFeatureSupportLevelhook and memoization of operator specifications is a good practice that will help maintain consistency across the application.
100-100: Clean operator name resolution approach.Using the centralized operator specs to retrieve operator titles is cleaner than the previous approach and provides better maintainability and consistency.
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (3)
15-16: Updated imports look good.The transition to using
useNewFeatureSupportLeveland importinggetOperatorSpecsaligns well with the refactoring approach across the codebase.
69-73: Feature support level implementation looks good.The approach to determine LVM support and memoize operator specifications aligns with the pattern used elsewhere in the codebase.
100-100: Simplified operator name display.The direct mapping approach to display failed operator names is much cleaner than the previous implementation. It properly handles fallback to the operator name if no title is found in the specs.
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (1)
33-101: OperatorRequirements component is well implemented.The component effectively displays requirements for operators, with appropriate handling of different node types and requirement properties. The conditional rendering logic is comprehensive.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
79-107: Good implementation of the OperatorsSelect component.The component is well structured, with appropriate loading state handling and error handling for the API call. The expandable section with operator count and selection information provides a good user experience.
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
70-353: LGTM! Well-structured operator specifications.The implementation is clean, modular, and easy to maintain. The function returning a configuration object makes it straightforward to add or modify operator specifications in the future.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (4)
17-21: Great refactoring to improve component structure.The component has been simplified and now uses the new
useClusterPreflightRequirementshook for fetching data, which improves code organization and separation of concerns.
22-40: Improved error handling and state management.The use of
handleApiErrorwith alerts provides a better user experience when errors occur, and the effect cleanup is properly handled.
42-45: Good handling of loading state.Displaying a dedicated loading component while data is being fetched improves user experience.
46-62: Clean component structure with modular design.Using dedicated components like
OperatorsBundleandOperatorsSelectimproves code organization and reusability.libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (3)
49-52: Good refactoring to use centralized operator specifications.Using
getOperatorSpecs()to dynamically retrieve operator titles improves code maintainability by centralizing operator information.
77-80: Consistent approach to operator title retrieval.The implementation consistently uses
getOperatorSpecs()throughout the file to retrieve operator titles.
302-304: Consistent pattern for operator title retrieval.The implementation maintains consistency in how operator titles are retrieved and used throughout the file.
libs/ui-lib/lib/ocm/services/OperatorsService.tsx (1)
3-8: Good simplification of the service.The service has been appropriately simplified, removing functionality that's now handled elsewhere. The arrow function syntax for
getSupportedOperatorsis more consistent with modern JavaScript practices.libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (6)
24-32: Function refactored to use dynamic data instead of constants.The
getOperatorsInitialValuesfunction has been significantly improved to derive values from the cluster and UI settings rather than relying on hard-coded constants. This makes the code more maintainable and adaptable to different cluster configurations.
50-56: Async/await pattern correctly implemented.Making
handleNextasynchronous and awaitingsubmitForm()ensures that form submission completes before navigation, preventing potential race conditions.
67-67: Fixed potential promise handling issue.Using
() => void handleNext()properly handles the promise returned by the async function, preventing unintended side effects.
86-89: Simplified operator construction logic.The enabledOperators construction has been streamlined to map directly from selectedOperators, making the code more readable and maintainable.
95-95: Proper UI settings persistence.Using
updateUISettingsto persist the selected bundles ensures that user selections are maintained across the wizard workflow.
109-109: Updated initialization to use current context.The Formik initialValues now correctly uses the current uiSettings and cluster data, making the form more responsive to the actual application state.
libs/ui-lib/lib/common/config/constants.ts (1)
320-331: New singleClusterOperators constant enhances maintainability.The introduction of a consolidated
singleClusterOperatorsarray streamlines the operator management by providing a single source of truth for operators available in single cluster deployments. This is more maintainable than using multiple separate arrays.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
7-31: Well-implemented recursive dependency resolution.The
getOperatorDependenciesfunction effectively handles recursive dependency resolution, ensuring that all required operators are included. The tracking of whether new dependencies were added optimizes the recursion process.
58-67: Effective bundle operator resolution.The
getBundleOperatorsfunction efficiently collects all operators associated with a bundle, including their dependencies, promoting a more cohesive operator management system.
rawagner
force-pushed
the
abi_operators
branch
3 times, most recently
from
cd9c8cc to
3b33d01
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (4)
libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (1)
124-124:⚠️ Potential issueRemove unused parameter from function call
The
openshiftVersionparameter is still being passed to theOperatorsPopovercomponent on line 124, but this parameter was removed from the component's signature in line 84.- <OperatorsPopover operators={operators} openshiftVersion={openshiftVersion}> + <OperatorsPopover operators={operators}>libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
55-60:⚠️ Potential issuePotential version parsing issue
parseFloat('4.10')yields4.1, causing'4.10' < '4.9'. This may break comparisons involving double-digit minor versions.Apply this fix to correctly compare version numbers:
-export const isOCPVersionEqualsOrMore = ( - openshiftVersion: string, - ocpVersionToCompare: string, -): boolean => { - return parseFloat(getMajorMinorVersion(openshiftVersion)) >= parseFloat(ocpVersionToCompare); -}; +export const isOCPVersionEqualsOrMore = ( + openshiftVersion: string, + ocpVersionToCompare: string, +): boolean => { + const [majorA, minorA] = getMajorMinorVersion(openshiftVersion).split('.'); + const [majorB, minorB] = ocpVersionToCompare.split('.'); + return ( + Number(majorA) > Number(majorB) || + (Number(majorA) === Number(majorB) && Number(minorA) >= Number(minorB)) + ); +};
154-156:⚠️ Potential issueIncorrect documentation link for Kube Descheduler
The link for Kube Descheduler is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific link for Kube Descheduler.
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + {/* TODO: Replace with proper Kube Descheduler link when available */} + <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/scheduling/nodes-descheduler.html">Learn more</ExternalLink>libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
33-56:⚠️ Potential issueMissing null check for operator specs.
There's a potential runtime error when accessing
opSpecs[dep].notStandaloneifdepisn't defined inopSpecs. This issue was identified in a previous review but has not been addressed in this version.- .filter((dep) => opSpecs[dep].notStandalone) + .filter((dep) => opSpecs[dep]?.notStandalone)
🧹 Nitpick comments (9)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (2)
72-75: Type definition improved but contains redundancy.The refactoring of
ClusterOperatorPropsto include the entireClusterobject provides more flexibility and aligns with the changes in the wizard transition logic. However,clusterIdis now redundant since it can be accessed viacluster.id.Consider simplifying further:
export type ClusterOperatorProps = { - clusterId: Cluster['id'] - cluster: Cluster; + cluster: Cluster; }
72-75: Missing semicolons in type definition properties.The property declarations lack semicolons, which is inconsistent with TypeScript style conventions often used in the codebase.
Add semicolons for consistency:
export type ClusterOperatorProps = { - clusterId: Cluster['id'] - cluster: Cluster; + clusterId: Cluster['id']; + cluster: Cluster; }libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx (2)
33-35: Consider displaying an empty state message.
Returningnullhides the table entirely when no operators are present. A short "No operators to display" placeholder may improve user clarity.
38-57: Table structure is clean and readable.
All table elements are compact, with relevant props. Consider integrating i18n for the table label and column titles in future iterations if needed.libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
100-100: String concatenation using' and 'may need i18n consideration.
Hard-coding" and "might be untranslatable for non-English locales. Consider a more localized approach if multiple languages are supported.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (1)
22-40: Improved error handling for API requests.The error handling now properly uses
handleApiErroralong with user-friendly alerts, which is a good practice. However, there's a small typo in the alert title.- title: 'Failed to fetch operator bunles', + title: 'Failed to fetch operator bundles',libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (1)
107-110: Small inconsistency in variable naming.In this function, you're using
operatorLabelwhile other similar functions useoperatorTitle. Consider standardizing on one term for better consistency.- const operatorLabel = opSpecs[OPERATOR_NAME_LVM]?.title; + const operatorTitle = opSpecs[OPERATOR_NAME_LVM]?.title;Then update the references accordingly.
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (1)
67-67: Minor improvement: Consider async function reference.Instead of using an arrow function to call
handleNext, you can directly pass the function reference, as it's already designed to handle the event properly.- onNext={() => void handleNext()} + onNext={handleNext}libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
69-100: Function name has a typo: "Budle" instead of "Bundle".The function name
getNewBudleOperatorsshould begetNewBundleOperatorsto maintain consistent naming conventions and avoid confusion.- export const getNewBudleOperators = ( + export const getNewBundleOperators = (Otherwise, the implementation looks good and correctly manages the addition and removal of bundle operators while considering dependencies from other bundles.
🛑 Comments failed to post (1)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
231-232:
⚠️ Potential issueIncorrect documentation link for Node Maintenance
The Node Maintenance operator is using the NODE_HEALTHCHECK_LINK instead of a specific link for Node Maintenance.
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + {/* TODO: Replace with proper Node Maintenance link when available */} + <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/nodes/nodes-nodes-working.html#nodes-nodes-working-maintenance_nodes-nodes-working">Learn more</ExternalLink>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{/* TODO: Replace with proper Node Maintenance link when available */} <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/nodes/nodes-nodes-working.html#nodes-nodes-working-maintenance_nodes-nodes-working">Learn more</ExternalLink> </>
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (3)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
55-60:⚠️ Potential issueFix version comparison logic.
Using
parseFloatfor version comparison is problematic asparseFloat('4.10')yields4.1, causing incorrect comparisons like'4.10' < '4.9'.Apply this diff to fix the version comparison:
export const isOCPVersionEqualsOrMore = ( openshiftVersion: string, ocpVersionToCompare: string, ): boolean => { - return parseFloat(getMajorMinorVersion(openshiftVersion)) >= parseFloat(ocpVersionToCompare); + const [majorA, minorA] = getMajorMinorVersion(openshiftVersion).split('.'); + const [majorB, minorB] = ocpVersionToCompare.split('.'); + return ( + Number(majorA) > Number(majorB) || + (Number(majorA) === Number(majorB) && Number(minorA) >= Number(minorB)) + ); };
154-156:⚠️ Potential issueIncorrect documentation link for Kube Descheduler.
The link for Kube Descheduler is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific Kube Descheduler link.
<ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink>Replace with a proper Kube Descheduler link:
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/scheduling/nodes-descheduler.html">Learn more</ExternalLink>libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
49-49:⚠️ Potential issueAdd null check to prevent runtime errors
As noted in a previous review, accessing
opSpecs[dep].notStandalonedirectly may cause runtime errors ifdepis not defined inopSpecs. Add optional chaining to prevent this issue.- .filter((dep) => opSpecs[dep].notStandalone) + .filter((dep) => opSpecs[dep]?.notStandalone)
🧹 Nitpick comments (11)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (1)
91-109: Consider simplifying the complex conditional logic.The disabled reason determination uses nested ternary operators that span multiple lines, which can be hard to read and maintain.
Consider extracting this logic into a separate function for better readability:
- const disabledReason = hasUnsupportedOperators - ? 'Some operators in this bundle are not supported with the current configuration.' - : isSNO && bundleSpec?.noSNO - ? 'This bundle is not available when deploying a Single Node OpenShift.' - : incompatibleBundle - ? `Bundle cannot be installed together with ${ - bundles.find(({ id }) => id === incompatibleBundle)?.title || incompatibleBundle - }` - : undefined; + const getDisabledReason = () => { + if (hasUnsupportedOperators) { + return 'Some operators in this bundle are not supported with the current configuration.'; + } + if (isSNO && bundleSpec?.noSNO) { + return 'This bundle is not available when deploying a Single Node OpenShift.'; + } + if (incompatibleBundle) { + const incompatibleBundleTitle = bundles.find(({ id }) => id === incompatibleBundle)?.title || incompatibleBundle; + return `Bundle cannot be installed together with ${incompatibleBundleTitle}`; + } + return undefined; + }; + + const disabledReason = getDisabledReason();libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (1)
28-33: Fix typo in error message.There's a spelling error in the alert title.
- title: 'Failed to fetch operator bunles', + title: 'Failed to fetch operator bundles',libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (3)
40-55: Consider adding error boundary to handle API failures.While the component correctly handles API errors by showing an alert, it's missing an error state in the UI. If the API call fails, the component will remain in loading state indefinitely.
React.useEffect(() => { const fetchSupportedOperators = async () => { try { const fetchedOperators = await OperatorsService.getSupportedOperators(); setSupportedOperators(fetchedOperators); } catch (error) { handleApiError(error, () => addAlert({ title: 'Failed to fetch operators', message: getApiErrorMessage(error) }), ); + setSupportedOperators([]); // Set empty array to allow component to render } finally { setIsLoading(false); } }; void fetchSupportedOperators(); }, [addAlert]);
75-77: Add fallback UI for empty operators list.If no operators are available (either due to API error or filtering), the component doesn't provide feedback to the user.
if (isLoading) { return <LoadingState />; } + + if (operators.length === 0) { + return <div data-testid="no-operators-message">No operators available</div>; + }
79-106: Ensure accessibility of the expandable section.The expandable section is properly implemented with appropriate data-testid attributes, but consider adding aria-label for better accessibility.
<ExpandableSection toggleText={`Single Operators (${operators.length} | ${values.selectedOperators.length} selected)`} onToggle={() => setIsExpanded(!isExpanded)} isExpanded={isExpanded} data-testid="single-operators-section" + aria-label="Selectable operators" >libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
70-353: Consider breaking down the largegetOperatorSpecsfunction for better maintainability.The function returns a large object with numerous operator specifications, which could make maintenance challenging. Consider organizing operators into categories or separating them into smaller functions.
Example approach:
// Split by functionality or domain const getStorageOperatorSpecs = (useLVMS?: boolean) => ({ [OPERATOR_NAME_LSO]: { /* LSO specs */ }, [OPERATOR_NAME_ODF]: { /* ODF specs */ }, [OPERATOR_NAME_LVM]: { /* LVM specs */ }, }); const getVirtualizationOperatorSpecs = () => ({ [OPERATOR_NAME_CNV]: { /* CNV specs */ }, [OPERATOR_NAME_MTV]: { /* MTV specs */ }, // ...other virtualization operators }); export const getOperatorSpecs = (useLVMS?: boolean) => { return { ...getStorageOperatorSpecs(useLVMS), ...getVirtualizationOperatorSpecs(), // ...other operator categories }; };libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (1)
242-243: Lowercase "Lso" in error message should be capitalized.For consistency with other error messages and proper naming, "Lso" should be capitalized as "LSO" or written out as "Local Storage Operator".
if (!isSupported) { - return `Lso is not available with the selected CPU architecture.`; + return `Local Storage Operator is not available with the selected CPU architecture.`; }libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (1)
50-56: Consider adding error handling to async functionThe
handleNextfunction has been made asynchronous, which is good for waiting for form submission to complete. However, there's no error handling in place ifsubmitForm()fails.const handleNext = async () => { - await submitForm(); + try { + await submitForm(); + } catch (error) { + // Handle form submission errors + console.error('Form submission failed:', error); + return; + } if (window.location.pathname.indexOf('assisted-installer') > -1) { navigate(pathname, { state: undefined, replace: true }); } clusterWizardContext.moveNext(); };libs/ui-lib/lib/common/config/constants.ts (1)
320-331: Add documentation for new constantThe
singleClusterOperatorsarray is introduced without any explanatory comments. Consider adding documentation to explain its purpose and usage in the application.+/** + * List of operators that are available for single cluster deployments. + * These operators can be managed through the cluster wizard. + */ export const singleClusterOperators = [ OPERATOR_NAME_CNV, OPERATOR_NAME_NODE_HEALTHCHECK, OPERATOR_NAME_FENCE_AGENTS_REMEDIATION, OPERATOR_NAME_NODE_MAINTENANCE, OPERATOR_NAME_MTV, OPERATOR_NAME_KUBE_DESCHEDULER, OPERATOR_NAME_NMSTATE, OPERATOR_NAME_SELF_NODE_REMEDIATION, OPERATOR_NAME_ODF, OPERATOR_NAME_LSO ];libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
69-69: Fix typo in function nameThe function name
getNewBudleOperatorshas a typo and should begetNewBundleOperators.-export const getNewBudleOperators = ( +export const getNewBundleOperators = ( currentOperators: string[], currentBundles: string[], allBundles: Bundle[],
79-81: Consider consolidating similar logicThe logic for adding operators in
getNewBudleOperatorsis similar to that ingetNewOperators. Consider extracting this logic to a separate helper function to avoid duplication.
🛑 Comments failed to post (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
7-31: 🛠️ Refactor suggestion
Add safeguard against infinite recursion
The recursive
getOperatorDependenciesfunction could potentially cause infinite recursion if there are circular dependencies between operators. Consider adding a mechanism to detect and prevent circular dependencies.const getOperatorDependencies = ( operatorId: string, preflightRequirements: PreflightHardwareRequirements | undefined, dependencies: string[] = [], + processedOperators: string[] = [], ) => { + // Prevent infinite recursion by tracking processed operators + if (processedOperators.includes(operatorId)) { + return dependencies; + } + processedOperators.push(operatorId); + const deps = preflightRequirements?.operators?.find(({ operatorName }) => operatorName === operatorId) ?.dependencies || []; let newDepAdded = false; deps.forEach((dep) => { if (!dependencies.includes(dep)) { dependencies.push(dep); newDepAdded = true; } }); if (newDepAdded) { // add deps of new deps deps.forEach((op) => { - getOperatorDependencies(op, preflightRequirements, dependencies); + getOperatorDependencies(op, preflightRequirements, dependencies, processedOperators); }); } return dependencies; };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const getOperatorDependencies = ( operatorId: string, preflightRequirements: PreflightHardwareRequirements | undefined, dependencies: string[] = [], processedOperators: string[] = [] ) => { // Prevent infinite recursion by tracking processed operators if (processedOperators.includes(operatorId)) { return dependencies; } processedOperators.push(operatorId); const deps = preflightRequirements?.operators?.find(({ operatorName }) => operatorName === operatorId) ?.dependencies || []; let newDepAdded = false; deps.forEach((dep) => { if (!dependencies.includes(dep)) { dependencies.push(dep); newDepAdded = true; } }); if (newDepAdded) { // add deps of new deps deps.forEach((op) => { getOperatorDependencies(op, preflightRequirements, dependencies, processedOperators); }); } return dependencies; };
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (3)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
55-60:⚠️ Potential issuePotential version parsing issue.
parseFloat('4.10')yields4.1, causing'4.10' < '4.9'. This may break comparisons involving double-digit minor versions.Below is a suggested fix that splits and compares versions as numbers:
-export const isOCPVersionEqualsOrMore = ( - openshiftVersion: string, - ocpVersionToCompare: string, -): boolean => { - return parseFloat(getMajorMinorVersion(openshiftVersion)) >= parseFloat(ocpVersionToCompare); -}; +export const isOCPVersionEqualsOrMore = ( + openshiftVersion: string, + ocpVersionToCompare: string, +): boolean => { + const [majorA, minorA] = getMajorMinorVersion(openshiftVersion).split('.'); + const [majorB, minorB] = ocpVersionToCompare.split('.'); + return ( + Number(majorA) > Number(majorB) || + (Number(majorA) === Number(majorB) && Number(minorA) >= Number(minorB)) + ); +};
154-156:⚠️ Potential issueIncorrect documentation link for Kube Descheduler.
The link for Kube Descheduler is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific link for Kube Descheduler.
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + {/* TODO: Replace with proper Kube Descheduler link when available */} + <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/scheduling/nodes-descheduler.html">Learn more</ExternalLink>libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
33-56:⚠️ Potential issueAdd potential null check to prevent runtime errors.
The code accesses
opSpecs[dep].notStandalonedirectly, which could cause runtime errors if an operator spec is missing. Consider adding a null check to improve robustness.Apply this diff to prevent potential runtime errors:
- .filter((dep) => opSpecs[dep].notStandalone) + .filter((dep) => opSpecs[dep]?.notStandalone)
🧹 Nitpick comments (11)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (3)
86-89: Duplicate hook call can be optimizedThe useNewFeatureSupportLevel hook is called twice unnecessarily.
- const featureSupportLevelData = useNewFeatureSupportLevel(); const { getFeatureSupportLevel } = useNewFeatureSupportLevel(); + const { getFeatureSupportLevel, isFeatureSupported } = useNewFeatureSupportLevel(); const useLVMS = getFeatureSupportLevel('LVM') === 'supported'; const opSpecs = React.useMemo(() => getOperatorSpecs(useLVMS), [useLVMS]); const hasUnsupportedOperators = !!bundle.operators?.some( - (operator) => !featureSupportLevelData.isFeatureSupported(opSpecs[operator].featureId), + (operator) => !isFeatureSupported(opSpecs[operator].featureId), );
91-93: Consider memoizing the unsupported operators checkThe hasUnsupportedOperators calculation could benefit from memoization since it depends on bundle.operators which shouldn't change frequently.
- const hasUnsupportedOperators = !!bundle.operators?.some( - (operator) => !featureSupportLevelData.isFeatureSupported(opSpecs[operator].featureId), - ); + const hasUnsupportedOperators = React.useMemo( + () => !!bundle.operators?.some( + (operator) => !featureSupportLevelData.isFeatureSupported(opSpecs[operator].featureId) + ), + [bundle.operators, featureSupportLevelData, opSpecs] + );
174-208: Add documentation for the isSingleClusterFeatureEnabled filteringThe logic that filters bundles to only show 'virtualization' when isSingleClusterFeatureEnabled is true lacks context. Consider adding a comment explaining this business logic.
return ( <Stack hasGutter> <StackItem> <Title headingLevel="h2" size="lg"> Bundles </Title> </StackItem> <StackItem> <Gallery hasGutter minWidths={{ default: '350px' }}> + {/* When in single cluster mode, only show virtualization bundle */} {(isSingleClusterFeatureEnabled ? bundles.filter((b) => b.id === 'virtualization') : bundles ).map((bundle) => (libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
100-104: Operator display logic is simplified but consider improving multi-operator listing.
Concatenating operator names with" and "works for two operators, but may read awkwardly (e.g., “A and B and C”). Consider joining them with commas or using a localized approach if you expect more than two failures.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (2)
33-101: Good job displaying additional requirements, but consider adding i18n.
TheOperatorRequirementscomponent clearly communicates memory/CPU requirements. However, these user-facing messages are hardcoded strings, potentially limiting localization. If future i18n is desired, consider an approach that leverages translation hooks.
103-201: Well-structured checkbox component with thorough dependency checks.
TheOperatorCheckboxlogic correctly handles bundled operators, dependencies, and viewer-mode restrictions. It also updates the form state viasetFieldValuein a straightforward manner.
• Consider adding i18n for user-facing disabled messages.
• The approach for generating new operators withgetNewOperatorsis neat, but ensure all orphans/dependencies are handled in edge cases.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (1)
30-30: Fix typo in error message.There's a typo in the error message: "bunles" should be "bundles".
- title: 'Failed to fetch operator bunles', + title: 'Failed to fetch operator bundles',libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
110-110: Fix typo in Authorino description.There's a typo in the description: "Lightweigth" should be "Lightweight".
- Lightweigth external authorization service for tailor-made Zero Trust API security.{' '} + Lightweight external authorization service for tailor-made Zero Trust API security.{' '}libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (2)
108-110: Use consistent variable naming.The variable
operatorLabelis used here, but similar variables in other functions are namedoperatorTitle(lines 51, 79, 129, 303). Consider using consistent naming for improved code readability.- const operatorLabel = opSpecs[OPERATOR_NAME_LVM]?.title; + const operatorTitle = opSpecs[OPERATOR_NAME_LVM]?.title;
242-243: Inconsistent capitalization in error message.The error message uses "Lso" while the operator name should be capitalized as "LSO" (Local Storage Operator) for consistency with other error messages.
- return `Lso is not available with the selected CPU architecture.`; + return `Local Storage Operator is not available with the selected CPU architecture.`;libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
69-100: Comprehensive bundle operators management.The
getNewBudleOperatorsfunction correctly handles both adding and removing bundle operators:
- When adding (lines 79-81): It combines current operators with all operators from the new bundle
- When removing (lines 83-99):
- It identifies which bundles remain selected
- Determines which operators are still needed by those bundles
- Removes only operators that aren't needed by any remaining bundle
This prevents accidentally removing operators that might be needed by other selected bundles.
Fix typo in function name.
There's a typo in the function name:
getNewBudleOperatorsshould begetNewBundleOperators.-export const getNewBudleOperators = ( +export const getNewBundleOperators = (
🛑 Comments failed to post (1)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (1)
21-40:
⚠️ Potential issueMissing cluster.id in useEffect dependencies.
The effect uses
cluster.idvia theuseClusterPreflightRequirementshook, but it's not included in the dependency array. This could cause the effect not to re-run if the cluster id changes.- }, [addAlert]); + }, [addAlert, cluster.id]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const { preflightRequirements } = useClusterPreflightRequirements(cluster.id); React.useEffect(() => { const fetchBundles = async () => { try { const fetchedBundles = await BundleService.listBundles(); setBundles(fetchedBundles); } catch (error) { handleApiError(error, () => addAlert({ title: 'Failed to fetch operator bunles', message: getApiErrorMessage(error), }), ); } finally { setIsLoading(false); } }; void fetchBundles(); }, [addAlert, cluster.id]);
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
55-60:⚠️ Potential issueFix version comparison logic to handle double-digit minor versions.
The current implementation has a critical flaw when comparing version numbers. Using
parseFloatto compare versions strips trailing zeros, causing incorrect comparisons - for example,parseFloat('4.10')yields4.1causing'4.10' < '4.9'which is incorrect.-export const isOCPVersionEqualsOrMore = ( - openshiftVersion: string, - ocpVersionToCompare: string, -): boolean => { - return parseFloat(getMajorMinorVersion(openshiftVersion)) >= parseFloat(ocpVersionToCompare); -}; +export const isOCPVersionEqualsOrMore = ( + openshiftVersion: string, + ocpVersionToCompare: string, +): boolean => { + const [majorA, minorA] = getMajorMinorVersion(openshiftVersion).split('.'); + const [majorB, minorB] = ocpVersionToCompare.split('.'); + return ( + Number(majorA) > Number(majorB) || + (Number(majorA) === Number(majorB) && Number(minorA) >= Number(minorB)) + ); +};
149-156:⚠️ Potential issueIncorrect documentation link for Kube Descheduler.
The link for Kube Descheduler is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific link for Kube Descheduler.
<ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink>Replace with a proper Kube Descheduler link:
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/scheduling/nodes-descheduler.html">Learn more</ExternalLink>libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
33-56:⚠️ Potential issueAdd safety check for operator specs access
This code needs a safety check when accessing operator specs to prevent potential runtime errors.
// uncheck unneeded not-standalone dependencies too const notStandaloneDeps = dependencies - .filter((dep) => opSpecs[dep].notStandalone) + .filter((dep) => opSpecs[dep]?.notStandalone) .filter((dep) => // some other operator may still depend on the not-standalone operator newOperators.every((op) => !getOperatorDependencies(op, preflightRequirements).includes(dep)), );
🧹 Nitpick comments (13)
libs/ui-lib/lib/common/config/constants.ts (1)
320-331: Consider adding a descriptive comment for the new constant.The newly added
singleClusterOperatorsarray includes a subset of operators, but lacks documentation explaining its purpose and usage. Adding a descriptive comment above this constant would improve code maintainability and help other developers understand why these specific operators were selected.+/** + * List of operators available for single cluster configuration. + * These operators can be enabled during cluster creation. + */ export const singleClusterOperators = [ OPERATOR_NAME_CNV, OPERATOR_NAME_NODE_HEALTHCHECK, OPERATOR_NAME_FENCE_AGENTS_REMEDIATION, OPERATOR_NAME_NODE_MAINTENANCE, OPERATOR_NAME_MTV, OPERATOR_NAME_KUBE_DESCHEDULER, OPERATOR_NAME_NMSTATE, OPERATOR_NAME_SELF_NODE_REMEDIATION, OPERATOR_NAME_ODF, OPERATOR_NAME_LSO, ];libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
70-353: Centralize operator specifications for better code organization and maintenance.The creation of this central
getOperatorSpecsfunction is a significant architectural improvement. It provides a single source of truth for operator details, making the codebase more maintainable.Each operator is well-documented with a clear title, description, requirements (when applicable), and helpful links to documentation. The
notStandaloneflag provides valuable information about whether operators can function independently.Consider adding tests to verify that all operators' specifications are correctly returned, especially since this function will be used across multiple components.
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (1)
49-52: Refactor to use centralized operator specifications for consistency.Great improvement to replace hardcoded operator labels with dynamic titles from the centralized
getOperatorSpecsfunction. This creates a single source of truth for operator information.Consider optimizing the code by calling
getOperatorSpecs()once at the module level and reusing the result, rather than calling it multiple times in different functions:+const opSpecs = getOperatorSpecs(); const getOdfDisabledReason = ( cluster: Cluster | undefined, activeFeatureConfiguration: ActiveFeatureConfiguration | undefined, isSupported: boolean, ) => { if (!cluster) { return undefined; } - const opSpecs = getOperatorSpecs(); const operatorTitle = opSpecs[OPERATOR_NAME_ODF]?.title || ''; // ... };This would slightly improve performance, especially if this utility is used frequently.
Also applies to: 78-80, 108-109, 128-129, 302-303
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (2)
116-125: Fix typo in function name referenceThere appears to be a typo in the function name
getNewBudleOperators(should begetNewBundleOperators). Ensure this function exists with this exact spelling in the imported modules.Consider renaming the function in its source file or updating this reference to match the correct spelling.
191-204: Consider adding pagination for large bundle collectionsThe current implementation renders all bundles in a gallery layout, which might cause performance issues or a cluttered UI if there are many bundles available in the future.
Consider implementing pagination or virtual scrolling for the bundle gallery if you anticipate a large number of bundles.
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (3)
75-93: Improve error messages for unsupported configurationsThe worker and master requirements display could be enhanced with more descriptive error messages for the case when no requirements are available.
Consider providing more context about why requirements might not be available, especially for operators that are typically expected to have requirements.
130-146: Extract complex logic into named functionsThe code contains complex logic for determining if an operator is in a bundle or has parent dependencies. These checks could be extracted into named functions to improve readability and maintainability.
+ // Check if the operator is included in any selected bundle + const isOperatorInSelectedBundle = (operatorId: string, selectedBundles: string[], bundles: Bundle[]) => { + return selectedBundles.some( + (sb) => !!bundles.find((b) => b.id === sb)?.operators?.includes(operatorId) + ); + }; + // Find parent operator that has the current operator as a dependency + const getParentOperatorName = ( + operatorId: string, + selectedOperators: string[], + preflightRequirements: PreflightHardwareRequirements | undefined, + opSpecs: { [key: string]: OperatorSpec } + ) => { + const parentOperator = preflightRequirements?.operators?.find((op) => + op.dependencies?.includes(operatorId) + ); + + if (parentOperator?.operatorName && selectedOperators.includes(parentOperator.operatorName)) { + return opSpecs[parentOperator.operatorName]?.title || parentOperator.operatorName; + } + + return ''; + }; // In the component: - const isInBundle = values.selectedBundles.some( - (sb) => !!bundles.find((b) => b.id === sb)?.operators?.includes(operatorId), - ); + const isInBundle = isOperatorInSelectedBundle(operatorId, values.selectedBundles, bundles); - const parentOperator = preflightRequirements?.operators?.find((op) => - op.dependencies?.includes(operatorId), - ); - - let parentOperatorName = ''; - if ( - parentOperator?.operatorName && - values.selectedOperators.includes(parentOperator.operatorName) - ) { - parentOperatorName = opSpecs[parentOperator.operatorName]?.title || parentOperator.operatorName; - } + const parentOperatorName = getParentOperatorName( + operatorId, + values.selectedOperators, + preflightRequirements, + opSpecs + );
176-187: Memoize complex operations to prevent unnecessary re-rendersThe onChange handler for the checkbox calls
getNewOperatorsdirectly, which might trigger unnecessary re-renders if the function reference changes. Consider memoizing this operation.+ const updateSelectedOperators = React.useCallback( + (checked: boolean) => { + setFieldValue( + 'selectedOperators', + getNewOperators( + values.selectedOperators, + operatorId, + preflightRequirements, + checked, + opSpecs, + ), + ); + }, + [values.selectedOperators, operatorId, preflightRequirements, setFieldValue, opSpecs] + ); // In the onChange handler: - onChange={(_, checked) => { - setFieldValue( - 'selectedOperators', - getNewOperators( - values.selectedOperators, - operatorId, - preflightRequirements, - checked, - opSpecs, - ), - ); - }} + onChange={(_, checked) => updateSelectedOperators(checked)}libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (1)
28-34: Fix typo in error messageThere's a typo in the error alert title: "operator bunles" should be "operator bundles".
handleApiError(error, () => addAlert({ - title: 'Failed to fetch operator bunles', + title: 'Failed to fetch operator bundles', message: getApiErrorMessage(error), }), );libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
40-55: Consider adding error handling for network failuresThe effect properly fetches supported operators, but could be enhanced to handle network failures more gracefully.
React.useEffect(() => { const fetchSupportedOperators = async () => { try { const fetchedOperators = await OperatorsService.getSupportedOperators(); setSupportedOperators(fetchedOperators); } catch (error) { handleApiError(error, () => addAlert({ title: 'Failed to fetch operators', message: getApiErrorMessage(error) }), ); + // Set supportedOperators to empty array to avoid undefined errors + setSupportedOperators([]); } finally { setIsLoading(false); } }; void fetchSupportedOperators(); }, [addAlert]);libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (1)
50-56: Consider adding error handling to the async handleNext functionThe function now correctly awaits form submission before navigating, but lacks error handling.
const handleNext = async () => { - await submitForm(); + try { + await submitForm(); + } catch (error) { + // Log error or show notification + console.error('Form submission failed:', error); + return; // Prevent navigation if form submission fails + } if (window.location.pathname.indexOf('assisted-installer') > -1) { navigate(pathname, { state: undefined, replace: true }); } clusterWizardContext.moveNext(); };libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
7-31: Well-implemented recursive dependency resolutionThe
getOperatorDependenciesfunction effectively traverses operator dependencies, but consider adding protection against circular dependencies.const getOperatorDependencies = ( operatorId: string, preflightRequirements: PreflightHardwareRequirements | undefined, dependencies: string[] = [], + visited: Set<string> = new Set(), ) => { + // Prevent circular dependencies + if (visited.has(operatorId)) { + return dependencies; + } + visited.add(operatorId); const deps = preflightRequirements?.operators?.find(({ operatorName }) => operatorName === operatorId) ?.dependencies || []; let newDepAdded = false; deps.forEach((dep) => { if (!dependencies.includes(dep)) { dependencies.push(dep); newDepAdded = true; } }); if (newDepAdded) { // add deps of new deps deps.forEach((op) => { - getOperatorDependencies(op, preflightRequirements, dependencies); + getOperatorDependencies(op, preflightRequirements, dependencies, visited); }); } return dependencies; };
69-100: Fix typo in function namegetNewBudleOperatorsThe function name has a typo - it should be
getNewBundleOperatorsfor better readability and consistency.-export const getNewBudleOperators = ( +export const getNewBundleOperators = ( currentOperators: string[], currentBundles: string[], allBundles: Bundle[], newBundle: Bundle, preflightRequirements: PreflightHardwareRequirements | undefined, add: boolean, ): string[] => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
libs/locales/lib/en/translation.json(0 hunks)libs/ui-lib-tests/cypress/support/variables/host-discovery.ts(1 hunks)libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts(0 hunks)libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx(1 hunks)libs/ui-lib/lib/cim/components/helpers/versions.ts(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx(3 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(1 hunks)libs/ui-lib/lib/common/config/constants.ts(1 hunks)libs/ui-lib/lib/common/selectors/clusterSelectors.ts(1 hunks)libs/ui-lib/lib/common/types/clusters.ts(1 hunks)libs/ui-lib/lib/common/utils.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusters/utils.ts(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(7 hunks)libs/ui-lib/lib/ocm/components/utils.ts(0 hunks)libs/ui-lib/lib/ocm/services/OperatorsService.tsx(1 hunks)
💤 Files with no reviewable changes (37)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx
- libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts
- libs/ui-lib/lib/ocm/components/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusters/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx
- libs/locales/lib/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (11)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css
- libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/common/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx
- libs/ui-lib/lib/common/selectors/clusterSelectors.ts
- libs/ui-lib/lib/cim/components/helpers/versions.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx
- libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx
🧰 Additional context used
🧬 Code Graph Analysis (8)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (3)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
ClusterOperatorProps(72-75)libs/types/assisted-installer-service.d.ts (1)
Bundle(89-109)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (6)
libs/ui-lib/lib/ocm/store/slices/current-cluster/selectors.ts (2)
selectIsCurrentClusterSNO(7-8)selectCurrentClusterPermissionsState(9-17)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(62-68)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFieldId(15-18)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
getNewOperators(33-56)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (5)
libs/ui-lib/lib/common/utils.ts (1)
getMajorMinorVersion(110-113)libs/ui-lib/lib/common/types/featureSupportLevel.ts (1)
FeatureId(8-15)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/config/constants.ts (22)
OPERATOR_NAME_MTV(302-302)OPERATOR_NAME_AMD_GPU(312-312)OPERATOR_NAME_LSO(298-298)OPERATOR_NAME_AUTHORINO(311-311)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_FENCE_AGENTS_REMEDIATION(316-316)OPERATOR_NAME_KMM(313-313)OPERATOR_NAME_KUBE_DESCHEDULER(318-318)OPERATOR_NAME_MCE(301-301)OPERATOR_NAME_NMSTATE(310-310)OPERATOR_NAME_NODE_FEATURE_DISCOVERY(303-303)OPERATOR_NAME_NODE_HEALTHCHECK(314-314)OPERATOR_NAME_NODE_MAINTENANCE(317-317)OPERATOR_NAME_NVIDIA_GPU(304-304)OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_OPENSHIFT_AI(308-308)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_PIPELINES(305-305)OPERATOR_NAME_SELF_NODE_REMEDIATION(315-315)OPERATOR_NAME_SERVERLESS(307-307)OPERATOR_NAME_SERVICEMESH(306-306)OPERATOR_NAME_LVM(300-300)libs/ui-lib/lib/common/config/docs_links.ts (22)
MTV_LINK(140-141)getLsoLink(155-158)AUTHORINO_OPERATOR_LINK(153-153)CNV_LINK(91-91)FENCE_AGENTS_REMEDIATION_LINK(114-115)KMM_LINK(93-94)NODE_HEALTHCHECK_LINK(108-109)getMceDocsLink(98-101)getNmstateLink(160-163)getNodeFeatureDiscoveryLink(165-168)getNvidiaGpuLink(170-173)ODF_REQUIREMENTS_LINK(82-83)ODF_LINK(96-96)OPENSHIFT_AI_REQUIREMENTS_LINK(85-86)OPENSHIFT_AI_LINK(143-144)OSC_REQUIREMENTS_LINK(88-89)OSC_LINK(146-146)PIPELINES_OPERATOR_LINK(175-176)SELF_NODE_REMEDIATION_LINK(111-112)SERVERLESS_OPERATOR_LINK(183-184)getServiceMeshLink(178-181)getLvmsDocsLink(103-106)
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (5)
libs/ui-lib/lib/common/types/ui-settings.ts (1)
UISettingsValues(1-6)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
selectOlmOperators(40-44)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsx (1)
useClusterWizardContext(25-31)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (7)
libs/types/assisted-installer-service.d.ts (2)
Bundle(89-109)PreflightHardwareRequirements(2419-2428)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
OperatorSpec(62-68)getOperatorSpecs(70-353)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx (1)
bundleSpecs(13-64)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/ocm/store/slices/current-cluster/selectors.ts (1)
selectIsCurrentClusterSNO(7-8)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
getNewBudleOperators(69-100)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (2)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(70-353)libs/ui-lib/lib/common/config/constants.ts (5)
OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_LVM(300-300)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_OPENSHIFT_AI(308-308)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
libs/types/assisted-installer-service.d.ts (2)
PreflightHardwareRequirements(2419-2428)Bundle(89-109)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(62-68)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (17)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
72-75:Details
❓ Verification inconclusive
Refactored
ClusterOperatorPropsto use the complete Cluster objectThe change improves the type definition by using the entire
Clusterobject instead of selectively picking properties. This provides better access to all cluster data and simplifies the type definition.Ensure all components consuming this type are updated to access properties through the
clusterobject (e.g.,props.cluster.openshiftVersioninstead ofprops.openshiftVersionandprops.cluster.monitoredOperatorsinstead ofprops.monitoredOperators).
🏁 Script executed:
#!/bin/bash # Find all components that use ClusterOperatorProps to verify they've been updated rg -A 5 -B 5 "ClusterOperatorProps" --type typescript # Find usages of the old property access patterns that might need updating rg "openshiftVersion\s*[:=]" --type typescript | grep -v "cluster\.openshiftVersion" rg "monitoredOperators\s*[:=]" --type typescript | grep -v "cluster\.monitoredOperators"Length of output: 336
Verify Cluster Operator Props Refactor and Component Updates
The refactor inlibs/ui-lib/lib/common/components/clusterWizard/types.ts(lines 72-75) now uses the completeClusterobject, streamlining access to cluster properties. Please ensure that all consuming components have been updated accordingly—access properties likeopenshiftVersionandmonitoredOperatorsthrough theclusterobject (e.g., useprops.cluster.openshiftVersioninstead ofprops.openshiftVersion).Note: The initial script using
--type typescriptproduced errors. The updated script above searches the entire repository without file type filtering. Kindly run or manually verify these new commands to ensure no outdated access patterns remain.
🏁 Script executed:
#!/bin/bash # Searching for instances of "ClusterOperatorProps" in the codebase rg -A 5 -B 5 "ClusterOperatorProps" . # Searching for old property access patterns for openshiftVersion not prefixed with "cluster." rg "openshiftVersion\s*[:=]" . | grep -v "cluster\.openshiftVersion" # Searching for old property access patterns for monitoredOperators not prefixed with "cluster." rg "monitoredOperators\s*[:=]" . | grep -v "cluster\.monitoredOperators"Length of output: 11089
Refactored
ClusterOperatorPropsto Leverage the Complete Cluster Object
The change inlibs/ui-lib/lib/common/components/clusterWizard/types.tsnow passes the entireClusterobject via theclusterproperty (while retainingclusterIdfor convenience). Consumers ofClusterOperatorPropsare expected to access all cluster data through this object (for example, useprops.cluster.openshiftVersionandprops.cluster.monitoredOperatorsrather than accessing these properties directly).
- In particular, files like
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsxshow that the refactor is being applied by destructuringclusterfrom the props.- Although a repository-wide search for old access patterns returns several instances of
openshiftVersionandmonitoredOperators, these appear in varied contexts. Please manually verify that no component consumingClusterOperatorPropsstill relies on the outdated pattern.libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (1)
7-11: Import operator name constants and centralized operator specs retrieval.Good refactoring to import operator name constants and the
getOperatorSpecsfunction for consistent operator specifications across the application.Also applies to: 20-20
libs/ui-lib-tests/cypress/support/variables/host-discovery.ts (1)
40-43:Details
✅ Verification successful
Update test selectors to match UI component changes.
The selector changes from checkbox fields to standard input fields reflect the UI component changes in the operator selection interface. This ensures the Cypress tests will continue to work with the updated UI.
Verify that all tests using these selectors have been updated to accommodate the new input type:
🏁 Script executed:
#!/bin/bash # Find tests that might use the old selectors rg "useContainerNativeVirtualizationField|useOpenShiftDataFoundation" --glob "*.{ts,tsx}" libs/ui-lib-tests/cypress/Length of output: 596
Test selectors verification passed – All instances use the updated input field selectors
The search results confirm that all test files referencing
Cypress.env('useContainerNativeVirtualizationField')andCypress.env('useOpenShiftDataFoundation')now point to the new selectors defined inlibs/ui-lib-tests/cypress/support/variables/host-discovery.ts. Specifically:
- Host Discovery Variables: Updated to use
'#form-input-cnv-field'foruseContainerNativeVirtualizationFieldand'#form-input-odf-field'foruseOpenShiftDataFoundation.- Bare Metal Discovery Tests: Continue to reference these environment variables and thereby use the updated selectors.
No outdated selector usage was detected in the codebase. Please verify that the UI changes are correctly implemented and that these selectors align with the updated design.
libs/ui-lib/lib/common/types/clusters.ts (1)
61-64:Details
❓ Verification inconclusive
Simplify operators value model with a selection-based approach.
The redesign of the
OperatorsValuestype is a significant improvement that:
- Simplifies the data model from many boolean flags to selection arrays
- Introduces flexibility for adding/removing operators without type changes
- Supports the concept of operator bundles alongside individual operators
This is a breaking change that will require updates to any code using the old type, but the benefits of the new design justify the change.
Verify the impact on existing code:
🏁 Script executed:
#!/bin/bash # Find places that might need updates due to OperatorsValues type change rg "OperatorsValues" --glob "*.{ts,tsx}" libs/ui-lib/Length of output: 1338
OperatorsValues Model Update – Verify Affected Usages
The updated
OperatorsValuestype now uses selection arrays instead of multiple boolean flags, which streamlines the data model and provides more flexibility for managing operators and bundles. This breaking change affects existing code that consumes this type. Our search found references in these files:
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsxPlease verify that all impacted components have been updated to correctly handle the new selection-based structure (e.g., ensuring any logic expecting boolean values has been revised). Overall, the redesign is a positive improvement—just ensure that all usage contexts are thoroughly reviewed to prevent regressions.
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
69-73: Clean implementation of operator spec retrievalThe addition of feature support level checking and operator specs retrieval is well implemented with proper React hooks usage. Good use of
useMemoto optimize performance.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (1)
33-101: Well-structured component for displaying operator requirementsThe
OperatorRequirementscomponent effectively displays hardware requirements for operators, with good conditional rendering and fallback values. The component properly handles cases when requirements are not available.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (3)
17-21: Clean implementation of component setupThe component now has a cleaner structure with proper destructuring of props and simplified state management. Good use of hooks for managing state and accessing preflight requirements.
42-44: Good user experience enhancement with loading stateThe addition of a loading state provides a better user experience while waiting for the bundles to be fetched. This is a good improvement.
51-60: Improved component compositionThe refactoring to use dedicated components for bundles and operator selection improves the separation of concerns and makes the code more maintainable. This is a positive architectural change.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (3)
32-38: Looks good - appropriate state initialization and hooks usageThe component correctly initializes state variables for loading status, expandable section, and supported operators list. The use of React hooks follows best practices.
60-73: Well-implemented operator sorting and filtering logicThe memoized
operatorsvalue efficiently sorts and filters the operators, avoiding unnecessary re-renders. The logic correctly handles theisSingleClusterFeatureEnabledfeature flag.
79-106: Good UI implementation with expandable sectionThe expandable section provides a clean interface for selecting operators, with a clear count of available and selected operators. The component correctly handles operator specifications and renders null for invalid specs.
libs/ui-lib/lib/ocm/services/OperatorsService.tsx (1)
4-7: Function refactored to arrow syntax - LGTMThe refactoring of
getSupportedOperatorsto arrow function syntax is a stylistic improvement that maintains the same functionality.libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (4)
24-31: Improved initial values function signatureThe function now accepts
uiSettingsandclusterparameters, allowing for more dynamic initialization. The function correctly retrieves bundle selections from UI settings and operator selections from the cluster's monitored operators.
43-48: Logic for determining if Next button should be disabled is comprehensiveThe condition checks all relevant states: auto-save running, form validity, alerts, submission status, and custom logic from
canNextOperators. This ensures users can only proceed when appropriate.
83-95: Simplified operator handling in submit functionThe handleSubmit function now creates operator objects directly from the selected operator IDs, which is cleaner and more maintainable than the previous approach.
108-110: Formik initialization refactored for better maintainabilityThe Formik component now directly calls
getOperatorsInitialValueswith the proper parameters, improving code readability and maintainability.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx
Outdated
Show resolved
Hide resolved
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (3)
55-60:⚠️ Potential issueFix version comparison logic.
The current version comparison has a critical flaw that could cause incorrect behavior. Using
parseFloatfor version comparisons is problematic because it converts'4.10'to4.1, resulting in incorrect comparisons where'4.10'would incorrectly evaluate as less than'4.9'.Apply this fix to properly compare version components:
export const isOCPVersionEqualsOrMore = ( openshiftVersion: string, ocpVersionToCompare: string, ): boolean => { - return parseFloat(getMajorMinorVersion(openshiftVersion)) >= parseFloat(ocpVersionToCompare); + const [majorA, minorA] = getMajorMinorVersion(openshiftVersion).split('.'); + const [majorB, minorB] = ocpVersionToCompare.split('.'); + return ( + Number(majorA) > Number(majorB) || + (Number(majorA) === Number(majorB) && Number(minorA) >= Number(minorB)) + ); };
154-156:⚠️ Potential issueIncorrect documentation link for Kube Descheduler.
The Kube Descheduler is incorrectly using the Node Healthcheck documentation link instead of a specific link for Kube Descheduler.
<> Evicts pods to reschedule them onto more suitable nodes.{' '} - <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/scheduling/nodes-descheduler.html">Learn more</ExternalLink> </>
225-232:⚠️ Potential issueIncorrect documentation link for Node Maintenance.
Node Maintenance is incorrectly using the Node Healthcheck documentation link instead of a specific link for Node Maintenance.
<> Place nodes in maintenance mode.{' '} - <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + <ExternalLink href="https://docs.redhat.com/en/documentation/workload_availability_for_red_hat_openshift/latest/html/remediation_fencing_and_maintenance/node-maintenance-operator">Learn more</ExternalLink> </>libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (2)
35-73:⚠️ Potential issueAdd defensive handling for missing bundle specs.
The
getBundleLabelfunction doesn't check ifbundleSpecs[bundle.id || '']exists before accessing its properties, which could lead to runtime errors.const getBundleLabel = (bundle: Bundle, opSpecs: { [key: string]: OperatorSpec }) => { const bundleSpec = bundleSpecs[bundle.id || '']; + // Handle case when bundleSpec is undefined + const hasSpec = !!bundleSpec; return ( <> <span>{bundle.title} </span> <PopoverIcon component={'a'} headerContent={<div>Requirements and dependencies</div>} bodyContent={ <Stack hasGutter> - {bundleSpec.Description && ( + {hasSpec && bundleSpec.Description && ( <StackItem> <bundleSpec.Description /> </StackItem> )} {bundle.operators?.length && ( <> <StackItem>Bundle operators:</StackItem> <StackItem> <List> {bundle.operators.map((op) => ( <ListItem key={op}>{opSpecs[op]?.title || op}</ListItem> ))} </List> </StackItem> </> )} - {bundleSpec.docsLink && ( + {hasSpec && bundleSpec.docsLink && ( <StackItem> <ExternalLink href={bundleSpec.docsLink}>Learn more</ExternalLink> </StackItem> )} </Stack> } /> </> ); };
91-93:⚠️ Potential issueAdd null check for operator feature IDs.
The current implementation directly accesses
opSpecs[operator].featureIdwithout checking ifopSpecs[operator]exists, which could cause runtime errors if an operator isn't found in the specs.const hasUnsupportedOperators = !!bundle.operators?.some( - (operator) => !featureSupportLevelData.isFeatureSupported(opSpecs[operator].featureId), + (operator) => { + const operatorSpec = opSpecs[operator]; + return operatorSpec ? !featureSupportLevelData.isFeatureSupported(operatorSpec.featureId) : false; + }, );libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
100-100: 🛠️ Refactor suggestionAdd error handling for missing operator specs.
The current implementation directly maps operator names to titles without checking if the operator exists in the specs. If any operator name is undefined or not found in the specs, this could lead to potential issues.
-{failedOperators.map(({ name }) => opSpecs[name || '']?.title || name).join(' and ')}{' '} +{failedOperators + .map(({ name }) => { + const operatorName = name || ''; + return opSpecs[operatorName]?.title || operatorName; + }) + .join(' and ')}{' '}libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
33-56: Add safety check for undefined operator specsThe filter in line 49 could throw an error if
opSpecs[dep]is undefined. This was previously flagged in a review.// uncheck unneeded not-standalone dependencies too const notStandaloneDeps = dependencies - .filter((dep) => opSpecs[dep].notStandalone) + .filter((dep) => opSpecs[dep]?.notStandalone) .filter((dep) => // some other operator may still depend on the not-standalone operator newOperators.every((op) => !getOperatorDependencies(op, preflightRequirements).includes(dep)), );
🧹 Nitpick comments (6)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (2)
108-109: Inconsistent naming and missing error handling.Unlike other similar functions, this function uses
operatorLabelinstead ofoperatorTitleand doesn't include a fallback to an empty string when the title might be missing.- const operatorLabel = opSpecs[OPERATOR_NAME_LVM]?.title; + const operatorTitle = opSpecs[OPERATOR_NAME_LVM]?.title || ''; - if (platformType === 'nutanix') { - return `${operatorLabel} is not supported when Nutanix platform type is selected.`; - } - if (!isSupported) { - return `${operatorLabel} is not supported in this OpenShift version.`; + if (platformType === 'nutanix') { + return `${operatorTitle} is not supported when Nutanix platform type is selected.`; + } + if (!isSupported) { + return `${operatorTitle} is not supported in this OpenShift version.`;Also applies to: 111-111, 114-114
40-67: Performance optimization opportunity.Each operator-related function makes a separate call to
getOperatorSpecs(), which could be inefficient if this function is expensive. Consider retrieving the operator specs once at a higher level and passing them to these functions.-const getOdfDisabledReason = ( +const getOdfDisabledReason = ( cluster: Cluster | undefined, activeFeatureConfiguration: ActiveFeatureConfiguration | undefined, isSupported: boolean, + opSpecs?: { [key: string]: OperatorSpec } ) => { if (!cluster) { return undefined; } - const opSpecs = getOperatorSpecs(); + const operatorSpecs = opSpecs || getOperatorSpecs(); - const operatorTitle = opSpecs[OPERATOR_NAME_ODF]?.title || ''; + const operatorTitle = operatorSpecs[OPERATOR_NAME_ODF]?.title || ''; // Rest of the function remains the same }; // Similar changes to other operator-related functions // Then in getNewFeatureDisabledReason: export const getNewFeatureDisabledReason = ( // existing parameters ): string | undefined => { + const opSpecs = getOperatorSpecs(); switch (featureId) { // Pass opSpecs to each function call case 'ODF': { - return getOdfDisabledReason(cluster, activeFeatureConfiguration, isSupported); + return getOdfDisabledReason(cluster, activeFeatureConfiguration, isSupported, opSpecs); } // Similarly for other operator-related cases } };Also applies to: 69-97, 99-117, 119-134, 293-315
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (1)
22-40: Typo in error message.There's a spelling error in the error alert title.
- title: 'Failed to fetch operator bunles', + title: 'Failed to fetch operator bundles',libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (3)
7-31: Consider adding safeguards against circular dependenciesThe recursive implementation of
getOperatorDependencieseffectively collects all transitive dependencies, but lacks protection against circular dependencies which could cause infinite recursion.Consider adding a cycle detection mechanism:
const getOperatorDependencies = ( operatorId: string, preflightRequirements: PreflightHardwareRequirements | undefined, dependencies: string[] = [], + processedOperators: Set<string> = new Set(), ) => { + if (processedOperators.has(operatorId)) { + return dependencies; + } + processedOperators.add(operatorId); + const deps = preflightRequirements?.operators?.find(({ operatorName }) => operatorName === operatorId) ?.dependencies || []; let newDepAdded = false; deps.forEach((dep) => { if (!dependencies.includes(dep)) { dependencies.push(dep); newDepAdded = true; } }); if (newDepAdded) { // add deps of new deps deps.forEach((op) => { - getOperatorDependencies(op, preflightRequirements, dependencies); + getOperatorDependencies(op, preflightRequirements, dependencies, processedOperators); }); } return dependencies; };
58-67: Consider deduplicating operators in getBundleOperatorsThe function produces a potentially duplicated list of operators as it pushes each operator and its dependencies without checking for duplicates.
const getBundleOperators = ( bundle: Bundle, preflightRequirements: PreflightHardwareRequirements | undefined, ) => { - const bundleOperators = [...(bundle.operators || [])]; - bundle.operators?.forEach((op) => { - bundleOperators.push(...getOperatorDependencies(op, preflightRequirements)); - }); - return bundleOperators; + const bundleOperators = new Set(bundle.operators || []); + bundle.operators?.forEach((op) => { + getOperatorDependencies(op, preflightRequirements).forEach(dep => bundleOperators.add(dep)); + }); + return Array.from(bundleOperators); };
69-100: Fix typo in function name and add null checksThere's a typo in the function name
getNewBudleOperators(missing 'n' in 'Bundle'). Also, consider adding null checks for parameters.-export const getNewBudleOperators = ( +export const getNewBundleOperators = ( currentOperators: string[], currentBundles: string[], allBundles: Bundle[], newBundle: Bundle, preflightRequirements: PreflightHardwareRequirements | undefined, add: boolean, ): string[] => { + if (!newBundle || !allBundles?.length) { + return [...currentOperators]; + } + const newBundleOperators = getBundleOperators(newBundle, preflightRequirements); if (add) { return [...new Set([...currentOperators, ...newBundleOperators])]; } const newBundles = currentBundles.filter((b) => b !== newBundle.id); const operatorsToKeep = newBundles.reduce((acc, bundleId) => { const bundle = allBundles.find(({ id }) => id === bundleId); if (bundle) { const bundleOperators = getBundleOperators(bundle, preflightRequirements); bundleOperators.forEach((op) => { if (!acc.includes(op)) { acc.push(op); } }); } return acc; }, [] as string[]); const operatorsToRemove = newBundleOperators.filter((op) => !operatorsToKeep.includes(op)); return currentOperators.filter((op) => !operatorsToRemove.includes(op)); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
libs/locales/lib/en/translation.json(0 hunks)libs/ui-lib-tests/cypress/support/variables/host-discovery.ts(1 hunks)libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts(0 hunks)libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx(1 hunks)libs/ui-lib/lib/cim/components/helpers/versions.ts(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx(3 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(1 hunks)libs/ui-lib/lib/common/config/constants.ts(1 hunks)libs/ui-lib/lib/common/selectors/clusterSelectors.ts(1 hunks)libs/ui-lib/lib/common/types/clusters.ts(1 hunks)libs/ui-lib/lib/common/utils.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusters/utils.ts(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(7 hunks)libs/ui-lib/lib/ocm/components/utils.ts(0 hunks)libs/ui-lib/lib/ocm/services/OperatorsService.tsx(1 hunks)
💤 Files with no reviewable changes (37)
- libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts
- libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx
- libs/ui-lib/lib/ocm/components/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusters/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx
- libs/locales/lib/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (14)
- libs/ui-lib/lib/common/selectors/clusterSelectors.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx
- libs/ui-lib-tests/cypress/support/variables/host-discovery.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css
- libs/ui-lib/lib/common/utils.ts
- libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx
- libs/ui-lib/lib/cim/components/helpers/versions.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx
- libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx
- libs/ui-lib/lib/common/components/clusterWizard/types.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx
🧰 Additional context used
🧬 Code Graph Analysis (6)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (3)
libs/ui-lib/lib/common/utils.ts (1)
getMajorMinorVersion(110-113)libs/ui-lib/lib/common/types/featureSupportLevel.ts (1)
FeatureId(8-15)libs/ui-lib/lib/common/config/constants.ts (22)
OPERATOR_NAME_MTV(302-302)OPERATOR_NAME_AMD_GPU(312-312)OPERATOR_NAME_LSO(298-298)OPERATOR_NAME_AUTHORINO(311-311)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_FENCE_AGENTS_REMEDIATION(316-316)OPERATOR_NAME_KMM(313-313)OPERATOR_NAME_KUBE_DESCHEDULER(318-318)OPERATOR_NAME_MCE(301-301)OPERATOR_NAME_NMSTATE(310-310)OPERATOR_NAME_NODE_FEATURE_DISCOVERY(303-303)OPERATOR_NAME_NODE_HEALTHCHECK(314-314)OPERATOR_NAME_NODE_MAINTENANCE(317-317)OPERATOR_NAME_NVIDIA_GPU(304-304)OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_OPENSHIFT_AI(308-308)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_PIPELINES(305-305)OPERATOR_NAME_SELF_NODE_REMEDIATION(315-315)OPERATOR_NAME_SERVERLESS(307-307)OPERATOR_NAME_SERVICEMESH(306-306)OPERATOR_NAME_LVM(300-300)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (3)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
ClusterOperatorProps(72-75)libs/types/assisted-installer-service.d.ts (1)
Bundle(89-109)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (7)
libs/types/assisted-installer-service.d.ts (2)
Bundle(89-109)PreflightHardwareRequirements(2419-2428)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
OperatorSpec(62-68)getOperatorSpecs(70-353)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx (1)
bundleSpecs(13-64)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/ocm/store/slices/current-cluster/selectors.ts (1)
selectIsCurrentClusterSNO(7-8)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
getNewBudleOperators(69-100)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (4)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(70-353)libs/ui-lib/lib/common/config/constants.ts (5)
OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_LVM(300-300)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_OPENSHIFT_AI(308-308)libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
isSNO(59-60)libs/ui-lib/lib/common/types/cpuArchitecture.ts (2)
architectureData(52-72)SupportedCpuArchitecture(40-43)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
libs/types/assisted-installer-service.d.ts (2)
PreflightHardwareRequirements(2419-2428)Bundle(89-109)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(62-68)
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (4)
libs/ui-lib/lib/common/types/ui-settings.ts (1)
UISettingsValues(1-6)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
selectOlmOperators(40-44)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: unit-tests
- GitHub Check: format
- GitHub Check: tests
- GitHub Check: circular-deps
- GitHub Check: lint
🔇 Additional comments (25)
libs/ui-lib/lib/common/config/constants.ts (1)
320-331: Introduction ofsingleClusterOperatorsarray simplifies operator managementThe introduction of the
singleClusterOperatorsarray consolidates operators that should be available in a single cluster environment. This is a good step toward better organization of operator-related constants, aligning with the PR title's goal of integrating operators into the ABI.According to the AI summary, this replaces the previously removed
OperatorNamesandExposedOperatorNamesarrays, which should simplify maintenance and reduce duplication.libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (5)
7-11: Imports refactored to support operator specs centralization.The imports have been updated to include operator name constants and the
getOperatorSpecsfunction, supporting the transition to a more centralized approach for managing operator specifications.Also applies to: 20-20
49-51: Dynamic operator titles improve maintainability.The function now retrieves operator titles dynamically from a central source instead of using hardcoded values, which improves maintainability and consistency.
Also applies to: 55-55, 58-58, 61-61, 64-64
78-79: Good error handling with fallback to empty string.The code appropriately handles potential missing operator titles by using the optional chaining operator (?.) and providing a fallback to an empty string, which is a good defensive programming practice.
Also applies to: 81-81, 89-93
128-131: Consistent pattern for operator title retrieval.The function follows the established pattern for retrieving and using operator titles dynamically.
302-306: Consistent implementation for OpenShift AI operator.The implementation follows the same pattern as other operator-related functions, maintaining code consistency.
Also applies to: 309-309, 312-312
libs/ui-lib/lib/common/types/clusters.ts (1)
61-64: LGTM: Simplified OperatorsValues type.The
OperatorsValuestype has been simplified to use string arrays for selected bundles and operators, which provides a cleaner and more maintainable approach.libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
72-74: LGTM: Added effective operator specs handling.Good addition of the
useLVMSflag and the memoizedopSpecsobject which will improve performance by preventing unnecessary recalculations.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (4)
1-16: Imports are now more focused and clear.The imports have been streamlined to remove unused ones and include only what's necessary for the component. The addition of error handling utilities and new operator-related components reflects the refactored architecture.
17-21: Component signature is simplified using destructuring.Good change to directly destructure the
clusterprop fromClusterOperatorProps, making it clear what's being used from the props.
42-44: Good addition of loading state.Adding a dedicated loading state enhances user experience by providing clear feedback during data fetching.
46-62: Component structure is cleaner with modular components.The render logic now delegates specific functionality to specialized components (
OperatorsBundleandOperatorsSelect), making the code more maintainable.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (4)
1-39: Well-structured component with proper typing.The component has clear prop definitions with appropriate TypeScript types. State management is well-organized with separate state variables for different concerns.
40-55: Robust error handling for API calls.The implementation properly handles API errors with user-friendly alerts and maintains loading state correctly.
57-73: Efficient operators list processing.Good use of
useMemoto optimize performance when filtering and sorting operators. The filtering logic correctly respects the single cluster feature flag.
75-107: Clear UI presentation with expandable section.The expandable section with operator count information provides a good user experience. The conditional rendering ensures no invalid operators are displayed.
libs/ui-lib/lib/ocm/services/OperatorsService.tsx (1)
3-8: Service function signature standardized.The
getSupportedOperatorsmethod has been refactored to use arrow function syntax, which aligns with modern JavaScript practices and likely matches the style used elsewhere in the codebase.libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (7)
24-32: Initial values function updated to use modern pattern.The
getOperatorsInitialValuesfunction now properly leverages UI settings and cluster data to initialize the form values, improving the integration with the application state.
38-40: Improved type safety with useFormikContext.The type parameter for
useFormikContextensures type safety when working with form values.
50-58: Enhanced form submission with async/await pattern.The
handleNextfunction is now properly async and awaits form submission before navigation, preventing potential race conditions.
69-69: Clean event handling with arrow function.Using an arrow function for the
onNextevent ensures proper binding and makes it clear thathandleNextis being called.
85-91: Simplified operator mapping.The construction of enabled operators has been simplified to directly map from the selected operators array, making the code more maintainable.
97-97: UI settings are properly persisted.Good addition of updating UI settings with the selected bundles to maintain state across sessions.
111-111: Form initialization is more concise.The Formik initialization now directly uses the refined
getOperatorsInitialValuesfunction with the correct parameters.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
1-5: LGTM! Good job organizing the imports.The imports are properly organized, importing types from external packages first, followed by internal imports.
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (12)
libs/ui-lib/lib/common/utils.ts (1)
110-113: Improve the version parsing regex pattern.The regex pattern
/[0-9].[0-9][0-9]?/ghas several issues:
- The dot character (
.) is not escaped, so it will match any character, not just a literal period- The global flag (
g) is unnecessary when usingexec()only once- The pattern lacks capture groups which would make the code more maintainable
Here's a more precise implementation:
export const getMajorMinorVersion = (version = '') => { - const match = /[0-9].[0-9][0-9]?/g.exec(version); + const match = /(\d+)\.(\d+)/i.exec(version); return match?.[0] || ''; };This improved regex:
- Properly escapes the dot to match only periods
- Uses
\d+to match one or more digits (more flexible than the current pattern)- Adds capture groups for better clarity
- Replaces the global flag with case-insensitivity flag
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
96-96: Simplified operator name display logicThe implementation now directly maps operator names to their titles using the centralized specs object. While this is cleaner than the previous approach, there's an opportunity to improve the error handling for missing operator specs.
Add error handling for missing operator specs
The current implementation directly maps operator names to titles without checking if the operator exists in the specs. If any operator name is undefined or not found in the specs, this could lead to potential issues.
-{failedOperators.map(({ name }) => opSpecs[name || '']?.title || name).join(' and ')}{' '} +{failedOperators + .map(({ name }) => { + const operatorName = name || ''; + return opSpecs[operatorName]?.title || operatorName; + }) + .join(' and ')}{' '}libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (3)
53-62: Add null safety checks in getBundleOperatorsThe function doesn't check if
bundleorbundle.operatorsis undefined before accessing them, which could lead to runtime errors.const getBundleOperators = ( bundle: Bundle, preflightRequirements: PreflightHardwareRequirements | undefined, ) => { + if (!bundle || !bundle.operators) { + return []; + } + const bundleOperators = new Set(bundle.operators || []); bundle.operators?.forEach((op) => { getOperatorDependencies(op, preflightRequirements).forEach((dep) => bundleOperators.add(dep)); }); return [...bundleOperators]; };
64-72: Add parameter validation in getNewBundleOperators functionThe function doesn't validate its input parameters. If
newBundleis undefined orallBundlesis empty, the function could encounter runtime errors.export const getNewBundleOperators = ( currentOperators: string[], currentBundles: string[], allBundles: Bundle[], newBundle: Bundle, preflightRequirements: PreflightHardwareRequirements | undefined, add: boolean, ): string[] => { + if (!newBundle || !newBundle.id) { + console.warn('Invalid bundle provided to getNewBundleOperators'); + return [...currentOperators]; + } + const newBundleOperators = getBundleOperators(newBundle, preflightRequirements);
7-26: 🛠️ Refactor suggestionProtect against circular dependencies in recursive function
The
getOperatorDependenciesfunction recursively fetches operator dependencies but lacks protection against circular dependencies, which could lead to infinite recursion and stack overflow errors in production.Add a Set parameter to track processed operators:
const getOperatorDependencies = ( operatorId: string, preflightRequirements: PreflightHardwareRequirements | undefined, dependencies: Set<string> = new Set(), + processed: Set<string> = new Set(), ) => { + if (processed.has(operatorId)) { + return [...dependencies]; + } + processed.add(operatorId); + const originalSize = dependencies.size; const deps = preflightRequirements?.operators?.find(({ operatorName }) => operatorName === operatorId) ?.dependencies || []; deps.forEach((dep) => dependencies.add(dep)); if (originalSize !== dependencies.size) { // add deps of new deps deps.forEach((op) => { - getOperatorDependencies(op, preflightRequirements, dependencies); + getOperatorDependencies(op, preflightRequirements, dependencies, processed); }); } return [...dependencies]; };libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
58-71: Add null check for operator names during sortingThe sorting function doesn't check if
aorbare valid strings before accessing them, which could lead to errors if the API returns invalid data.const operators = React.useMemo(() => { return supportedOperators .filter(Boolean) // Add this line to filter out null/undefined values .sort((a, b) => { - const aTitle = opSpecs[a]?.title || a; - const bTitle = opSpecs[b]?.title || b; + const aTitle = opSpecs[a]?.title || (a || ''); + const bTitle = opSpecs[b]?.title || (b || ''); return aTitle.localeCompare(bTitle); }) .filter((op) => { if (!isSingleClusterFeatureEnabled) { return true; } return singleClusterOperators.includes(op); }); }, [isSingleClusterFeatureEnabled, supportedOperators, opSpecs]);libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (2)
52-52: 🛠️ Refactor suggestionConsider adding null check for preflightRequirements.
The preflightRequirements object is passed directly to OperatorsBundle component without checking if it's null.
- <OperatorsBundle bundles={bundles} preflightRequirements={preflightRequirements} /> + <OperatorsBundle bundles={bundles} preflightRequirements={preflightRequirements || undefined} />
55-59: 🛠️ Refactor suggestionConsider adding null check for preflightRequirements in OperatorsSelect.
Similarly, preflightRequirements is passed to the OperatorsSelect component without checking if it's null.
<OperatorsSelect bundles={bundles} cluster={cluster} - preflightRequirements={preflightRequirements} + preflightRequirements={preflightRequirements || undefined} />libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (2)
81-86: 🛠️ Refactor suggestionAdd safeguard for empty operator names.
The code assumes all entries in selectedOperators are valid non-empty strings. Consider adding a filter to ensure only valid operator names are used.
- const enabledOperators = values.selectedOperators.map((so) => ({ - name: so, - })); + const enabledOperators = values.selectedOperators + .filter((so) => so.trim() !== '') // Filter out empty strings + .map((so) => ({ + name: so, + }));
88-95: 🛠️ Refactor suggestionAdd error handling for updateUISettings.
There's error handling for the cluster update API call, but not for the updateUISettings call. If the first succeeds but the second fails, it could leave the application in an inconsistent state.
try { const { data: updatedCluster } = await ClustersService.update(cluster.id, cluster.tags, { olmOperators: enabledOperators, }); - await updateUISettings({ bundlesSelected: values.selectedBundles }); + try { + await updateUISettings({ bundlesSelected: values.selectedBundles }); + } catch (uiError) { + addAlert({ + title: 'Failed to update UI settings', + message: 'Your operator selections were saved, but bundle preferences could not be updated.' + }); + } dispatch(updateCluster(updatedCluster));libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
154-161: 🛠️ Refactor suggestionIncorrect documentation link for Kube Descheduler.
The link for Kube Descheduler is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific link for Kube Descheduler.
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + {/* TODO: Replace with proper Kube Descheduler link when available */} + <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/scheduling/nodes-descheduler.html">Learn more</ExternalLink>
210-217: 🛠️ Refactor suggestionIncorrect documentation link for Node Maintenance.
Node Maintenance is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific link for Node Maintenance.
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + <ExternalLink href="https://docs.redhat.com/en/documentation/workload_availability_for_red_hat_openshift/latest/html/remediation_fencing_and_maintenance/node-maintenance-operator">Learn more</ExternalLink>
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (2)
85-101: Add loading state for zero operatorsThe component correctly handles the loading state, but doesn't account for when
operatorsis an empty array after filtering. Consider showing a message when no operators are available.Add a check for empty operators list:
<Stack hasGutter data-testid={'operators-form'}> + {operators.length === 0 && ( + <StackItem>No operators are available for selection.</StackItem> + )} {operators.map((operatorKey) => { if (!opSpecs[operatorKey]) { return null; } return ( <StackItem key={operatorKey}> <OperatorCheckbox bundles={bundles} operatorId={operatorKey} cluster={cluster} openshiftVersion={cluster.openshiftVersion} preflightRequirements={preflightRequirements} {...opSpecs[operatorKey]} /> </StackItem> ); })} </Stack>
78-83: Improve accessibility for expandable sectionThe expandable section lacks an
aria-labelattribute, which would improve screen reader accessibility by providing more context about the expandable content.Add an aria-label attribute:
<ExpandableSection toggleText={`Single Operators (${operators.length} | ${values.selectedOperators.length} selected)`} onToggle={() => setIsExpanded(!isExpanded)} isExpanded={isExpanded} data-testid="single-operators-section" + aria-label="Single operators selection section" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (69)
libs/locales/lib/en/translation.json(0 hunks)libs/ui-lib-tests/cypress/integration/use-cases/create-cluster/with-mtv-operator.cy.ts(3 hunks)libs/ui-lib-tests/cypress/support/interceptors.ts(3 hunks)libs/ui-lib-tests/cypress/support/variables/host-discovery.ts(1 hunks)libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts(0 hunks)libs/ui-lib-tests/cypress/views/forms/Operators/OperatorsForm.ts(4 hunks)libs/ui-lib-tests/cypress/views/operatorsPage.ts(1 hunks)libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx(1 hunks)libs/ui-lib/lib/cim/components/helpers/versions.ts(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/ClusterProgressItems.tsx(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx(4 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(1 hunks)libs/ui-lib/lib/common/config/constants.ts(1 hunks)libs/ui-lib/lib/common/selectors/clusterSelectors.ts(1 hunks)libs/ui-lib/lib/common/types/clusters.ts(1 hunks)libs/ui-lib/lib/common/utils.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterProgressItems.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusters/utils.ts(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(7 hunks)libs/ui-lib/lib/ocm/components/utils.ts(0 hunks)libs/ui-lib/lib/ocm/services/OperatorsService.tsx(1 hunks)
💤 Files with no reviewable changes (37)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusters/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx
- libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx
- libs/ui-lib/lib/ocm/components/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx
- libs/locales/lib/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (20)
- libs/ui-lib-tests/cypress/views/operatorsPage.ts
- libs/ui-lib/lib/common/components/clusterDetail/ClusterProgressItems.tsx
- libs/ui-lib/lib/common/selectors/clusterSelectors.ts
- libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterProgressItems.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx
- libs/ui-lib-tests/cypress/support/variables/host-discovery.ts
- libs/ui-lib/lib/cim/components/helpers/versions.ts
- libs/ui-lib/lib/common/components/clusterWizard/types.ts
- libs/ui-lib-tests/cypress/integration/use-cases/create-cluster/with-mtv-operator.cy.ts
- libs/ui-lib-tests/cypress/views/forms/Operators/OperatorsForm.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx
- libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx
- libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts
- libs/ui-lib/lib/common/config/constants.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx
🧰 Additional context used
🧠 Learnings (1)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
Learnt from: rawagner
PR: openshift-assisted/assisted-installer-ui#2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.813Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
🧬 Code Graph Analysis (6)
libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts (1)
libs/ui-lib/lib/common/types/featureSupportLevel.ts (1)
FeatureId(8-15)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (5)
libs/types/assisted-installer-service.d.ts (3)
Cluster(110-398)Bundle(89-109)PreflightHardwareRequirements(2419-2428)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)libs/ui-lib/lib/common/config/constants.ts (1)
singleClusterOperators(320-331)
libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx (4)
libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/TableSummaryExpandable.tsx (1)
TableSummaryExpandable(4-26)libs/ui-lib/lib/common/components/ui/table/utils.ts (1)
genericTableRowKey(15-15)
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
libs/types/assisted-installer-service.d.ts (2)
PreflightHardwareRequirements(2419-2428)Bundle(89-109)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(68-74)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (6)
libs/ui-lib/lib/common/utils.ts (1)
getMajorMinorVersion(110-113)libs/ui-lib/lib/common/types/featureSupportLevel.ts (1)
FeatureId(8-15)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/config/constants.ts (22)
OPERATOR_NAME_MTV(302-302)OPERATOR_NAME_AMD_GPU(312-312)OPERATOR_NAME_LSO(298-298)OPERATOR_NAME_AUTHORINO(311-311)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_FENCE_AGENTS_REMEDIATION(316-316)OPERATOR_NAME_KMM(313-313)OPERATOR_NAME_KUBE_DESCHEDULER(318-318)OPERATOR_NAME_MCE(301-301)OPERATOR_NAME_NMSTATE(310-310)OPERATOR_NAME_NODE_FEATURE_DISCOVERY(303-303)OPERATOR_NAME_NODE_HEALTHCHECK(314-314)OPERATOR_NAME_NODE_MAINTENANCE(317-317)OPERATOR_NAME_NVIDIA_GPU(304-304)OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_OPENSHIFT_AI(308-308)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_PIPELINES(305-305)OPERATOR_NAME_SELF_NODE_REMEDIATION(315-315)OPERATOR_NAME_SERVERLESS(307-307)OPERATOR_NAME_SERVICEMESH(306-306)OPERATOR_NAME_LVM(300-300)libs/ui-lib/lib/common/config/docs_links.ts (22)
MTV_LINK(140-141)getLsoLink(155-158)AUTHORINO_OPERATOR_LINK(153-153)CNV_LINK(91-91)FENCE_AGENTS_REMEDIATION_LINK(114-115)KMM_LINK(93-94)NODE_HEALTHCHECK_LINK(108-109)getMceDocsLink(98-101)getNmstateLink(160-163)getNodeFeatureDiscoveryLink(165-168)getNvidiaGpuLink(170-173)ODF_REQUIREMENTS_LINK(82-83)ODF_LINK(96-96)OPENSHIFT_AI_REQUIREMENTS_LINK(85-86)OPENSHIFT_AI_LINK(143-144)OSC_REQUIREMENTS_LINK(88-89)OSC_LINK(146-146)PIPELINES_OPERATOR_LINK(175-176)SELF_NODE_REMEDIATION_LINK(111-112)SERVERLESS_OPERATOR_LINK(183-184)getServiceMeshLink(178-181)getLvmsDocsLink(103-106)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: tests
- GitHub Check: unit-tests
- GitHub Check: format
- GitHub Check: lint
🔇 Additional comments (24)
libs/ui-lib-tests/cypress/support/interceptors.ts (3)
25-25: Good addition of API path function.The new function follows the same pattern as other API path functions in the file, maintaining code consistency.
211-223: Well-structured interceptor for preflight requirements.The new interceptor function properly sets up a mock response for the preflight requirements API, returning a structured operators array with dependency relationships (mtv depends on cnv, which depends on lso).
488-488: Good integration of preflight requirements interceptor.The addition of
addPreflightRequirementsIntercepts()to theloadDay1Interceptsfunction ensures the interceptor is properly included in the Day 1 testing flow.libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx (4)
3-6: Import statements updated to support the refactored componentThe imports have been updated to support the new implementation approach. The component now uses
genericTableRowKeyfor consistent row key generation,TableSummaryExpandablefor better UI organization, and theuseOperatorSpecshook for centralized operator data management.
9-28: Improved operator data handling with centralized spec managementThe component now uses the
useOperatorSpecshook to access operator specifications from a central source. The rows generation logic has been improved to:
- Filter operators based on their existence in the specs collection
- Create structured row objects with proper identifiers
- Use proper memoization with all dependencies
This approach is more maintainable than the previous implementation as it centralizes operator specifications and makes the component more resilient to changes in operator definitions.
30-32: Added null check for empty operators listGood addition of a null check to prevent rendering an empty table when no operators are available.
35-54: Enhanced table rendering with expandable sectionThe table rendering has been improved by:
- Wrapping it in a
TableSummaryExpandablecomponent which provides better UX with collapse/expand functionality- Using proper key generation with
genericTableRowKeyfor optimal React rendering- Maintaining a consistent structure with the design system
This enhances both the user experience and code maintainability.
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (2)
15-15: Added import for centralized operator specs managementGood addition of the
useOperatorSpecshook import, which aligns with the broader refactoring to centralize operator specifications management.
69-69: Using centralized operator specs hookReplacing the previous approach with the
useOperatorSpecshook ensures consistent access to operator specifications across components.libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts (1)
34-34: LGTM! Style update to isFeatureSupported definitionThe change from method declaration to a property with a function type maintains the same functionality while aligning with modern TypeScript style guidelines.
libs/ui-lib/lib/common/types/clusters.ts (1)
61-64: Simplified OperatorsValues type structureThe new structure simplifies operator selection to two arrays of strings rather than the previous set of boolean flags. This approach is more scalable and easier to maintain as new operators are added.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (4)
1-15: Clean import organization.The imports have been nicely streamlined to include only what's necessary, improving code readability. The component now imports specific functionality from common tools and introduces the new OperatorsBundle and OperatorsSelect components.
17-21: Good restructuring of component props.Refactoring to accept the entire cluster object instead of individual properties is a cleaner approach. This makes the component more maintainable as cluster-related properties can be accessed directly.
22-40: Improved error handling for bundle fetching.The error handling for bundle fetching has been enhanced with proper alert notifications, using the handleApiError utility to manage API errors consistently.
42-44: Combined loading state implementation.Loading state now properly accounts for both the preflight requirements loading state and the bundles loading state, providing a more accurate user experience during data fetching.
libs/ui-lib/lib/ocm/services/OperatorsService.tsx (1)
3-8:Details
✅ Verification successful
Simplified service with focused functionality.
The OperatorsService has been significantly simplified, removing the getOLMOperators method and focusing only on fetching supported operators. The arrow function syntax for getSupportedOperators is a modern approach that maintains the same functionality.
Verify that any code previously depending on the removed getOLMOperators method has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Check for any remaining references to the removed getOLMOperators method echo "Searching for references to getOLMOperators..." rg "getOLMOperators" --type ts --type tsxLength of output: 172
Below is a revised shell script that searches for the
getOLMOperatorsreference using glob patterns (thus including both *.ts and *.tsx files) instead of the file type flag:Please run this script for manual verification to ensure there are no remaining references to the removed
getOLMOperatorsmethod.
🏁 Script executed:
#!/bin/bash echo "Searching for references to getOLMOperators in .ts and .tsx files..." rg "getOLMOperators" -g "*.ts" -g "*.tsx"Length of output: 310
Simplified Service Verified – No References to
getOLMOperatorsDetectedThe OperatorsService now solely provides the
getSupportedOperatorsmethod using a modern arrow function, and subsequent searches confirm that no remaining references to the removedgetOLMOperatorsexist in the codebase. Please ensure that any dependent code has been updated accordingly.libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (4)
24-31: Improved initialization function with better parameters.The getOperatorsInitialValues function now uses uiSettings and cluster parameters directly, which is more straightforward and maintainable than using monitoredOperators.
71-72: Clean component passing approach.Passing the entire cluster object to OperatorsStep instead of individual properties is a cleaner pattern that reduces prop drilling and improves maintainability.
78-79: Improved context usage.Directly using updateUISettings and uiSettings from the ClusterWizardContext is cleaner than managing this state separately.
107-109: Formik initialization approach.Using getOperatorsInitialValues for initialValues is a clean approach. As previously confirmed, this won't cause issues with user input since Formik only uses the initialValues on the initial render, and subsequent re-renders won't affect the form state unless enableReinitialize is set to true.
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (4)
56-66: Well-implemented version comparison function.The isOCPVersionEqualsOrMore function correctly splits version strings and compares them as numbers, avoiding the issue where parseFloat('4.10') would yield 4.1 and cause incorrect comparisons with versions like '4.9'.
68-74: Well-defined OperatorSpec type.The OperatorSpec type is well-structured with clear properties and usage of optional fields where appropriate. The use of React.ComponentType with generic parameters for Description and Requirements components is a good pattern.
76-336: Comprehensive operator specifications.The getOperatorSpecs function provides detailed specifications for all operators, including titles, feature IDs, descriptions, and requirements. This centralized approach will make it easier to maintain and extend operator information.
339-343: Efficient memoization in useOperatorSpecs hook.The useOperatorSpecs hook efficiently uses useMemo to prevent unnecessary recalculations of operator specifications, with the dependency array correctly set to only recalculate when the LVMS support level changes.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
96-96: Add error handling for missing operator specs.The current implementation directly maps operator names to titles without checking if the operator exists in the specs. If any operator name is undefined or not found in the specs, this could lead to potential issues.
-{failedOperators.map(({ name }) => opSpecs[name || '']?.title || name).join(' and ')}{' '} +{failedOperators + .map(({ name }) => { + const operatorName = name || ''; + return opSpecs[operatorName]?.title || operatorName; + }) + .join(' and ')}{' '}libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (2)
32-35: 🛠️ Refactor suggestionUse useStateSafely for all async state updates
Both
isLoadingandisExpandedstates are updated in async/callback contexts but are using standardReact.useStateinstead ofuseStateSafely. The retrieved learning explicitly mentions thatuseStateSafelyshould be used to prevent state updates on unmounted components.- const [isLoading, setIsLoading] = React.useState(true); + const [isLoading, setIsLoading] = useStateSafely(true); const { addAlert } = useAlerts(); - const [isExpanded, setIsExpanded] = React.useState(false); + const [isExpanded, setIsExpanded] = useStateSafely(false); const [supportedOperators, setSupportedOperators] = useStateSafely<string[]>([]);
58-64: 🛠️ Refactor suggestionAdd null check for operator names during sorting
The sorting function doesn't check if
aorbare null/undefined before accessing them, which could lead to errors if the API returns invalid data.const operators = React.useMemo(() => { return supportedOperators + .filter(Boolean) // Filter out null/undefined values .sort((a, b) => { - const aTitle = opSpecs[a]?.title || a; - const bTitle = opSpecs[b]?.title || b; + const aTitle = opSpecs[a]?.title || (a || ''); + const bTitle = opSpecs[b]?.title || (b || ''); return aTitle.localeCompare(bTitle); })libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (2)
17-21: 🛠️ Refactor suggestionConsider adding error handling for preflight requirements
While error handling for fetching bundles is implemented, there's no error handling for preflight requirements fetch failures through the
useClusterPreflightRequirementshook.- const { preflightRequirements, isLoading } = useClusterPreflightRequirements(cluster.id); + const { preflightRequirements, isLoading, error } = useClusterPreflightRequirements(cluster.id); + + React.useEffect(() => { + if (error) { + handleApiError(error, () => + addAlert({ + title: 'Failed to fetch preflight requirements', + message: getApiErrorMessage(error), + }), + ); + } + }, [error, addAlert]);
52-59: 🛠️ Refactor suggestionAdd null check for preflightRequirements
Both
OperatorsBundleandOperatorsSelectcomponents receive thepreflightRequirementsprop without a null check, which could cause issues if preflight requirements failed to load.- <OperatorsBundle bundles={bundles} preflightRequirements={preflightRequirements} /> + <OperatorsBundle bundles={bundles} preflightRequirements={preflightRequirements || undefined} /> </StackItem> <StackItem> <OperatorsSelect bundles={bundles} cluster={cluster} - preflightRequirements={preflightRequirements} + preflightRequirements={preflightRequirements || undefined} />libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
7-26:⚠️ Potential issueGuard against potential circular dependencies.
This recursive approach could lead to infinite recursion if operators reference each other. Use a visited
Setto prevent revisiting already processed operators.const getOperatorDependencies = ( operatorId: string, preflightRequirements: PreflightHardwareRequirements | undefined, - dependencies: Set<string> = new Set(), + dependencies: Set<string>, + seen: Set<string> = new Set(), ) => { const originalSize = dependencies.size; + if (seen.has(operatorId)) return [...dependencies]; + seen.add(operatorId); ... if (originalSize !== dependencies.size) { // add deps of new deps deps.forEach((op) => { - getOperatorDependencies(op, preflightRequirements, dependencies); + getOperatorDependencies(op, preflightRequirements, dependencies, seen); }); } return [...dependencies]; };
🧹 Nitpick comments (9)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (1)
108-112: Refactored LVM operator title retrievalThe code now uses the central operator specs for LVM operator title, following the same pattern as other operators. Note that this uses the title property without a fallback empty string like other implementations.
Consider adding a fallback empty string for consistency with other operator title retrievals:
- const operatorTitle = opSpecs[OPERATOR_NAME_LVM]?.title; + const operatorTitle = opSpecs[OPERATOR_NAME_LVM]?.title || '';Also applies to: 114-114
libs/ui-lib-tests/cypress/support/interceptors.ts (1)
211-223: Good implementation of preflight requirements intercepts.The implementation correctly mocks the API response for preflight requirements with operator dependencies. This will be useful for testing operator selection scenarios.
Consider extracting the hardcoded operators array to a fixtures file for better maintainability, especially if these operators will be referenced in other tests.
+const mockPreflightRequirementsResponse = { + operators: [ + { + operatorName: 'mtv', + dependencies: ['cnv'] + }, + { + operatorName: 'cnv', + dependencies: ['lso'] + } + ] +}; const addPreflightRequirementsIntercepts = () => { const clusterReqApiPath = getDay1ClusterPreflightRequirementsApiPath(); - cy.intercept('GET', clusterReqApiPath, { operators: [ - { - operatorName: 'mtv', - dependencies: ['cnv'] - }, - { - operatorName: 'cnv', - dependencies: ['lso'] - } - ] }).as('cluster-req'); + cy.intercept('GET', clusterReqApiPath, mockPreflightRequirementsResponse).as('cluster-req'); }libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (3)
33-108: Well-structured OperatorRequirements component.The component effectively displays hardware requirements for different node types based on preflight requirements data. The conditional rendering logic is clean and handles various cases appropriately.
Consider adding JSDoc comments to document the purpose and behavior of this component, especially since it's dealing with complex operator requirements data.
110-207: Well-implemented OperatorCheckbox component with good state management.The component handles operator selection, dependencies, and validation logic effectively. It properly uses Formik for form state management and considers various conditions for enabling/disabling the checkbox.
Some recommendations to improve readability and maintainability:
- The complex conditional for
disabledReason(lines 154-160) could be refactored for clarity- Consider adding comments to explain the logic for determining if an operator is checked or disabled
+ // Determine if the operator is disabled and the reason for it + // Priority order: bundle inclusion > standalone restriction > dependency requirement > feature support level const disabledReason = isInBundle ? 'This operator is part of a bundle and cannot be deselected.' : notStandalone ? 'This operator cannot be installed as a standalone' : parentOperatorName ? `This operator is a dependency of ${parentOperatorName}` : getFeatureDisabledReason(featureId);
182-193: Consider adding error handling for form updates.The onChange handler calls
setFieldValuebut doesn't handle potential errors that might occur during the state update process. Consider adding error handling or ensuring thatgetNewOperatorsdoesn't throw exceptions.onChange={(_, checked) => { + try { setFieldValue( 'selectedOperators', getNewOperators( values.selectedOperators, operatorId, preflightRequirements, checked, opSpecs, ), ); + } catch (error) { + console.error('Error updating operator selection:', error); + } }}libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (2)
24-26: Consider validating parameters when generating initial values.While
uiSettingsandclusterare expected to exist, adding basic checks (e.g., fallback for null or undefined) could improve resilience in edge cases.+export const getOperatorsInitialValues = ( + uiSettings: UISettingsValues | undefined, + cluster?: Cluster, +): OperatorsValues => { + if (!cluster) { + return { selectedBundles: [], selectedOperators: [] }; + } return { selectedBundles: uiSettings?.bundlesSelected || [], selectedOperators: selectOlmOperators(cluster).map((o) => o.name || ''), }; };
81-87: Handle potential empty operator names.Mapping selectedOperators into
{ name: so }is correct, but consider filtering out empty or invalid operator names, as recommended in earlier comments for a resilient approach.const enabledOperators = values.selectedOperators + .filter((so) => so.trim() !== '') .map((so) => ({ name: so, }));libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
53-62:getBundleOperatorsis straightforward.It cleanly collects all bundle operators plus their dependencies. As with
getOperatorDependencies, consider ensuring circular dependencies can’t cause infinite recursion.
64-95: HandlenewBundleorallBundlesvalidity for robust usage.If
newBundleorallBundlesis missing or malformed, filtering bynewBundle.idor searching inallBundlesmight fail. Consider basic checks before processing to reduce potential runtime errors.+ if (!newBundle || !newBundle.id) { + console.warn('getNewBundleOperators: newBundle is invalid or missing an ID.'); + return [...currentOperators]; + } + const newBundleOperators = getBundleOperators(newBundle, preflightRequirements);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (69)
libs/locales/lib/en/translation.json(0 hunks)libs/ui-lib-tests/cypress/integration/use-cases/create-cluster/with-mtv-operator.cy.ts(3 hunks)libs/ui-lib-tests/cypress/support/interceptors.ts(3 hunks)libs/ui-lib-tests/cypress/support/variables/host-discovery.ts(1 hunks)libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts(0 hunks)libs/ui-lib-tests/cypress/views/forms/Operators/OperatorsForm.ts(4 hunks)libs/ui-lib-tests/cypress/views/operatorsPage.ts(1 hunks)libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx(1 hunks)libs/ui-lib/lib/cim/components/helpers/versions.ts(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/ClusterProgressItems.tsx(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx(4 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(1 hunks)libs/ui-lib/lib/common/config/constants.ts(1 hunks)libs/ui-lib/lib/common/selectors/clusterSelectors.ts(1 hunks)libs/ui-lib/lib/common/types/clusters.ts(1 hunks)libs/ui-lib/lib/common/utils.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterProgressItems.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusters/utils.ts(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(7 hunks)libs/ui-lib/lib/ocm/components/utils.ts(0 hunks)libs/ui-lib/lib/ocm/services/OperatorsService.tsx(1 hunks)
💤 Files with no reviewable changes (37)
- libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx
- libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/utils.ts
- libs/ui-lib/lib/ocm/components/clusters/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx
- libs/locales/lib/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (20)
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css
- libs/ui-lib/lib/cim/components/helpers/versions.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterProgressItems.tsx
- libs/ui-lib/lib/common/selectors/clusterSelectors.ts
- libs/ui-lib/lib/common/utils.ts
- libs/ui-lib-tests/cypress/support/variables/host-discovery.ts
- libs/ui-lib-tests/cypress/integration/use-cases/create-cluster/with-mtv-operator.cy.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx
- libs/ui-lib/lib/common/components/clusterWizard/types.ts
- libs/ui-lib-tests/cypress/views/forms/Operators/OperatorsForm.ts
- libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx
- libs/ui-lib-tests/cypress/views/operatorsPage.ts
- libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx
- libs/ui-lib/lib/common/components/clusterDetail/ClusterProgressItems.tsx
- libs/ui-lib/lib/common/config/constants.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx
🧰 Additional context used
🧠 Learnings (1)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
Learnt from: rawagner
PR: openshift-assisted/assisted-installer-ui#2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.813Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
🧬 Code Graph Analysis (7)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (3)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
ClusterOperatorProps(72-74)libs/types/assisted-installer-service.d.ts (1)
Bundle(89-109)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (4)
libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/api/utils.ts (1)
getApiErrorMessage(53-58)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)libs/ui-lib/lib/common/config/constants.ts (1)
singleClusterOperators(320-331)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (7)
libs/types/assisted-installer-service.d.ts (3)
Cluster(110-398)PreflightHardwareRequirements(2419-2428)Bundle(89-109)libs/ui-lib/lib/ocm/store/slices/current-cluster/selectors.ts (2)
selectIsCurrentClusterSNO(7-8)selectCurrentClusterPermissionsState(9-17)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(68-74)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFieldId(15-18)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
getNewOperators(28-51)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (4)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(76-337)libs/ui-lib/lib/common/config/constants.ts (5)
OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_LVM(300-300)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_OPENSHIFT_AI(308-308)libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
isSNO(59-60)libs/ui-lib/lib/common/types/cpuArchitecture.ts (2)
architectureData(52-72)SupportedCpuArchitecture(40-43)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (6)
libs/ui-lib/lib/common/utils.ts (1)
getMajorMinorVersion(110-113)libs/ui-lib/lib/common/types/featureSupportLevel.ts (1)
FeatureId(8-15)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/config/constants.ts (22)
OPERATOR_NAME_MTV(302-302)OPERATOR_NAME_AMD_GPU(312-312)OPERATOR_NAME_LSO(298-298)OPERATOR_NAME_AUTHORINO(311-311)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_FENCE_AGENTS_REMEDIATION(316-316)OPERATOR_NAME_KMM(313-313)OPERATOR_NAME_KUBE_DESCHEDULER(318-318)OPERATOR_NAME_MCE(301-301)OPERATOR_NAME_NMSTATE(310-310)OPERATOR_NAME_NODE_FEATURE_DISCOVERY(303-303)OPERATOR_NAME_NODE_HEALTHCHECK(314-314)OPERATOR_NAME_NODE_MAINTENANCE(317-317)OPERATOR_NAME_NVIDIA_GPU(304-304)OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_OPENSHIFT_AI(308-308)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_PIPELINES(305-305)OPERATOR_NAME_SELF_NODE_REMEDIATION(315-315)OPERATOR_NAME_SERVERLESS(307-307)OPERATOR_NAME_SERVICEMESH(306-306)OPERATOR_NAME_LVM(300-300)libs/ui-lib/lib/common/config/docs_links.ts (5)
MTV_LINK(140-141)getLsoLink(155-158)AUTHORINO_OPERATOR_LINK(153-153)CNV_LINK(91-91)FENCE_AGENTS_REMEDIATION_LINK(114-115)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)
libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (2)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx (2)
getAggregatedStatus(22-30)getOperatorsLabel(34-58)
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (30)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (6)
7-11: Refactored operator imports to use centralized constantsThe code now imports operator name constants from the common location and adds the
getOperatorSpecsfunction. This approach centralizes operator definitions and improves maintainability.Also applies to: 20-20
49-52: Dynamically retrieving operator titles from specificationsThe code now uses
getOperatorSpecs()to dynamically fetch the ODF operator title, replacing hardcoded strings. This is a good pattern for maintainability and consistency.
55-55: Updated ODF disabled reason messages with dynamic titleThe error messages now use the dynamically retrieved operator title instead of hardcoded strings, which ensures consistency when operator names change.
Also applies to: 58-58, 61-61, 64-64
78-81: Improved CNV operator title handlingSimilar to the ODF changes, the code now retrieves the CNV operator title dynamically using
getOperatorSpecs(). This ensures that any changes to operator titles only need to be made in one place.Also applies to: 89-89
128-132: Updated OSC operator disabled reason messagesThe code now uses the dynamically retrieved OSC operator title, ensuring consistency across operator references in the UI.
302-306: Improved OpenShift AI operator title handlingThe code now retrieves the OpenShift AI operator title dynamically using
getOperatorSpecs(), following the same pattern as the other operators.Also applies to: 309-309, 312-312
libs/ui-lib-tests/cypress/support/interceptors.ts (2)
25-25: Good addition for API path construction.The function effectively constructs the path for preflight requirements using the clusterId from Cypress environment variables.
488-488: Good integration of the new intercept.Including the preflight requirements intercept in the Day1 flow ensures proper testing of operator selection during cluster creation.
libs/ui-lib/lib/common/types/clusters.ts (1)
61-64: Good simplification of the OperatorsValues type.The new type definition is more concise and focused on what's actually needed - arrays of selected bundles and operators. This simplification should make the code more maintainable and easier to understand.
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (2)
15-15: Good addition of the useOperatorSpecs hook.Importing and using this hook will provide a centralized way to access operator specifications, which is more maintainable than the previous approach.
69-69: Good addition of opSpecs access.Using the hook to get operator specifications is cleaner than the previous implementation.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (2)
39-54: LGTM! Appropriate asynchronous data handlingThe component correctly fetches data in a useEffect hook with proper error handling and loading state management. Using
useStateSafelyfor the operators state ensures safe updates even if the component unmounts during the fetch operation.
77-104: LGTM! Well-structured UI componentThe ExpandableSection implementation is well-structured with appropriate data-testid attributes for testing. The mapping over operators with null checking is a good practice.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (2)
22-40: LGTM! Well-implemented error handling for bundle fetchThe error handling for fetching bundles is well-implemented with appropriate use of handleApiError and providing a clear error message to the user.
42-44: LGTM! Combined loading stateGood approach to show a loading state when either bundles or preflight requirements are being loaded.
libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (2)
81-96: LGTM! Improved operator name resolutionGood refactoring to use the
useOperatorSpecshook instead of relying on the OpenShift version. The operator name resolution is now cleaner, using the operator specs title with a fallback to the operator name.
112-123: LGTM! Simplified component interfaceRemoving the unnecessary
openshiftVersionparameter from the component signature and usage simplifies the interface and improves maintainability.libs/ui-lib/lib/ocm/services/OperatorsService.tsx (1)
3-7: LGTM! Simplified service implementationThe service has been significantly simplified, removing unnecessary code while preserving the essential functionality of fetching supported operators. The arrow function syntax is more concise and aligns with modern JavaScript practices.
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (5)
5-5: Imports look properly organized.These newly introduced imports correctly reference the external types and services required by this file. No issues found regarding naming consistency or import usage.
Also applies to: 10-11, 19-19
71-71: Passing the full cluster object to OperatorsStep looks consistent.This approach ensures the child component receives the complete cluster data, which can be beneficial for consolidated logic. Good job.
78-78: Context extraction is appropriate.Destructuring
updateUISettingsanduiSettingsfrom the wizard context is clean and keeps the code readable. No concerns here.
93-93: UI settings update logic is straightforward.The call to
updateUISettingsis enclosed within the try/catch block, ensuring errors are handled gracefully. No issues found.
29-30:Details
❓ Verification inconclusive
Verify
selectOlmOperatorsusage.Check that the
selectOlmOperatorsfunction always returns a valid array. If it might return undefined, consider a safe-guard to prevent runtime errors when mapping over it.
🏁 Script executed:
#!/bin/bash # Searching for the `selectOlmOperators` definition and usage to confirm it reliably returns an array rg -A 5 "function selectOlmOperators"Length of output: 39
Verify the return type of
selectOlmOperatorsto ensure it’s an array:
The initial search for its definition usingrg -A 5 "function selectOlmOperators"returned no results. Since its usage in mapping (i.e.,selectOlmOperators(cluster).map((o) => o.name || '')) relies on receiving an array, please manually verify that the function always returns an array. If there’s any chance it might returnundefinedor a non-array value, add a safeguard (e.g., using a default empty array fallback) to prevent runtime errors.
- Action Required:
- Confirm the implementation details of
selectOlmOperators—whether it’s declared as an arrow function or imported from another module—and ensure that its return type is reliably an array.- If necessary, modify the mapping line to handle a possible
undefinedby using(selectOlmOperators(cluster) || []).libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (5)
1-47: Imports and constants are correctly structured.All required constants, documentation links, and type imports appear to be in place. The references to
@openshift-assisted/typesand local config files seem consistent.
56-66: Version comparison logic is robust.Using discrete major/minor parsing avoids floating-point inaccuracies. This solution properly handles double-digit minor versions.
68-74:OperatorSpectype definition is clear.The properties (such as
title,featureId,Description, etc.) are well-defined and make usage explicit. No improvements necessary.
339-344: useOperatorSpecs hook is well-structured.Memoizing the results of
getOperatorSpecsbased on the LVMS feature ensures efficient retrieval. Implementation looks good.
76-343:Details
❓ Verification inconclusive
Verify documentation links for correctness and consistency.
All operators define their own
Descriptionand links. Notably, the Kube Descheduler operator referencesNODE_HEALTHCHECK_LINK. This may still be an incorrect link per previous reviews. Please confirm whether there's a dedicated or more accurate link for the Descheduler feature.
🏁 Script executed:
#!/bin/bash # Description: Identify references to 'Kube Descheduler' and confirm if there's a known correct link rg "Kube Descheduler" -A 5Length of output: 1667
Documentation Link Verification for Kube Descheduler
The Kube Descheduler operator currently uses
NODE_HEALTHCHECK_LINKin itsDescription. Our search confirms that no dedicated documentation link (e.g. aKUBE_DESCHEDULER_LINK) exists within the codebase. Please review and confirm whether:
NODE_HEALTHCHECK_LINKis intentionally used for Kube Descheduler, or- a dedicated, more accurate link should be established for this operator.
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
2-5: Import statements are straightforward.The addition of
OperatorSpecand relevant types fromassisted-installer-serviceis consistent. No issues flagged.
28-51:getNewOperatorslogic clarifies addition or removal of operators and dependencies.The approach of including not-standalone dependencies only if required by other operators is concise. No immediate issues noticed.
rawagner
force-pushed
the
abi_operators
branch
2 times, most recently
from
d6e55c5 to
d08eff3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (9)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
210-217:⚠️ Potential issueIncorrect documentation link for Node Maintenance.
The Node Maintenance operator is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific link for Node Maintenance documentation.
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + <ExternalLink href="https://docs.redhat.com/en/documentation/workload_availability_for_red_hat_openshift/latest/html/remediation_fencing_and_maintenance/node-maintenance-operator">Learn more</ExternalLink>
154-164:⚠️ Potential issueIncorrect documentation link for Kube Descheduler.
The link for Kube Descheduler is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific link for Kube Descheduler.
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/scheduling/nodes-descheduler.html">Learn more</ExternalLink>libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
34-34: 🛠️ Refactor suggestionConsider using
useStateSafelyfor isExpanded stateBased on the retrieved learning,
useStateSafelyshould be used instead ofReact.useStatefor components with asynchronous operations to prevent state updates on unmounted components and avoid memory leaks.- const [isExpanded, setIsExpanded] = React.useState(false); + const [isExpanded, setIsExpanded] = useStateSafely(false);libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (2)
17-21: 🛠️ Refactor suggestionConsider adding preflight requirements error handling
The component uses
useClusterPreflightRequirementsbut doesn't handle potential errors from this hook. If the fetch fails, users won't receive any feedback.export const OperatorsStep = ({ cluster }: ClusterOperatorProps) => { const { addAlert } = useAlerts(); const [bundlesLoading, setBundlesLoading] = React.useState(true); const [bundles, setBundles] = React.useState<Bundle[]>([]); - const { preflightRequirements, isLoading } = useClusterPreflightRequirements(cluster.id); + const { preflightRequirements, isLoading, error: preflightError } = useClusterPreflightRequirements(cluster.id); + + React.useEffect(() => { + if (preflightError) { + handleApiError(preflightError, () => + addAlert({ + title: 'Failed to fetch preflight requirements', + message: getApiErrorMessage(preflightError), + }), + ); + } + }, [preflightError, addAlert]);
52-59: 🛠️ Refactor suggestionAdd null check for preflightRequirements
The
preflightRequirementsobject is passed to child components without checking if it's null or undefined, which could cause issues if it failed to load but the loading state is complete.<StackItem> - <OperatorsBundle bundles={bundles} preflightRequirements={preflightRequirements} /> + <OperatorsBundle bundles={bundles} preflightRequirements={preflightRequirements || undefined} /> </StackItem> <StackItem> <OperatorsSelect bundles={bundles} cluster={cluster} - preflightRequirements={preflightRequirements} + preflightRequirements={preflightRequirements || undefined} />libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (4)
28-51:⚠️ Potential issueEnsure null safety in operator specs access.
In the
getNewOperatorsfunction, accessingopSpecs[dep].notStandalonecould throw an error ifopSpecs[dep]is undefined.There's already a fix for this in line 44 which uses optional chaining
opSpecs[dep]?.notStandalone, which is good. However, you should ensure this fix is applied consistently throughout the codebase.
53-62:⚠️ Potential issueAdd null safety checks in getBundleOperators.
The function doesn't check if
bundleorbundle.operatorsis undefined before accessing them, which could lead to runtime errors.const getBundleOperators = ( bundle: Bundle, preflightRequirements: PreflightHardwareRequirements | undefined, ) => { + if (!bundle || !bundle.operators) { + return []; + } + const bundleOperators = new Set(bundle.operators || []); bundle.operators?.forEach((op) => { getOperatorDependencies(op, preflightRequirements).forEach((dep) => bundleOperators.add(dep)); }); return [...bundleOperators]; };
64-95:⚠️ Potential issueAdd parameter validation in getNewBundleOperators function.
The function doesn't validate its input parameters. If
newBundleis undefined orallBundlesis empty, the function could encounter runtime errors.Add validation at the beginning of the function:
export const getNewBundleOperators = ( currentOperators: string[], currentBundles: string[], allBundles: Bundle[], newBundle: Bundle, preflightRequirements: PreflightHardwareRequirements | undefined, add: boolean, ): string[] => { + if (!newBundle || !newBundle.id) { + console.warn('Invalid bundle provided to getNewBundleOperators'); + return [...currentOperators]; + } + const newBundleOperators = getBundleOperators(newBundle, preflightRequirements); if (add) { return [...new Set([...currentOperators, ...newBundleOperators])]; }
7-26:⚠️ Potential issueAdd protection against circular dependencies in recursive function.
The
getOperatorDependenciesfunction could potentially enter an infinite recursion if there are circular dependencies between operators.Modify the implementation to protect against circular dependencies:
const getOperatorDependencies = ( operatorId: string, preflightRequirements: PreflightHardwareRequirements | undefined, dependencies: Set<string> = new Set(), + processedOperators: Set<string> = new Set(), ) => { + if (processedOperators.has(operatorId)) { + return [...dependencies]; + } + + processedOperators.add(operatorId); const originalSize = dependencies.size; const deps = preflightRequirements?.operators?.find(({ operatorName }) => operatorName === operatorId) ?.dependencies || []; deps.forEach((dep) => dependencies.add(dep)); if (originalSize !== dependencies.size) { // add deps of new deps deps.forEach((op) => { - getOperatorDependencies(op, preflightRequirements, dependencies); + getOperatorDependencies(op, preflightRequirements, dependencies, processedOperators); }); } return [...dependencies]; };
🧹 Nitpick comments (7)
libs/ui-lib/lib/common/config/constants.ts (1)
135-135: Consider adding a descriptive label for nmstate requirements.The 'nmstate-requirements-satisfied' validation has an empty label string, while most other operators have descriptive labels. For consistency, consider adding an appropriate description.
- 'nmstate-requirements-satisfied': '', + 'nmstate-requirements-satisfied': t('ai:Nmstate requirements'),libs/ui-lib-tests/cypress/support/interceptors.ts (1)
211-223: Consider making the mock data configurable.The hardcoded mock data for preflight requirements might be inflexible for different test scenarios. Consider extracting this to a fixtures file or making it scenario-dependent like other mocks in this file.
- cy.intercept('GET', clusterReqApiPath, { operators: [ - { - operatorName: 'mtv', - dependencies: ['cnv'] - }, - { - operatorName: 'cnv', - dependencies: ['lso'] - } - ] }).as('cluster-req'); + const fixtureMapping = getScenarioFixtureMapping(); + const operators = fixtureMapping?.preflightRequirements?.operators || [ + { + operatorName: 'mtv', + dependencies: ['cnv'] + }, + { + operatorName: 'cnv', + dependencies: ['lso'] + } + ]; + cy.intercept('GET', clusterReqApiPath, { operators }).as('cluster-req');libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (1)
182-193: Consider adding error handling for setFieldValue operation.The onChange handler updates the form state but doesn't handle potential errors that might occur during the update process.
onChange={(_, checked) => { setFieldValue( 'selectedOperators', getNewOperators( values.selectedOperators, operatorId, preflightRequirements, checked, opSpecs, ), + (error) => { + if (error) { + console.error(`Failed to update operator ${operatorId}:`, error); + } + } ); }}libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (2)
58-71: Add filter for null/undefined values in operators listThe current implementation could encounter issues if the
supportedOperatorsarray contains null or undefined values. Consider adding a filter to handle these cases before sorting.const operators = React.useMemo(() => { return supportedOperators + .filter(Boolean) // Filter out null/undefined values .sort((a, b) => { const aTitle = opSpecs[a]?.title || a; const bTitle = opSpecs[b]?.title || b; return aTitle.localeCompare(bTitle); }) .filter((op) => { if (!isSingleClusterFeatureEnabled) { return true; } return singleClusterOperators.includes(op); }); }, [isSingleClusterFeatureEnabled, supportedOperators, opSpecs]);
85-102: Improve rendering performance with memoizationThe component renders a list of operator checkboxes inside an expandable section. Consider using
React.useMemoto memoize the rendered list of operators to avoid unnecessary re-rendering.+ const operatorCheckboxes = React.useMemo(() => { + return operators.map((operatorKey) => { + if (!opSpecs[operatorKey]) { + return null; + } + + return ( + <StackItem key={operatorKey}> + <OperatorCheckbox + bundles={bundles} + operatorId={operatorKey} + cluster={cluster} + openshiftVersion={cluster.openshiftVersion} + preflightRequirements={preflightRequirements} + {...opSpecs[operatorKey]} + /> + </StackItem> + ); + }); + }, [operators, opSpecs, bundles, cluster, preflightRequirements]); return ( <ExpandableSection toggleText={`Single Operators (${operators.length} | ${values.selectedOperators.length} selected)`} onToggle={() => setIsExpanded(!isExpanded)} isExpanded={isExpanded} data-testid="single-operators-section" > <Stack hasGutter data-testid={'operators-form'}> - {operators.map((operatorKey) => { - if (!opSpecs[operatorKey]) { - return null; - } - - return ( - <StackItem key={operatorKey}> - <OperatorCheckbox - bundles={bundles} - operatorId={operatorKey} - cluster={cluster} - openshiftVersion={cluster.openshiftVersion} - preflightRequirements={preflightRequirements} - {...opSpecs[operatorKey]} - /> - </StackItem> - ); - })} + {operatorCheckboxes} </Stack> </ExpandableSection> );libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
96-96: Consider more robust handling of failed operators mappingThe current implementation might encounter issues if the operators array contains invalid entries. Additionally, joining with "and" may not be appropriate for a large number of operators.
- {failedOperators.map(({ name }) => opSpecs[name || '']?.title || name).join(' and ')}{' '} + {failedOperators + .filter(operator => operator && operator.name) // Ensure operators and names exist + .map(({ name }) => { + const operatorName = name || ''; + return opSpecs[operatorName]?.title || operatorName; + }) + .join(failedOperators.length > 2 ? ', ' : ' and ')}{' '}libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (1)
95-95: Add null check before accessing operator propertiesThe current implementation could have issues if the operator object itself is null or undefined, not just the name property.
- const name = opSpecs[operator.name || '']?.title || operator.name; + const name = operator && opSpecs[operator.name || '']?.title || (operator && operator.name) || '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (69)
libs/locales/lib/en/translation.json(0 hunks)libs/ui-lib-tests/cypress/integration/use-cases/create-cluster/with-mtv-operator.cy.ts(3 hunks)libs/ui-lib-tests/cypress/support/interceptors.ts(3 hunks)libs/ui-lib-tests/cypress/support/variables/host-discovery.ts(1 hunks)libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts(0 hunks)libs/ui-lib-tests/cypress/views/forms/Operators/OperatorsForm.ts(4 hunks)libs/ui-lib-tests/cypress/views/operatorsPage.ts(1 hunks)libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx(1 hunks)libs/ui-lib/lib/cim/components/helpers/versions.ts(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/ClusterProgressItems.tsx(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx(4 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(1 hunks)libs/ui-lib/lib/common/config/constants.ts(1 hunks)libs/ui-lib/lib/common/selectors/clusterSelectors.ts(1 hunks)libs/ui-lib/lib/common/types/clusters.ts(1 hunks)libs/ui-lib/lib/common/utils.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterProgressItems.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusters/utils.ts(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(7 hunks)libs/ui-lib/lib/ocm/components/utils.ts(0 hunks)libs/ui-lib/lib/ocm/services/OperatorsService.tsx(1 hunks)
💤 Files with no reviewable changes (37)
- libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx
- libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts
- libs/ui-lib/lib/ocm/components/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusters/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx
- libs/locales/lib/en/translation.json
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css
- libs/ui-lib/lib/common/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterProgressItems.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx
- libs/ui-lib-tests/cypress/support/variables/host-discovery.ts
- libs/ui-lib/lib/common/selectors/clusterSelectors.ts
- libs/ui-lib/lib/common/components/clusterWizard/types.ts
- libs/ui-lib-tests/cypress/views/operatorsPage.ts
- libs/ui-lib/lib/common/components/clusterDetail/ClusterProgressItems.tsx
- libs/ui-lib-tests/cypress/views/forms/Operators/OperatorsForm.ts
- libs/ui-lib/lib/cim/components/helpers/versions.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx
- libs/ui-lib-tests/cypress/integration/use-cases/create-cluster/with-mtv-operator.cy.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx
- libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx
🧰 Additional context used
🧠 Learnings (1)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
Learnt from: rawagner
PR: openshift-assisted/assisted-installer-ui#2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.813Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
🧬 Code Graph Analysis (7)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (5)
libs/types/assisted-installer-service.d.ts (3)
Cluster(110-398)Bundle(89-109)PreflightHardwareRequirements(2419-2428)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)libs/ui-lib/lib/common/config/constants.ts (1)
singleClusterOperators(320-331)
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (3)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
ClusterOperatorProps(72-74)libs/types/assisted-installer-service.d.ts (1)
Bundle(89-109)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
libs/types/assisted-installer-service.d.ts (2)
PreflightHardwareRequirements(2419-2428)Bundle(89-109)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(68-74)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (7)
libs/types/assisted-installer-service.d.ts (3)
Cluster(110-398)PreflightHardwareRequirements(2419-2428)Bundle(89-109)libs/ui-lib/lib/ocm/store/slices/current-cluster/selectors.ts (2)
selectIsCurrentClusterSNO(7-8)selectCurrentClusterPermissionsState(9-17)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(68-74)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFieldId(15-18)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (1)
getNewOperators(28-51)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (6)
libs/ui-lib/lib/common/utils.ts (1)
getMajorMinorVersion(110-113)libs/ui-lib/lib/common/types/featureSupportLevel.ts (1)
FeatureId(8-15)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/config/constants.ts (22)
OPERATOR_NAME_MTV(302-302)OPERATOR_NAME_AMD_GPU(312-312)OPERATOR_NAME_LSO(298-298)OPERATOR_NAME_AUTHORINO(311-311)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_FENCE_AGENTS_REMEDIATION(316-316)OPERATOR_NAME_KMM(313-313)OPERATOR_NAME_KUBE_DESCHEDULER(318-318)OPERATOR_NAME_MCE(301-301)OPERATOR_NAME_NMSTATE(310-310)OPERATOR_NAME_NODE_FEATURE_DISCOVERY(303-303)OPERATOR_NAME_NODE_HEALTHCHECK(314-314)OPERATOR_NAME_NODE_MAINTENANCE(317-317)OPERATOR_NAME_NVIDIA_GPU(304-304)OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_OPENSHIFT_AI(308-308)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_PIPELINES(305-305)OPERATOR_NAME_SELF_NODE_REMEDIATION(315-315)OPERATOR_NAME_SERVERLESS(307-307)OPERATOR_NAME_SERVICEMESH(306-306)OPERATOR_NAME_LVM(300-300)libs/ui-lib/lib/common/config/docs_links.ts (22)
MTV_LINK(140-141)getLsoLink(155-158)AUTHORINO_OPERATOR_LINK(153-153)CNV_LINK(91-91)FENCE_AGENTS_REMEDIATION_LINK(114-115)KMM_LINK(93-94)NODE_HEALTHCHECK_LINK(108-109)getMceDocsLink(98-101)getNmstateLink(160-163)getNodeFeatureDiscoveryLink(165-168)getNvidiaGpuLink(170-173)ODF_REQUIREMENTS_LINK(82-83)ODF_LINK(96-96)OPENSHIFT_AI_REQUIREMENTS_LINK(85-86)OPENSHIFT_AI_LINK(143-144)OSC_REQUIREMENTS_LINK(88-89)OSC_LINK(146-146)PIPELINES_OPERATOR_LINK(175-176)SELF_NODE_REMEDIATION_LINK(111-112)SERVERLESS_OPERATOR_LINK(183-184)getServiceMeshLink(178-181)getLvmsDocsLink(103-106)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (4)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(76-337)libs/ui-lib/lib/common/config/constants.ts (5)
OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_LVM(300-300)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_OPENSHIFT_AI(308-308)libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
isSNO(59-60)libs/ui-lib/lib/common/types/cpuArchitecture.ts (2)
architectureData(52-72)SupportedCpuArchitecture(40-43)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests
🔇 Additional comments (18)
libs/ui-lib/lib/common/config/constants.ts (2)
320-331: Good improvement on operator organization.The new
singleClusterOperatorsarray provides a clear, focused list of operators for single cluster deployments. I verified that all operators in this array now have corresponding validation requirements in thehostValidationLabelsfunction, addressing the past review comment concerns.
333-333: New virtualization bundle concept introduced.The new
singleClusterBundlesarray introduces the concept of bundling operators for specific functionalities, starting with 'virtualization'. This aligns well with the PR's intent of integrating Operators into the ABI.libs/ui-lib-tests/cypress/support/interceptors.ts (2)
25-25: Good addition of path construct function.The new function
getDay1ClusterPreflightRequirementsApiPathfollows the same pattern as existing path generators, maintaining consistency in the codebase.
488-488: LGTM!Good integration of the new preflight requirements intercepts into the Day1 intercepts loading flow.
libs/ui-lib/lib/ocm/services/OperatorsService.tsx (1)
4-7: Code simplification looks good.You've successfully simplified the OperatorsService by removing the complex operator management logic and focusing it on a single responsibility. This aligns with the goal of moving operators into ABI.
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (3)
56-66: Well-implemented version comparison function.The implementation correctly compares OpenShift versions by splitting the version string and comparing the major and minor parts numerically, avoiding the floating-point conversion issues that were present in the previous code.
68-74: Good type definition for operator specifications.The
OperatorSpectype provides a clean, well-structured interface for defining operator metadata, with appropriate optional fields.
339-343: Well-implemented hook for operator specs.The
useOperatorSpecshook efficiently uses React's memoization to prevent unnecessary recalculations of operator specifications, and it correctly adapts to the LVM feature support level.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (3)
33-108: Well-designed OperatorRequirements component.The component provides comprehensive information about operator requirements in a clean and user-friendly manner. It considers both worker and master node requirements and handles conditional rendering effectively.
110-152: Good handling of operator dependencies and bundle membership.The component correctly identifies when an operator is part of a bundle or is a dependency of another selected operator, providing appropriate disabled states and tooltips.
154-161: Clear error messaging for disabled operators.The component provides specific and informative reasons why an operator might be disabled, improving the user experience when interacting with the checkboxes.
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (7)
7-11: Good update to use operator name constants.The change to import individual operator name constants (
OPERATOR_NAME_CNV,OPERATOR_NAME_LVM, etc.) instead of using hardcoded operator labels is a positive improvement that centralizes the operator naming management.
20-20: LGTM - Good addition of the operator specs import.This import allows for dynamic retrieval of operator information, which supports the refactoring in this file.
49-51: Good implementation of dynamic operator title retrieval.Replacing hardcoded operator labels with dynamic retrieval using
getOperatorSpecs()improves maintainability. The optional chaining (?.) and fallback to empty string for the title are good safety measures.
78-79: Good implementation of dynamic operator title retrieval for CNV.The refactoring follows the same pattern as for other operators, maintaining consistency in the codebase.
108-109: Good implementation of dynamic operator title retrieval for LVM.The retrieval of operator title for LVM correctly uses the constant and safely accesses the title property.
128-129: Good implementation of dynamic operator title retrieval for OSC.This follows the consistent pattern used throughout the file for other operators.
302-303: Good implementation of dynamic operator title retrieval for OpenShift AI.The refactoring maintains consistency with other similar functions in this file.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)
159-161: Incorrect documentation link for Kube Descheduler.The link for Kube Descheduler is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific link for Kube Descheduler.
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + <ExternalLink href="https://docs.openshift.com/container-platform/latest/nodes/scheduling/nodes-descheduler.html">Learn more</ExternalLink>
210-217: Incorrect documentation link for Node Maintenance.Similar to the Kube Descheduler issue, Node Maintenance is incorrectly using the NODE_HEALTHCHECK_LINK instead of a specific link for Node Maintenance.
- <ExternalLink href={NODE_HEALTHCHECK_LINK}>Learn more</ExternalLink> + <ExternalLink href="https://docs.redhat.com/en/documentation/workload_availability_for_red_hat_openshift/latest/html/remediation_fencing_and_maintenance/node-maintenance-operator">Learn more</ExternalLink>libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
96-96: Previous error handling suggestion has been implemented.The implementation now safely handles missing operator specs through the optional chaining and fallback pattern. The code correctly maps operator names to their titles with appropriate fallbacks.
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
53-62:⚠️ Potential issueAdd bundle null safety checks
The function does not check if
bundleis undefined before accessing it, which could lead to runtime errors. Previous review comments mentioned this issue was addressed in commit 6d21831, but the fix is not completely implemented here.const getBundleOperators = ( bundle: Bundle, preflightRequirements: PreflightHardwareRequirements | undefined, ) => { + if (!bundle) { + return []; + } + const bundleOperators = new Set(bundle.operators || []); bundle.operators?.forEach((op) => { getOperatorDependencies(op, preflightRequirements).forEach((dep) => bundleOperators.add(dep)); }); return [...bundleOperators]; };
64-95:⚠️ Potential issueAdd parameter validation and optimize performance
The function doesn't validate if
newBundleornewBundle.idis undefined, and could be optimized by using Sets for lookup operations. Previous review comments mentioned validation was addressed in commit 81ce50e, but the fix is not present in this code.export const getNewBundleOperators = ( currentOperators: string[], currentBundles: string[], allBundles: Bundle[], newBundle: Bundle, preflightRequirements: PreflightHardwareRequirements | undefined, add: boolean, ): string[] => { + if (!newBundle || !newBundle.id) { + console.warn('Invalid bundle provided to getNewBundleOperators'); + return [...currentOperators]; + } + const newBundleOperators = getBundleOperators(newBundle, preflightRequirements); if (add) { return [...new Set([...currentOperators, ...newBundleOperators])]; } const newBundles = currentBundles.filter((b) => b !== newBundle.id); const operatorsToKeep = [ ...newBundles.reduce((acc, bundleId) => { const bundle = allBundles.find(({ id }) => id === bundleId); if (bundle) { const bundleOperators = getBundleOperators(bundle, preflightRequirements); bundleOperators.forEach((op) => { acc.add(op); }); } return acc; }, new Set<string>()), ]; + const operatorsToKeepSet = new Set(operatorsToKeep); + const operatorsToRemove = newBundleOperators.filter((op) => !operatorsToKeepSet.has(op)); - const operatorsToRemove = newBundleOperators.filter((op) => !operatorsToKeep.includes(op)); + const operatorsToRemoveSet = new Set(operatorsToRemove); + return currentOperators.filter((op) => !operatorsToRemoveSet.has(op)); - return currentOperators.filter((op) => !operatorsToRemove.includes(op)); };libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (3)
32-37: 🛠️ Refactor suggestionInconsistent use of state management hooks.
Based on the retrieved learning from this PR,
useStateSafelyshould be used for all state that might be updated during async operations to prevent memory leaks. However,isExpandeduses regularReact.useState.Apply this change for consistent state management:
const [isLoading, setIsLoading] = useStateSafely(true); const { addAlert } = useAlerts(); - const [isExpanded, setIsExpanded] = React.useState(false); + const [isExpanded, setIsExpanded] = useStateSafely(false); const [supportedOperators, setSupportedOperators] = useStateSafely<string[]>([]);
58-71: 🛠️ Refactor suggestionAdd null check for operator names in sorting function.
The sorting function doesn't verify if
aorbare valid strings before accessing them, which could cause errors if the API returns unexpected data.const operators = React.useMemo(() => { return supportedOperators + .filter(Boolean) // Filter out null/undefined values .sort((a, b) => { - const aTitle = opSpecs[a]?.title || a; - const bTitle = opSpecs[b]?.title || b; + const aTitle = opSpecs[a]?.title || (a || ''); + const bTitle = opSpecs[b]?.title || (b || ''); return aTitle.localeCompare(bTitle); }) .filter((op) => { if (!isSingleClusterFeatureEnabled) { return true; } return singleClusterOperators.includes(op); }); }, [isSingleClusterFeatureEnabled, supportedOperators, opSpecs]);
39-54: 💡 Verification agent🧩 Analysis chain
Missing cancellation mechanism for API request.
The async API call lacks a proper cancellation mechanism, which could lead to memory leaks if the component unmounts during the API call.
If the service method doesn't support cancellation, consider implementing a cancellation mechanism using AbortController or a mounting flag:
React.useEffect(() => { + const controller = new AbortController(); + let isMounted = true; const fetchSupportedOperators = async () => { try { const fetchedOperators = await OperatorsService.getSupportedOperators(); - setSupportedOperators(fetchedOperators); + if (isMounted) { + setSupportedOperators(fetchedOperators); + } } catch (error) { + if (!isMounted) return; handleApiError(error, () => addAlert({ title: 'Failed to fetch operators', message: getApiErrorMessage(error) }), ); } finally { + if (isMounted) { setIsLoading(false); + } } }; void fetchSupportedOperators(); + return () => { + isMounted = false; + controller.abort(); + }; }, [addAlert, setSupportedOperators, setIsLoading]);
🏁 Script executed:
#!/bin/bash # Check if the OperatorsService.getSupportedOperators method accepts a signal parameter rg "getSupportedOperators.*\(" --type tsLength of output: 296
Implement API Request Cancellation
The shell script confirms that the signature of
OperatorsService.getSupportedOperators()does not accept an AbortSignal. As a result, if the component unmounts during the API call, the lack of a cancellation mechanism could lead to memory leaks. To address this, consider refactoring the code to include an AbortController (or a mounting flag) to prevent state updates after unmounting.Key steps:
- Introduce an AbortController instance.
- Use an
isMountedflag to ensure state updates occur only when the component is still mounted.- Abort pending API calls in the cleanup function.
For example:
React.useEffect(() => { + const controller = new AbortController(); + let isMounted = true; const fetchSupportedOperators = async () => { try { const fetchedOperators = await OperatorsService.getSupportedOperators(); - setSupportedOperators(fetchedOperators); + if (isMounted) { + setSupportedOperators(fetchedOperators); + } } catch (error) { + if (!isMounted) return; handleApiError(error, () => addAlert({ title: 'Failed to fetch operators', message: getApiErrorMessage(error) }), ); } finally { + if (isMounted) { setIsLoading(false); + } } }; void fetchSupportedOperators(); + return () => { + isMounted = false; + controller.abort(); + }; }, [addAlert, setSupportedOperators, setIsLoading]);Please update the implementation accordingly.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (1)
52-60: 🛠️ Refactor suggestionConsider adding null checks for preflightRequirements.
While the component passes preflightRequirements to child components, it doesn't check if the value is null or undefined, which could cause issues if the data failed to load.
Apply these changes to ensure safe prop passing:
- <OperatorsBundle bundles={bundles} preflightRequirements={preflightRequirements} /> + <OperatorsBundle bundles={bundles} preflightRequirements={preflightRequirements || undefined} /> </StackItem> <StackItem> <OperatorsSelect bundles={bundles} cluster={cluster} - preflightRequirements={preflightRequirements} + preflightRequirements={preflightRequirements || undefined} />
🧹 Nitpick comments (4)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (1)
40-67: Consider extracting operator title retrieval logic.The pattern of retrieving operator titles is repeated in multiple functions. Consider extracting this logic to a helper function to reduce duplication.
+const getOperatorTitle = (operatorName: string): string => { + const opSpecs = getOperatorSpecs(); + return opSpecs[operatorName]?.title || ''; +}; const getOdfDisabledReason = ( cluster: Cluster | undefined, activeFeatureConfiguration: ActiveFeatureConfiguration | undefined, isSupported: boolean, ) => { if (!cluster) { return undefined; } - const opSpecs = getOperatorSpecs(); - const operatorTitle = opSpecs[OPERATOR_NAME_ODF]?.title || ''; + const operatorTitle = getOperatorTitle(OPERATOR_NAME_ODF); const isArm = activeFeatureConfiguration?.underlyingCpuArchitecture === CpuArchitecture.ARM; // ... rest of the functionAlso applies to: 77-115, 127-134, 302-313
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
28-51: Optimize performance in getNewOperators functionThe function creates a new array
newOperatorsbut doesn't use it in the final return statement, resulting in redundant filtering operations.export const getNewOperators = ( currentOperators: string[], operatorId: string, preflightRequirements: PreflightHardwareRequirements | undefined, add: boolean, opSpecs: { [key: string]: OperatorSpec }, ): string[] => { const dependencies = getOperatorDependencies(operatorId, preflightRequirements); if (add) { return [...new Set([...currentOperators, ...dependencies, operatorId])]; } const newOperators = currentOperators.filter((op) => op !== operatorId); // uncheck unneeded not-standalone dependencies too const notStandaloneDeps = dependencies .filter((dep) => opSpecs[dep]?.notStandalone) .filter((dep) => // some other operator may still depend on the not-standalone operator newOperators.every((op) => !getOperatorDependencies(op, preflightRequirements).includes(dep)), ); - return currentOperators.filter((op) => op !== operatorId && !notStandaloneDeps.includes(op)); + return newOperators.filter((op) => !notStandaloneDeps.includes(op)); };
1-95: Consider further improvements to the utils fileWhile the implementations are generally solid, there are several opportunities for improvement throughout the file:
- Add type checking for recursive data structures
- Consider using memoization for the dependency calculations
- Add proper JSDoc comments to explain the purpose and behavior of each function
- Consider using functional programming patterns more consistently
Additionally, this file would benefit from comprehensive unit tests to ensure the complex dependency logic works correctly in all edge cases.
libs/ui-lib-tests/cypress/support/interceptors.ts (1)
211-223: Add documentation for mock data selection.The mock data includes specific operators (mtv, cnv, lso) with dependencies, but lacks documentation explaining why these particular operators were chosen for testing.
Consider adding a comment explaining the significance of these operators and their dependency relationships:
const addPreflightRequirementsIntercepts = () => { const clusterReqApiPath = getDay1ClusterPreflightRequirementsApiPath(); + // Mock response with common operators and their dependencies + // mtv (Migration Toolkit for Virtualization) depends on cnv (Container Native Virtualization) + // cnv depends on lso (Local Storage Operator) cy.intercept('GET', clusterReqApiPath, { operators: [ { operatorName: 'mtv', dependencies: ['cnv'] }, { operatorName: 'cnv', dependencies: ['lso'] } ] }).as('cluster-req'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
libs/locales/lib/en/translation.json(0 hunks)libs/ui-lib-tests/cypress/integration/use-cases/create-cluster/with-mtv-operator.cy.ts(3 hunks)libs/ui-lib-tests/cypress/support/interceptors.ts(3 hunks)libs/ui-lib-tests/cypress/support/variables/host-discovery.ts(1 hunks)libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts(0 hunks)libs/ui-lib-tests/cypress/views/forms/Operators/OperatorsForm.ts(4 hunks)libs/ui-lib-tests/cypress/views/operatorsPage.ts(1 hunks)libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx(1 hunks)libs/ui-lib/lib/cim/components/helpers/versions.ts(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/ClusterProgressItems.tsx(1 hunks)libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx(4 hunks)libs/ui-lib/lib/common/components/clusterWizard/types.ts(1 hunks)libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(1 hunks)libs/ui-lib/lib/common/config/constants.ts(1 hunks)libs/ui-lib/lib/common/selectors/clusterSelectors.ts(1 hunks)libs/ui-lib/lib/common/types/clusters.ts(1 hunks)libs/ui-lib/lib/common/utils.ts(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterProgressItems.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts(0 hunks)libs/ui-lib/lib/ocm/components/clusters/utils.ts(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx(0 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(7 hunks)libs/ui-lib/lib/ocm/components/utils.ts(0 hunks)libs/ui-lib/lib/ocm/services/OperatorsService.tsx(1 hunks)
💤 Files with no reviewable changes (39)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAIRequirements.tsx
- libs/ui-lib/lib/ocm/components/utils.ts
- libs/ui-lib-tests/cypress/views/bareMetalDiscovery.ts
- libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContextProvider.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/wizardTransition.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeHealthcheckCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAIAmdRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OdfCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/PipelinesChekbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KubeDeschedulerCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/VirtualizationRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/CnvHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OscCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/KmmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NmstateRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SelfNodeRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AmdGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NvidiaGpuCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusters/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmHostRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenShiftAICheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/AuthorinoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OpenshiftAINvidiaRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeMaintenanceOperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/FenceAgentsRemediationCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/NodeFeatureDiscoveryCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MceCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LvmCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/LsoCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/MtvRequirements.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServicemeshCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/ServerlessCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/SupportedOperators.tsx
- libs/locales/lib/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (21)
- libs/ui-lib-tests/cypress/views/operatorsPage.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/DiscoveryImageModal.tsx
- libs/ui-lib/lib/cim/components/featureSupportLevels/ACMFeatureSupportLevelProvider.tsx
- libs/ui-lib/lib/common/selectors/clusterSelectors.ts
- libs/ui-lib-tests/cypress/support/variables/host-discovery.ts
- libs/ui-lib/lib/common/components/clusterDetail/ClusterProgressItems.tsx
- libs/ui-lib/lib/common/components/clusterWizard/types.ts
- libs/ui-lib-tests/cypress/views/forms/Operators/OperatorsForm.ts
- libs/ui-lib/lib/common/utils.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/bundleSpecs.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx
- libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts
- libs/ui-lib-tests/cypress/integration/use-cases/create-cluster/with-mtv-operator.cy.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmClusterProgressItems.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.css
- libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewSummary.tsx
- libs/ui-lib/lib/cim/components/helpers/versions.ts
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx
- libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx
🧰 Additional context used
🧠 Learnings (2)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
Learnt from: rawagner
PR: openshift-assisted/assisted-installer-ui#2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx:32-55
Timestamp: 2025-04-14T09:11:08.813Z
Learning: In React components with asynchronous operations, `useStateSafely` custom hook should be used instead of `React.useState` to prevent state updates on unmounted components and avoid memory leaks.
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (1)
Learnt from: rawagner
PR: openshift-assisted/assisted-installer-ui#2899
File: libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx:107-109
Timestamp: 2025-04-14T09:08:46.974Z
Learning: Formik only uses the initialValues prop during the initial mounting of the component. Any changes to the initialValues prop in subsequent renders will be ignored unless the enableReinitialize prop is explicitly set to true.
🧬 Code Graph Analysis (6)
libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (5)
libs/types/assisted-installer-service.d.ts (3)
Cluster(110-398)Bundle(89-109)PreflightHardwareRequirements(2419-2428)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-64)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
useOperatorSpecs(339-343)libs/ui-lib/lib/common/config/constants.ts (1)
singleClusterOperators(320-331)
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (3)
libs/ui-lib/lib/common/components/clusterWizard/types.ts (1)
ClusterOperatorProps(72-74)libs/types/assisted-installer-service.d.ts (1)
Bundle(89-109)libs/ui-lib/lib/common/api/utils.ts (2)
handleApiError(19-51)getApiErrorMessage(53-58)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (2)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
getOperatorSpecs(76-337)libs/ui-lib/lib/common/config/constants.ts (5)
OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_LVM(300-300)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_OPENSHIFT_AI(308-308)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (6)
libs/ui-lib/lib/common/utils.ts (1)
getMajorMinorVersion(110-113)libs/ui-lib/lib/common/types/featureSupportLevel.ts (1)
FeatureId(8-15)libs/types/assisted-installer-service.d.ts (1)
Cluster(110-398)libs/ui-lib/lib/common/config/constants.ts (22)
OPERATOR_NAME_MTV(302-302)OPERATOR_NAME_AMD_GPU(312-312)OPERATOR_NAME_LSO(298-298)OPERATOR_NAME_AUTHORINO(311-311)OPERATOR_NAME_CNV(297-297)OPERATOR_NAME_FENCE_AGENTS_REMEDIATION(316-316)OPERATOR_NAME_KMM(313-313)OPERATOR_NAME_KUBE_DESCHEDULER(318-318)OPERATOR_NAME_MCE(301-301)OPERATOR_NAME_NMSTATE(310-310)OPERATOR_NAME_NODE_FEATURE_DISCOVERY(303-303)OPERATOR_NAME_NODE_HEALTHCHECK(314-314)OPERATOR_NAME_NODE_MAINTENANCE(317-317)OPERATOR_NAME_NVIDIA_GPU(304-304)OPERATOR_NAME_ODF(299-299)OPERATOR_NAME_OPENSHIFT_AI(308-308)OPERATOR_NAME_OSC(309-309)OPERATOR_NAME_PIPELINES(305-305)OPERATOR_NAME_SELF_NODE_REMEDIATION(315-315)OPERATOR_NAME_SERVERLESS(307-307)OPERATOR_NAME_SERVICEMESH(306-306)OPERATOR_NAME_LVM(300-300)libs/ui-lib/lib/common/config/docs_links.ts (5)
MTV_LINK(140-141)getLsoLink(155-158)AUTHORINO_OPERATOR_LINK(153-153)CNV_LINK(91-91)FENCE_AGENTS_REMEDIATION_LINK(114-115)libs/ui-lib/lib/common/components/newFeatureSupportLevels/NewFeatureSupportLevelContext.tsx (1)
useNewFeatureSupportLevel(17-25)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/utils.tsx (2)
libs/types/assisted-installer-service.d.ts (2)
PreflightHardwareRequirements(2419-2428)Bundle(89-109)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
OperatorSpec(68-74)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: unit-tests
- GitHub Check: circular-deps
- GitHub Check: lint
- GitHub Check: format
🔇 Additional comments (25)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (4)
56-66: Good implementation of the version comparison function!The function correctly splits version numbers and compares them as numbers, avoiding potential issues with floating-point comparisons (like
parseFloat('4.10')yielding4.1).
68-74: Well-structured type definition for OperatorSpec.Clean and comprehensive interface with appropriate optionality for fields like Description and Requirements.
321-335: Clear conditional rendering for LVM operator.The implementation correctly handles different descriptions based on the
useLVMSflag. This makes the component adaptable to different configurations.
339-343: Efficient implementation of the useOperatorSpecs hook.The hook properly uses React.useMemo to optimize performance by only recalculating operator specs when the LVMS support level changes.
libs/ui-lib/lib/common/config/constants.ts (1)
333-333: New constant for virtualization bundles.The addition of
singleClusterBundlesaligns with the refactoring to better organize operator-related constants.libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (2)
7-20: Clean import restructuring.The imports have been updated to include operator name constants and the new
getOperatorSpecsfunction, supporting the transition to dynamic operator specifications.
49-52: Good transition to dynamic operator titles.Replacing hardcoded operator labels with dynamic titles from
getOperatorSpecsimproves maintainability and consistency.libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (2)
15-15: Good addition of the useOperatorSpecs hook.Adding this import improves code organization by centralizing operator specs retrieval logic.
69-69: Effective use of the centralized hook.Using the
useOperatorSpecshook simplifies the code by removing the need for a separate function to handle operator names and translations.libs/ui-lib-tests/cypress/support/interceptors.ts (2)
25-25: Good utility function for API path construction.The function provides a clear way to construct the API path for preflight requirements, improving code readability and maintainability.
488-488: Good integration of preflight requirements intercepts.The integration of the new
addPreflightRequirementsInterceptsfunction into theloadDay1Interceptsfunction ensures that the mock data is properly loaded during Day 1 flow testing.libs/ui-lib/lib/common/types/clusters.ts (1)
61-64: Excellent simplification of OperatorsValues type.This change significantly improves the type definition by replacing what was previously a complex structure with boolean flags to a more flexible array-based approach. This simplification makes the codebase more maintainable and accommodates future operator additions without requiring type changes.
The new structure with
selectedBundlesandselectedOperatorsas string arrays provides a clear separation between these two concepts while maintaining flexibility.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (1)
77-105: Good use of expandable section and component composition.The component effectively uses the ExpandableSection component to provide a clean UI for operator selection, and the integration with OperatorCheckbox components is well-implemented. The display of total and selected operator counts provides useful feedback to the user.
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsStep.tsx (3)
17-21: Improved component signature and hooks usage.The destructured props pattern is more readable than using the generic
propsparameter. The component effectively uses hooks for state management and API data fetching.
22-40: Enhanced error handling for bundle fetching.The error handling for bundle fetching has been improved with proper use of
handleApiErrorand alert notifications, making the user experience better when errors occur.
42-44: Proper combined loading state implementation.The component correctly checks both
isLoading(for preflight requirements) andbundlesLoadingto determine when to show the loading state, ensuring a consistent user experience.libs/ui-lib/lib/ocm/services/OperatorsService.tsx (1)
1-10: LGTM! Code has been significantly simplified.The service has been streamlined to provide only the essential
getSupportedOperatorsmethod with cleaner arrow function syntax. This aligns with the overall refactoring approach of simplifying operator management in the codebase.libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (8)
5-11: Import changes look good.The imports have been updated to include the proper types needed for the refactored approach, particularly adding
Clusterfrom the types package and usingselectOlmOperatorsinstead of direct operator management.
19-20: Service import change is appropriate.Importing
ClustersServicedirectly is consistent with the simplification of operator management, as the component now performs cluster updates directly.
24-31: Function signature change improves modularity.The
getOperatorsInitialValuesfunction now accepts the completeclusterobject and UI settings, which is more flexible than the previous approach. The implementation correctly usesselectOlmOperatorsto extract operator data from the cluster.Previous review comments about null safety checks have been addressed in commit 6d21831.
71-71: Simplifying props improves maintainability.Passing the complete
clusterobject toOperatorsStepinstead of individual properties makes the code more maintainable and reduces the need to update multiple component signatures when the data requirements change.
78-78: Good use of context API.Correctly destructuring both
updateUISettingsanduiSettingsfrom the context ensures consistent access to UI settings across the component.
81-86: Function simplification is a positive change.The
handleSubmitfunction has been streamlined to directly map selected operators without unnecessary complexity. This approach is cleaner and easier to understand.Previous review comments about safeguarding empty operator names have been addressed in commit 81ce50e.
93-93: UI settings update looks good.Direct update of UI settings with selected bundles is appropriate.
Previous review comments about error handling for
updateUISettingshave been addressed in commit 6d21831.
107-107: Formik initialization is correct.Using
getOperatorsInitialValuesto generate initial values is clean and appropriate. As previously discussed in the PR comments, this won't cause issues since Formik only uses the initialValues prop during the initial mounting of the component unless enableReinitialize is explicitly set to true.
|
/lgtm |
|
[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 3f4d6c6
into
openshift-assisted:master
|
/cherry-pick releases/v2.39 |
|
@ammont82: #2899 failed to apply on top of branch "releases/v2.39": 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. |
|
git am --show-current-patch=diff |
* NO-ISSUE: First step in ABI is Operators too * Reimplement operators step
* NO-ISSUE: First step in ABI is Operators too * Reimplement operators step
6 participants
Summary by CodeRabbit
New Features
OperatorsBundlecomponent for managing operator bundles in the cluster configuration interface.OperatorsSelectcomponent to display and manage a list of selectable operators based on cluster configuration.OperatorsStepcomponent to streamline bundle fetching and selection with improved modularity.Bug Fixes
Refactor
ReviewOperatorsTablecomponent to utilize a new method for filtering and displaying operators based on specifications.OperatorsServicefor better maintainability.Operatorscomponent for operator selection.OcmClusterProgressItemscomponent to simplify prop usage.ClusterProgressItemscomponent by removing unnecessary props.DiscoveryImageModalcomponent to improve callback handling.AmdGpuCheckbox,AuthorinoCheckbox, and others to declutter the codebase.Chores