CONSOLE-4709: Remove kebab factory uses from resourceQuota and pod-list pages #15772
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@vikram-raj: This pull request references CONSOLE-4709 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 story to target the "4.21.0" version, but no target version was set. In 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. |
|
/cc @yapei For QE review |
WalkthroughAdds two resource-provider registrations for ResourceQuota and ClusterResourceQuota, refactors quota action menus to use LazyActionMenu with new public references, removes an exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (8)
Comment |
openshift-ci
bot
added
component/core
![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
🧹 Nitpick comments (1)
frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.ts (1)
37-38: Consider adding type annotation for thedataparameter.The
dataparameter is untyped, which reduces type safety. Consider adding a type annotation to document the expected shape{ quota: K8sResourceKind; namespace: string }.-export const useAppliedClusterResourceQuotaActionsProvider = (data) => { - const { quota, namespace } = data; +export const useAppliedClusterResourceQuotaActionsProvider = (data: { + quota: K8sResourceKind; + namespace: string; +}) => { + const { quota, namespace } = data;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
frontend/packages/console-app/console-extensions.json(3 hunks)frontend/packages/console-app/package.json(1 hunks)frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.ts(1 hunks)frontend/public/components/pod-list.tsx(0 hunks)frontend/public/components/resource-quota.jsx(5 hunks)frontend/public/locales/en/public.json(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/public/components/pod-list.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-app/package.jsonfrontend/public/locales/en/public.jsonfrontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.tsfrontend/packages/console-app/console-extensions.jsonfrontend/public/components/resource-quota.jsx
🔇 Additional comments (6)
frontend/packages/console-app/package.json (1)
102-102: LGTM!The new module exposure follows the established naming convention and pattern used by other action providers in this file.
frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.ts (1)
1-75: Well-structured provider implementation.The hook follows established patterns from other action providers, correctly aggregates common actions (labels, annotations, delete) with the custom edit action, and properly handles the loading state via
!inFlight.frontend/public/locales/en/public.json (1)
1720-1720: LGTM!Translation entry for the new edit action is correctly added.
frontend/packages/console-app/console-extensions.json (1)
533-567: LGTM!The three new resource providers are correctly configured:
ResourceQuotaandClusterResourceQuotaappropriately use the default actions providerAppliedClusterResourceQuotacorrectly references the new custom provider that handles the quota/namespace context shapefrontend/public/components/resource-quota.jsx (2)
64-76: Clean refactoring from static Kebab actions to dynamic LazyActionMenu.The
quotaActionsfunction correctly maps each quota type to its corresponding action menu with the appropriate context shape. TheAppliedClusterResourceQuotacase correctly passes{ quota, namespace }which matches the expected shape in the new provider.
811-847: LGTM!The
DetailsPagecomponents are correctly updated to usecustomActionMenuwithLazyActionMenu, maintaining consistency with the list view actions. TheAppliedClusterResourceQuotasDetailsPagecorrectly passes the namespace fromuseParams()in the context.
vikram-raj
force-pushed
the
console-4709-public-3
branch
from
0b6d121 to
b527515
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.ts (1)
11-35: Consider addingaccessReviewto the edit action for permission-based visibility.Other action providers in the codebase typically include an
accessReviewproperty to conditionally show/hide actions based on user permissions. Without it, users without edit permissions will see the action but encounter an error when navigating.const useEditAppliedClusterResourceQuotaActions = ( resource: K8sResourceKind, namespace: string, ) => { const { t } = useTranslation(); const navigate = useNavigate(); const factory = useMemo( () => ({ editResourceQuota: () => ({ id: 'edit-resource-quota', label: t('public~Edit AppliedClusterResourceQuota'), + accessReview: { + group: AppliedClusterResourceQuotaModel.apiGroup, + resource: AppliedClusterResourceQuotaModel.plural, + name: resource?.metadata?.name, + namespace, + verb: 'update', + }, cta: () => navigate( `/k8s/ns/${namespace}/${referenceForModel(AppliedClusterResourceQuotaModel)}/${ resource?.metadata?.name }/yaml`, ), }), }), - [t, navigate, namespace, resource?.metadata?.name], + [t, navigate, namespace, resource], );frontend/public/components/resource-quota.jsx (1)
64-76: Add a fallback return for unmatched quota types.The function implicitly returns
undefinedif none of the conditions match. While this may work in practice, an explicit return makes the intent clear and prevents potential rendering issues.const quotaActions = (quota, namespace = undefined) => { if (quota.metadata.namespace) { return <LazyActionMenu context={{ [referenceForModel(ResourceQuotaModel)]: quota }} />; } if (quota.kind === 'ClusterResourceQuota') { return <LazyActionMenu context={{ [clusterQuotaReference]: quota }} />; } if (quota.kind === 'AppliedClusterResourceQuota') { return <LazyActionMenu context={{ [appliedClusterQuotaReference]: { quota, namespace } }} />; } + + return null; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
frontend/packages/console-app/console-extensions.json(3 hunks)frontend/packages/console-app/package.json(1 hunks)frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.ts(1 hunks)frontend/public/components/pod-list.tsx(0 hunks)frontend/public/components/resource-quota.jsx(5 hunks)frontend/public/locales/en/public.json(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/public/components/pod-list.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/console-app/package.json
- frontend/public/locales/en/public.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.tsfrontend/public/components/resource-quota.jsxfrontend/packages/console-app/console-extensions.json
🔇 Additional comments (5)
frontend/packages/console-app/console-extensions.json (1)
533-567: LGTM! New action providers for quota resources are properly registered.The three new
console.action/resource-providerentries follow the established pattern. UsingdefaultProviderforResourceQuotaandClusterResourceQuotais appropriate, whileAppliedClusterResourceQuotacorrectly uses a custom provider that handles its unique context requirements (passing both quota and namespace).frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.ts (1)
37-77: LGTM! Provider implementation follows established patterns.The hook correctly composes common actions (labels, annotations, delete) with the custom edit action and returns them in a logical order. The memoization is appropriately applied to avoid unnecessary re-renders.
frontend/public/components/resource-quota.jsx (3)
811-825: LGTM! DetailsPage migration to customActionMenu is correct.The migration from
menuActionstocustomActionMenuwithLazyActionMenufollows the new pattern. UsingActionMenuVariant.DROPDOWNis appropriate for the details page context.
827-847: LGTM! AppliedClusterResourceQuota DetailsPage correctly passes quota and namespace in context.The context shape
{ quota: obj, namespace: params?.ns }matches what theuseAppliedClusterResourceQuotaActionsProviderexpects.
456-459: LGTM! Row actions correctly use the quotaActions helper.The LazyActionMenu is now rendered directly in the cell, which is the expected pattern for the new action system.
vikram-raj
force-pushed
the
console-4709-public-3
branch
2 times, most recently
from
69ffc01 to
3d24bb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/public/components/resource-quota.jsx (1)
816-835: Unusedk8sObjparameter in customActionMenu callback.The callback receives
(k8sObj, obj)but only usesobj. Verify if this is intentional or ifk8sObjshould be used instead. Ifk8sObjis not needed, consider using an underscore prefix (_k8sObj) to indicate it's intentionally ignored.customActionMenu={(k8sObj, obj) => ( + customActionMenu={(_k8sObj, obj) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
frontend/packages/console-app/console-extensions.json(3 hunks)frontend/packages/console-app/package.json(1 hunks)frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.ts(1 hunks)frontend/public/components/pod-list.tsx(0 hunks)frontend/public/components/resource-quota.jsx(5 hunks)frontend/public/locales/en/public.json(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/public/components/pod-list.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-app/package.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.tsfrontend/public/locales/en/public.jsonfrontend/packages/console-app/console-extensions.jsonfrontend/public/components/resource-quota.jsx
🧬 Code graph analysis (2)
frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.ts (4)
frontend/public/models/index.ts (1)
AppliedClusterResourceQuotaModel(728-743)frontend/public/components/utils/inject.ts (1)
kindObj(23-40)frontend/public/module/k8s/k8s.ts (1)
referenceFor(50-65)frontend/packages/console-app/src/actions/hooks/useCommonActions.ts (1)
useCommonActions(38-196)
frontend/public/components/resource-quota.jsx (2)
frontend/packages/console-dynamic-plugin-sdk/src/api/internal-api.ts (1)
LazyActionMenu(67-68)frontend/public/models/index.ts (1)
ResourceQuotaModel(696-709)
🔇 Additional comments (10)
frontend/public/locales/en/public.json (1)
1720-1720: Translation key reorganization looks good.The move of the "Edit AppliedClusterResourceQuota" translation entry is clean—the key-value pair remains identical and JSON translation files have no ordering dependencies, so this reorganization aligns naturally with the PR's AppliedClusterResourceQuota refactoring work.
frontend/packages/console-app/src/actions/providers/applied-cluster-resource-quota-provider.ts (1)
12-79: AppliedClusterResourceQuota actions provider wiring looks solidThe edit action, common actions (labels/annotations/delete), and model loading via
useK8sModel(referenceFor(quota))are composed cleanly, and the returned tuple shape[resourceQuotaActions, !inFlight, false]matches the existing provider pattern. I don’t see correctness or maintainability issues here.frontend/packages/console-app/console-extensions.json (3)
435-437: VolumeAttributesClass provider reformat is non-functionalOnly the JSON shape for
"provider"was expanded to multi-line; the codeRef is unchanged, so behavior stays the same.
1326-1327: VAC_PLATFORM_SUPPORT flag normalization is OKThe flags block for the VolumeAttributesClasses navigation item is just reformatted; required flag and behavior are unchanged.
533-567: Quota action providers are correctly modeled and hooked upThe new
console.action/resource-providerentries forResourceQuota,ClusterResourceQuota, andAppliedClusterResourceQuotause the expected group/version/kind values and route the last one throughappliedClusterResourceQuotaProvider.useAppliedClusterResourceQuotaActionsProvider, which matches the new hook implementation.frontend/public/components/resource-quota.jsx (5)
56-57: LGTM!Imports are correctly added for the new action menu components, consistent with the SDK's internal API structure.
64-77: LGTM!The quota type discrimination logic is sound—namespaced resources have
metadata.namespace, while cluster-scoped quotas do not. The context structures correctly match the expected action provider signatures.
457-460: LGTM!Correct integration of
quotaActionsin the data view rows, passing both the quota object and namespace context.
520-523: LGTM!Consistent usage of
quotaActionsfor AppliedClusterResourceQuota rows.
812-814: Verify action menu behavior for ResourceQuotasDetailsPage.The component no longer explicitly configures a custom action menu, unlike
AppliedClusterResourceQuotasDetailsPage. Confirm that the action menu is correctly provided through the spreadpropsor defaultDetailsPagebehavior forResourceQuotaresources.
|
/retest |
|
@yanpzhan please help verify, thanks! |
|
@vikram-raj Checked on cluster launched against the pr, when user edited label/annotation from one AppliedClusterResourceQuota action/kebab list, error showed up after user clicked 'Save'. |
|
Same error when delete AppliedClusterResourceQuota: |
vikram-raj
force-pushed
the
console-4709-public-3
branch
from
3d24bb6 to
741446f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/public/components/resource-quota.jsx (1)
486-515: Critical: Missing actions cell for AppliedClusterResourceQuota rows.The
rowCellsobject ingetAppliedClusterResourceQuotaDataViewRowsonly defines cells for column indices 0-4, butappliedClusterResourceQuotaTableColumnInfohas 6 columns (indices 0-5). The actions column (index 5) is missing, so it will render as DASH instead of showing the action menu.This is inconsistent with
getResourceQuotaDataViewRows(lines 452-455) which includes the actions cell, and contributes to the QE-reported issue where actions are unavailable for AppliedClusterResourceQuota.Add the missing actions cell after line 514:
[appliedClusterResourceQuotaTableColumnInfo[4].id]: { cell: <Timestamp timestamp={metadata.creationTimestamp} />, }, + [appliedClusterResourceQuotaTableColumnInfo[5].id]: { + cell: quotaActions(obj, namespace), + props: actionsCellProps, + }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
frontend/packages/console-app/console-extensions.json(3 hunks)frontend/public/components/pod-list.tsx(0 hunks)frontend/public/components/resource-quota.jsx(3 hunks)frontend/public/locales/en/public.json(0 hunks)
💤 Files with no reviewable changes (2)
- frontend/public/components/pod-list.tsx
- frontend/public/locales/en/public.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/components/resource-quota.jsxfrontend/packages/console-app/console-extensions.json
🧬 Code graph analysis (1)
frontend/public/components/resource-quota.jsx (2)
frontend/public/models/index.ts (3)
ClusterResourceQuotaModel(711-726)AppliedClusterResourceQuotaModel(728-743)ResourceQuotaModel(696-709)frontend/packages/console-dynamic-plugin-sdk/src/api/internal-api.ts (1)
LazyActionMenu(67-68)
🔇 Additional comments (3)
frontend/packages/console-app/console-extensions.json (2)
435-437: LGTM: Formatting improvement.The multi-line formatting of the
providerproperty improves readability without any functional change.
1313-1315: LGTM: Formatting improvement.The multi-line formatting of the
flagsobject is consistent with the formatting style used elsewhere in the file.frontend/public/components/resource-quota.jsx (1)
804-804: Cannot verify action provider registration due to repository access failure.The review comment requests verification that the ResourceQuota action provider is properly registered in console-extensions.json, which is a valid concern. However, I'm unable to access the repository to confirm whether this registration actually exists in the codebase.
While web search confirms that ResourceQuota action providers should follow the
console.action/resource-providerpattern with appropriate model group/version/kind and provider CodeRef, I cannot definitively verify:
- If the ResourceQuota provider is actually registered in the project's console-extensions.json
- If the registration matches the required structure
- If the LazyActionMenu will correctly pick up these actions
Manual verification needed: Check that the ResourceQuota and AppliedClusterResourceQuota action providers are properly registered in the console-extensions.json file following the documented pattern for
console.action/resource-providerextensions.
vikram-raj
force-pushed
the
console-4709-public-3
branch
from
741446f to
71639ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/packages/console-app/console-extensions.json(3 hunks)frontend/public/components/resource-quota.jsx(3 hunks)frontend/public/locales/en/public.json(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/public/locales/en/public.json
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-app/console-extensions.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/public/components/resource-quota.jsx
🧬 Code graph analysis (1)
frontend/public/components/resource-quota.jsx (2)
frontend/public/models/index.ts (3)
ClusterResourceQuotaModel(711-726)AppliedClusterResourceQuotaModel(728-743)ResourceQuotaModel(696-709)frontend/packages/console-dynamic-plugin-sdk/src/api/internal-api.ts (1)
LazyActionMenu(67-68)
🔇 Additional comments (2)
frontend/public/components/resource-quota.jsx (2)
56-56: LGTM: LazyActionMenu import.The import correctly replaces the previous Kebab-based approach.
804-804: LGTM: DetailsPage migration to LazyActionMenu.Both changes correctly migrate to the LazyActionMenu pattern:
- Removed
menuActionsprop to rely on LazyActionMenu context-based actions- Added explicit
kindprop for AppliedClusterResourceQuota to ensure proper action provider resolutionAlso applies to: 811-811
vikram-raj
force-pushed
the
console-4709-public-3
branch
from
71639ac to
b07e9ff
Compare
vikram-raj
force-pushed
the
console-4709-public-3
branch
from
b07e9ff to
2f725d8
Compare
|
/label px-approved |
openshift-ci
bot
added
the
px-approved
openshift-ci
bot
added
the
docs-approved
|
There is not blocking issue for the pr now. |
openshift-ci-robot
added
the
verified
|
@yanpzhan: This PR has been marked as verified by In 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. |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, vikram-raj The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@vikram-raj: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
5 participants
Remove the use of the kebab factory from the following pages of the devconsole plugin
Changes from the big PR #15520