MGMT-20483: Use categories for operators#2998
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ammont82: This pull request references MGMT-20483 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe changes restructure operator specifications by introducing categorized grouping, centralized description constants, and enhanced metadata such as support levels. The Changes
Sequence Diagram(s)sequenceDiagram
participant UI_Component
participant useOperatorSpecs
participant operatorSpecs
participant operatorDescriptions
UI_Component->>useOperatorSpecs: Call useOperatorSpecs()
useOperatorSpecs->>operatorSpecs: getOperatorSpecs(getFeatureSupportLevel, useLVMS)
operatorSpecs->>operatorDescriptions: Fetch descriptionText constants
operatorSpecs-->>useOperatorSpecs: Return { byCategory, byKey }
useOperatorSpecs-->>UI_Component: Return { byCategory, byKey }
UI_Component->>byKey: Access operator spec by key
UI_Component->>byCategory: Iterate operators by category
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/common/components/newFeatureSupportLevels/types.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/common/components/operators/operatorSpecs.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/ocm/components/clusterConfiguration/operators/OperatorCheckbox.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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
@ammont82: This pull request references MGMT-20483 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx (1)
9-15: Runtime crash whencluster.monitoredOperatorsisundefinedIf
monitoredOperatorsis missing (common for new/draft clusters),?.filterreturnsundefined, and the subsequent.mapcall throws.- cluster.monitoredOperators - ?.filter(({ name }) => !!name && !!opSpecs[name]) - .map((op) => { + (cluster.monitoredOperators ?? []) + .filter(({ name }) => !!name && !!opSpecs[name]) + .map((op) => {Guarding with
?? []ensures the table renders empty instead of crashing.
🧹 Nitpick comments (2)
libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts (1)
49-53: Minor perf/readability nit – fetch specs once
getOperatorSpecsByKey()is invoked five times.
Because it returns a stable, immutable map, cache it once at module scope to avoid repeated work and to keep code DRY.-import { getOperatorSpecsByKey } from '../../../common/components/operators/operatorSpecs'; +import { getOperatorSpecsByKey } from '../../../common/components/operators/operatorSpecs'; + +// memoised once per module evaluation +const OPERATOR_SPECS = getOperatorSpecsByKey();Then replace the individual
const opSpecs = getOperatorSpecsByKey();lines withOPERATOR_SPECS.Also applies to: 78-82, 108-112, 128-132, 302-304
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
463-479: Consider moving enum and categories to the top of the fileFor better code organization and readability, consider moving the
Categoryenum andcategoriesmapping to the top of the file, before they are used in thegetOperatorSpecsfunction.+enum Category { + STORAGE, + VIRT, + AI, + NETWORK, + REMEDIATION, + OTHER, +} + +export const categories: { [key in Category]: string } = { + [Category.STORAGE]: 'Storage', + [Category.VIRT]: 'Virtualization', + [Category.AI]: 'AI', + [Category.NETWORK]: 'Network', + [Category.REMEDIATION]: 'Remediation', + [Category.OTHER]: 'Other', +}; + // TODO check if it's unused and it can be deleted in favor of "isMajorMinorVersionEqualOrGreater" export const isOCPVersionEqualsOrMore = (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx(1 hunks)libs/ui-lib/lib/common/components/operators/operatorDescriptions.tsx(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/review/ReviewOperatorsTable.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(2 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: tests
- GitHub Check: circular-deps
- GitHub Check: translation-files
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: format
🔇 Additional comments (10)
libs/ui-lib/lib/common/components/clusterDetail/OperatorsProgressItem.tsx (1)
83-85: Destructuring looks goodSwitching to
{ byKey: opSpecs }is consistent with the new hook contract – no issues spotted here.libs/ui-lib/lib/ocm/components/clusterConfiguration/OcmOperatorProgressItem.tsx (1)
83-85: Consistent use ofbyKeySame change as in the generic component; everything compiles cleanly.
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (1)
129-131: ```shell
#!/bin/bashSearch for any occurrence, import, or definition of getNewOperators across the codebase
rg -n "getNewOperators" -C3
</details> <details> <summary>libs/ui-lib/lib/ocm/components/clusterDetail/ProgressBarAlerts.tsx (1)</summary> `69-69`: **LGTM!** The destructuring of `byKey` from `useOperatorSpecs()` correctly adapts to the new operator specs structure while maintaining the same functionality. </details> <details> <summary>libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx (1)</summary> `36-36`: **Refactor correctly applied throughout the component** All changes consistently adapt to the new operator specs structure: - Import includes the `OperatorSpec` type for proper typing - Both `BundleLabel` and `BundleCard` components correctly destructure `byKey` - The `getBundleSupportLevel` function signature properly reflects the new type Also applies to: 42-42, 84-84, 90-90, 117-117 </details> <details> <summary>libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx (2)</summary> `56-56`: **Excellent implementation of categorized operator display** The refactor successfully implements the categorized display of operators. The UI now groups operators by their categories, which improves organization and user experience. Also applies to: 83-108 --- `59-65`: **Verify removal of operator sorting is intentional** The previous implementation sorted operators by title. With the new categorized display, operators appear in the order they're defined within each category. Is this change intentional? </details> <details> <summary>libs/ui-lib/lib/common/components/operators/operatorDescriptions.tsx (1)</summary> `1-59`: **Well-structured centralization of operator descriptions** Excellent separation of concerns by extracting all operator descriptions into a dedicated module. The descriptions are concise, informative, and follow a consistent naming pattern. </details> <details> <summary>libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (2)</summary> `57-80`: **Excellent implementation of categorized operator specifications** The refactor successfully: - Organizes operators into logical categories - Integrates centralized descriptions - Maintains backward compatibility via the `byKey` property - Provides flexible access patterns for different use cases Also applies to: 96-104, 106-432 --- `434-461`: **Well-designed hook with proper memoization** The `useOperatorSpecs` hook and `getOperatorSpecsByKey` utility function are well-implemented: - Proper memoization prevents unnecessary recalculations - Provides both categorized and flat access patterns - The utility function enables usage outside React components </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
/cherry-pick releases/v2.42 |
|
@ammont82: once the present PR merges, I will cherry-pick it on top of 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. |
|
@ammont82: This pull request references MGMT-20483 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx (1)
123-136: Parameter name shadows outer constant – leads to accidentalundefinedpropagationInside
getDisabledReasonCallback, the parametergetFeatureSupportLevelshadows the outerconst getFeatureSupportLevel.
If the caller omits this argument (the common case), the inner value isundefined, yet it is the one forwarded togetNewFeatureDisabledReason, so the default implementation is lost.- getFeatureSupportLevel?: GetFeatureSupportLevel, + getFeatureSupportLevelParam: GetFeatureSupportLevel = getFeatureSupportLevel, useLVMS?: boolean, … - getFeatureSupportLevel, + getFeatureSupportLevelParam, useLVMS,This preserves the provider’s own function while still allowing overrides.
♻️ Duplicate comments (1)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
507-523: Inefficient lookup – repeats earlier feedback
getOperatorSpecByKeystill rebuilds all specs and performs an O(N) search each call.
You already exposebyKeyfromuseOperatorSpecs; favour that map or create a cached map once.
🧹 Nitpick comments (3)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (3)
108-112:getOperatorSpecsrebuilds a large object on each callEvery invocation recreates ~200 lines of static data, incurring unnecessary GC pressure. Either:
- Memoise on
[getFeatureSupportLevel, useLVMS], or- Pre-compute the static parts (title/description) once and only inject
supportLeveldynamically.This pays off when
getOperatorSpecByKeyand external hooks call the helper frequently.
461-473: Naming mismatch may confuse future maintainers
getOperatorstakesisSingleClusterFeatureEnabled, butuseOperatorSpecsforwardsisSingleFeatureEnabled.
Consider renaming the second parameter (or both) to the same identifier to avoid cognitive overhead.
480-486:useLVMSrecomputation is fine, but dependency list already covers it – tiny simplification possibleSince
useLVMSdepends solely ongetFeatureSupportLevel, you can inline the boolean into theuseMemothat buildsbyCategory, removing an extra variable:- const useLVMS = getFeatureSupportLevel('LVM') === 'supported'; - const byCategory = React.useMemo( - () => getOperatorSpecs(getFeatureSupportLevel, useLVMS), - [getFeatureSupportLevel, useLVMS], - ); + const byCategory = React.useMemo(() => { + const lvmsEnabled = getFeatureSupportLevel('LVM') === 'supported'; + return getOperatorSpecs(getFeatureSupportLevel, lvmsEnabled); + }, [getFeatureSupportLevel]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(4 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(2 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(1 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/FeatureSupportLevelProvider.tsx(3 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx
- libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: translation-files
- GitHub Check: circular-deps
- GitHub Check: unit-tests
- GitHub Check: tests
- GitHub Check: lint
- GitHub Check: format
🔇 Additional comments (2)
libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts (2)
17-21: Type alias relocation and style LGTM
GetFeatureSupportLevelis now properly hoisted and typed – no functional or stylistic issues spotted.
27-29: Confirm downstream call-sites now pass the two new parametersAdding
getFeatureSupportLevelanduseLVMStoGetFeatureDisabledReasonexpands the signature. Make sure every invocation (e.g. infeatureStateUtils.ts& consuming UI components) is adjusted; otherwise, TypeScript will silently passundefined, and disabled-reason logic that relies on these may break.
This reverts commit bd0d086.
|
@ammont82: This pull request references MGMT-20483 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.20.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
483-494: Potential performance issue with repeated operator specs creation.The function calls
getOperatorSpecs(() => undefined)on every invocation, which recreates all operator specifications. For better performance, consider memoizing this or using the existinguseOperatorSpecshook data.export const getOperatorTitleByFeatureId = (featureId: FeatureId): string | undefined => { // Consider memoizing this or accepting the specs as a parameter const allSpecs = getOperatorSpecs(() => undefined); for (const categorySpecs of Object.values(allSpecs)) { const spec = categorySpecs.find((s) => s.featureId === featureId); if (spec) { return spec.title; } } return undefined; };Also, this addresses the past review comment about getting a single operator spec without building the entire map each time.
🧹 Nitpick comments (1)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
107-458: Consider refactoring to reduce code duplication.While the categorized structure is well-organized, the function has significant code duplication across operator definitions. Each operator follows nearly identical patterns with only different values.
Consider extracting a helper function to create operator specs:
const createOperatorSpec = ( operatorKey: string, title: string, featureId: FeatureId, category: string, getFeatureSupportLevel: GetFeatureSupportLevel, options: { descriptionText?: string; Description?: React.ComponentType<{ openshiftVersion?: string }>; Requirements?: React.ComponentType<{ cluster: Cluster }>; notStandalone?: boolean; } ): OperatorSpec => ({ operatorKey, title, featureId, category, supportLevel: getFeatureSupportLevel(featureId), ...options, });This would reduce duplication and make the operator definitions more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts(1 hunks)libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx(1 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx(3 hunks)libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx(2 hunks)libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/ui-lib/lib/ocm/components/featureSupportLevels/featureStateUtils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx
- libs/ui-lib/lib/common/components/newFeatureSupportLevels/types.ts
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.tsx
- libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsBundle.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: unit-tests
- GitHub Check: tests
- GitHub Check: translation-files
- GitHub Check: format
- GitHub Check: lint
- GitHub Check: circular-deps
🔇 Additional comments (4)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (4)
2-80: LGTM! Well-organized imports for the refactor.The new imports for SupportLevel type, GetFeatureSupportLevel hook, and centralized operator descriptions are properly organized and support the categorization refactor effectively.
95-105: Enhanced OperatorSpec type with useful metadata fields.The additions of
operatorKey,descriptionText,category, andsupportLevelfields provide better structure and metadata for operator specifications, supporting the categorization goals.
460-481: Efficient implementation with proper memoization.The hook correctly uses
useMemofor bothbyCategoryandbyKeymaps, ensuring good performance. The flattening logic forbyKeyis clean and efficient.
496-512: Clean category definitions with proper TypeScript enum usage.The Category enum and categories mapping provide type-safe category management. The string values are user-friendly and appropriate for UI display.
celdrake
left a comment
celdrake
left a comment
There was a problem hiding this comment.
Apologies for the suggestion to move logic to the hook, since the module structure would have made the code more complex than it should.
Looks great like this!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ammont82, celdrake, 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 91974ec
into
openshift-assisted:master
|
@ammont82: new pull request created: #3002 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. |
* Classify operators by categories * Improving the categories * Revert "Improving the categories" This reverts commit bd0d086. * Improving categories for operators
5 participants
Related with: https://issues.redhat.com/browse/MGMT-20483
Use these categories for operators:
Summary by CodeRabbit
New Features
Refactor
Chores