OCPBUGS-72585: Do not resolve disabled catalog type extensions #15908
Conversation
openshift-ci-robot
added
jira/severity-moderate
|
@logonoff: This pull request references Jira Issue OCPBUGS-72585, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughFilters disabled catalog item types during extension processing, replaces useResolvedExtensions with useExtensions, derives disabled sub-catalogs from SERVER_FLAGS via a memoized hook, adds item relevance scoring/sorting helpers, and updates tests to mock/use the new useExtensions behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@logonoff: This pull request references Jira Issue OCPBUGS-72585, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@logonoff: This pull request references Jira Issue OCPBUGS-72585, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/operator-lifecycle-manager/console-extensions.json (1)
378-388: AddSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLEDto operator item-provider flags for consistent gating.The
console.catalog/item-providerfor operators (line 386) only disallowsOLMV1_ENABLED, while the correspondingconsole.catalog/item-type(line 440) andcategories-provider(line 450) both includeSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLED. This asymmetric gating breaks the established pattern: theOperatorBackedServiceitem-provider and item-type both use their respectiveSOFTWARE_CATALOG_TYPE_*_DISABLEDflags consistently. The operator item-provider should includeSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLEDin its disallowed array to ensure symmetric feature gating and prevent data fetching when the operator catalog is disabled.
🤖 Fix all issues with AI agents
In @frontend/packages/dev-console/src/actions/providers.tsx:
- Around line 370-384: useDisabledCatalogTypes currently only sets
SOFTWARE_CATALOG_TYPE_<TYPE>_DISABLED to true for items in catalogTypes.disabled
and never resets flags to false; update the logic in useDisabledCatalogTypes so
you compute the union of catalogTypes.disabled and catalogTypes.enabled (or a
known set of types), then call setFeatureFlag for each type: set true when the
type is in catalogTypes.disabled, otherwise set false, ensuring flags are
explicitly reset when a type is re-enabled; reference the
useDisabledCatalogTypes function, the catalogTypes.disabled/
catalogTypes.enabled values, and the setFeatureFlag call.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
frontend/packages/dev-console/console-extensions.jsonfrontend/packages/dev-console/src/actions/providers.tsxfrontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsxfrontend/packages/dev-console/src/types/catalog.tsfrontend/packages/helm-plugin/console-extensions.jsonfrontend/packages/knative-plugin/console-extensions.jsonfrontend/packages/operator-lifecycle-manager-v1/console-extensions.jsonfrontend/packages/operator-lifecycle-manager/console-extensions.json
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx (1)
frontend/packages/dev-console/src/types/catalog.ts (1)
DeveloperCatalogTypes(3-7)
frontend/packages/dev-console/src/actions/providers.tsx (2)
frontend/packages/console-dynamic-plugin-sdk/src/extensions/feature-flags.ts (1)
FeatureFlagHandler(46-46)frontend/packages/dev-console/src/types/catalog.ts (1)
SoftwareCatalogTypesConsoleConfig(9-17)
🔇 Additional comments (19)
frontend/packages/operator-lifecycle-manager-v1/console-extensions.json (1)
52-76: LGTM! Consistent gating of OLMv1 operator catalog extensions.Both
console.catalog/item-providerandconsole.catalog/categories-providerare correctly gated withSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLEDdisallowed flag. This ensures hooks won't execute when the operator catalog type is disabled via console-operator config.frontend/packages/operator-lifecycle-manager/console-extensions.json (2)
264-295: LGTM! OperatorBackedService extensions properly gated.Both
console.catalog/item-typeandconsole.catalog/item-providerforOperatorBackedServiceare correctly configured withSOFTWARE_CATALOG_TYPE_OPERATORBACKEDSERVICE_DISABLEDin the disallowed flags. This ensures the hooks won't run when this catalog type is disabled.
438-452: LGTM! Operator item-type and categories-provider correctly gated.The
console.catalog/item-typefor operators andconsole.catalog/categories-providerboth includeSOFTWARE_CATALOG_TYPE_OPERATOR_DISABLEDin their disallowed flags, ensuring proper feature-flag gating when the catalog type is disabled.frontend/packages/helm-plugin/console-extensions.json (1)
182-217: LGTM! Helm chart catalog extensions properly gated.Both
console.catalog/item-typeandconsole.catalog/item-providerforHelmChartcorrectly includeSOFTWARE_CATALOG_TYPE_HELMCHART_DISABLEDin their disallowed flags alongside the existingOPENSHIFT_HELMrequired flag. This ensures Helm chart hooks won't execute when the catalog type is disabled.frontend/packages/knative-plugin/console-extensions.json (3)
473-504: LGTM! EventType catalog extensions properly gated.Both
console.catalog/item-typeandconsole.catalog/item-providerforEventTypecorrectly includeSOFTWARE_CATALOG_TYPE_EVENTTYPE_DISABLEDin disallowed flags.
706-750: LGTM! EventSource catalog extensions comprehensively gated.All three EventSource-related extensions are properly configured:
console.catalog/item-typefor EventSourceconsole.catalog/item-providerusinguseEventSourceProviderconsole.catalog/item-providerusinguseKameletsProviderAll include
SOFTWARE_CATALOG_TYPE_EVENTSOURCE_DISABLEDin their disallowed flags.
751-795: LGTM! EventSink catalog extensions comprehensively gated.All three EventSink-related extensions are properly configured:
console.catalog/item-typefor EventSinkconsole.catalog/item-providerusinguseKameletsSinkProviderconsole.catalog/item-providerusinguseKafkaSinkProviderAll include
SOFTWARE_CATALOG_TYPE_EVENTSINK_DISABLEDin their disallowed flags, ensuring consistent behavior when the catalog type is disabled.frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx (2)
18-18: LGTM! Clean type consolidation.Good refactoring to import
DeveloperCatalogTypesandSoftwareCatalogTypesConsoleConfigfrom the centralized types file. This improves maintainability by having a single source of truth for these types, which aligns with the broader PR changes introducing the catalog type gating mechanism.
52-52: LGTM! State typing updated correctly.The
useState<DeveloperCatalogTypes>()correctly aligns with the imported type, which per the code snippet includesstate: 'Enabled' | 'Disabled'plus optionalenabledanddisabledarrays. This matches the existing usage throughout the component.frontend/packages/dev-console/src/types/catalog.ts (2)
9-17: LGTM!The
SoftwareCatalogTypesConsoleConfigtype properly extendsK8sResourceKindand uses appropriate optional chaining for the nested config structure. This aligns well with the console operator config schema pattern.
3-7: Thestatefield andenabledarray are actively used throughout the codebase and the type definition is well-implemented. Thestatefield determines the filtering mode:'Enabled'operates as an allowlist (only types in theenabledarray are shown), while'Disabled'operates as a denylist (types in thedisabledarray are hidden). BothCatalogTypesConfigurationand the shared catalog utilities properly respect this semantic. No changes needed.frontend/packages/dev-console/src/actions/providers.tsx (2)
72-80: LGTM!The
useSoftwareCatalogProvidercorrectly uses theFeatureFlagHandlertype, aligning with the established pattern in this codebase. The hook calls within this handler follow theconsole.flag/hookProviderextension contract.
26-26: LGTM!New imports for
useConsoleOperatorConfigandSoftwareCatalogTypesConsoleConfigare appropriate for the new functionality.Also applies to: 39-39
frontend/packages/dev-console/console-extensions.json (6)
174-179: LGTM!The new
console.flag/hookProviderextension correctly wiresuseDisabledCatalogTypesto set feature flags based on console operator config. This follows the established pattern used byuseSoftwareCatalogProvider.
469-486: LGTM!The
disallowedflags for BuilderImage item-type and item-provider are correctly added. The flag nameSOFTWARE_CATALOG_TYPE_BUILDERIMAGE_DISABLEDmatches the uppercase transformation inuseDisabledCatalogTypes.
495-512: LGTM!Template item-type and item-provider correctly gated with
SOFTWARE_CATALOG_TYPE_TEMPLATE_DISABLED.
521-538: LGTM!Devfile item-type and item-provider correctly gated with
SOFTWARE_CATALOG_TYPE_DEVFILE_DISABLED.
562-587: LGTM!The samples-catalog providers for BuilderImage and Devfile types are correctly gated with their respective
_DISABLEDflags. This ensures that when a catalog type is disabled, both the main catalog and the samples catalog respect the configuration.
539-561: Flag naming and usage are consistent and correct.The codebase dynamically generates
SOFTWARE_CATALOG_TYPE_<TYPE>_DISABLEDflags from the operator config's disabled array values (seefrontend/packages/dev-console/src/actions/providers.tsx). This ensures that cluster admins simply list catalog type names (e.g.,"Sample") and the flag name generation handles consistency automatically.Note:
CONSOLESAMPLEis a separate feature flag for the ConsoleSample provider itself and serves a different purpose than the catalog type disabling mechanism. The two are used correctly.
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
951e59a to
b0ab1fb
Compare
openshift-ci
bot
added
component/dev-console
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
b0ab1fb to
706397e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts:
- Around line 34-44: The isNotDisabledType predicate currently returns falsy
when e.properties.type is undefined, which incorrectly filters out
global/typeless extensions; update the isNotDisabledType callback so it returns
true when e.properties.type is not set (i.e., allow typeless entries) and only
checks catalogTypes.disabled.includes(e.properties.type) when a type exists;
ensure you modify the function named isNotDisabledType (used alongside
CatalogCategoriesProvider predicates) to first short-circuit for
!e.properties.type and otherwise check the disabled list.
🧹 Nitpick comments (3)
frontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.ts (1)
7-13: Consider making the hook return type explicit (to prevent tuple-shape drift).Since callers commonly destructure this hook, an explicit return type (based on
useK8sWatchResource) can help catch mismatches early if the watch hook’s signature evolves.frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (2)
3-4: Type import layering: console-shared depending on dev-console types is awkward long-term.Even as a type-only import, this creates coupling from
console-sharedtodev-console. Consider movingSoftwareCatalogTypesConsoleConfig(or a narrower “developerCatalog.types” shape) into a shared location withinconsole-shared(or a common types package) to keep dependency direction clean.
46-116: Add focused tests for disabled types + type-less extensions.Given the new filtering is now a cross-cutting gate for all resolved extension kinds, please add coverage for:
- a disabled type being filtered out for each extension kind you expect to be typed
- an extension with no
properties.typestill being retained (if supported/expected), especially forCatalogCategoriesProviderAlso applies to: 118-147
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.tsfrontend/packages/console-shared/src/components/cluster-configuration/index.tsfrontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.tsfrontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsxfrontend/packages/dev-console/src/types/catalog.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/packages/dev-console/src/types/catalog.ts
- frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (2)
frontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.ts (1)
useConsoleOperatorConfig(7-13)frontend/packages/dev-console/src/types/catalog.ts (1)
SoftwareCatalogTypesConsoleConfig(9-17)
frontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.ts (1)
frontend/packages/console-shared/src/components/cluster-configuration/index.ts (1)
useConsoleOperatorConfig(3-3)
🔇 Additional comments (1)
frontend/packages/console-shared/src/components/cluster-configuration/index.ts (1)
3-3: Refactor to named export is safe—no default imports remain.The codebase has been successfully sanitized: no default imports of
useConsoleOperatorConfigexist. Both the source file and the index correctly export it as a named export, forcing all consumers to use the named import pattern. The transition is complete with no breaking references lingering.
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
706397e to
8f769d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts:
- Around line 34-48: The isNotDisabledType callback must handle
DeveloperCatalogTypes' state modes and guard against undefined arrays: if
e.properties.type is missing, keep returning true; otherwise read catalogTypes
from consoleConfig and if catalogTypes.state === 'Enabled' return
catalogTypes.enabled?.includes(e.properties.type) === true; if
catalogTypes.state === 'Disabled' and catalogTypes.disabled is undefined treat
that as "everything disabled" (return false), else return
!catalogTypes.disabled?.includes(e.properties.type); ensure you never call
.includes on an undefined array by using optional chaining or nullish coalescing
when checking enabled/disabled arrays.
- Line 3: The import of SoftwareCatalogTypesConsoleConfig (and similarly
INCONTEXT_ACTIONS_CONNECTS_TO) from the plugin package creates a forbidden
coupling; move the shared catalog types/constants into the console-shared
package and update imports: extract SoftwareCatalogTypesConsoleConfig into a new
type file (e.g., console-shared's types catalog export) and extract
INCONTEXT_ACTIONS_CONNECTS_TO into a console-shared constants module, export
them, then replace the import in useCatalogExtensions.ts (and any other
references) to import from the new console-shared types/constants instead of
from @console/dev-console.
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
127-129: Redundant filtering —itemTypeExtensionsalready filtered.The
itemTypeExtensionsarray was already filtered byisNotDisabledTypeduringuseResolvedExtensionsresolution (line 54). This second.filter(isNotDisabledType)iterates the array again without effect.♻️ Remove redundant filter
(catalogType ? itemTypeExtensions.filter((e) => e.properties.type === catalogType) : itemTypeExtensions - ) - .filter(isNotDisabledType) - .map((e) => { + ).map((e) => {Also remove
isNotDisabledTypefrom the dependency array at line 150 if this is cleaned up.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.tsfrontend/packages/console-shared/src/components/cluster-configuration/index.tsfrontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.tsfrontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsxfrontend/packages/dev-console/src/types/catalog.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/packages/console-shared/src/components/cluster-configuration/useConsoleOperatorConfig.ts
- frontend/packages/dev-console/src/types/catalog.ts
- frontend/packages/console-shared/src/components/cluster-configuration/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx (1)
frontend/packages/dev-console/src/types/catalog.ts (1)
DeveloperCatalogTypes(3-7)
🔇 Additional comments (2)
frontend/packages/dev-console/src/components/catalog/CatalogTypesConfiguration.tsx (1)
18-18: Clean type extraction to shared module — LGTM.Good separation of concerns moving
DeveloperCatalogTypesandSoftwareCatalogTypesConsoleConfigto a dedicated types module. This aligns with our pattern of keeping reusable types accessible across packages.Also applies to: 52-52
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts (1)
50-120: Consistent filtering across all extension types — good pattern.The
isNotDisabledTypepredicate is correctly applied to all relevant extension resolution callbacks (CatalogItemType,CatalogItemTypeMetadata,CatalogItemProvider,CatalogItemFilter,CatalogCategoriesProvider,CatalogItemMetadataProvider), and dependency arrays are properly updated.Once the logic issue in
isNotDisabledTypeis addressed, this will properly prevent disabled catalog hooks from executing.
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/catalog/hooks/useCatalogExtensions.ts
Outdated
Show resolved
Hide resolved
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
8f769d4 to
c14b708
Compare
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
10d9681 to
0bfaee7
Compare
|
/assign @yapei |
|
/retest-required |
|
@logonoff could you rebase the pr? Could not launch cluster with cluster-bot currently. |
openshift-merge-robot
added
the
needs-rebase
logonoff
force-pushed
the
OCPBUGS-72585-devfile
branch
from
0bfaee7 to
862bf21
Compare
openshift-merge-robot
removed
the
needs-rebase
|
Tested on cluster launched against the pr, set Cluster Samples Operator as /verified by yanpzhan |
openshift-ci-robot
added
the
verified
|
@yanpzhan: This PR has been marked as verified by 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: logonoff, TheRealJon, vojtechszocs 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 |
|
/retest |
1 similar comment
|
/retest |
|
@logonoff: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold Revision 862bf21 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
8 participants
Previously, if a catalog/item-type was disabled in console-operator config, the hooks associated with that catalog type would still resolve and run.
Now, if a catalog category is disabled, the associated hooks will not run.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.