OCPBUGS-98943: Make developerCatalog.types matching case-insensitive - #16876
OCPBUGS-98943: Make developerCatalog.types matching case-insensitive#16876ericahinkleRH wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@ericahinkleRH: This pull request references Jira Issue OCPBUGS-98943, 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCatalog utility filtering now performs case-insensitive matching for configured catalog types. Tests cover missing, enabled, and disabled ChangesCatalog type matching
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Note on validation: This PR makes type matching case-insensitive, which fixes the immediate issue where The JIRA also mentions that invalid type IDs are silently ignored (no validation warnings). That's a separate enhancement beyond the scope of this fix - the console would need to add validation that checks if a configured type ID matches any registered catalog type extension and warn users if not. For now, case-insensitive matching significantly reduces the chance of typos causing silent failures. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ericahinkleRH The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/packages/console-shared/src/components/catalog/utils/__tests__/catalog-utils.spec.tsx (1)
482-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the hook’s mixed-case extension filtering.
These tests exercise
isCatalogTypeEnabledonly. Add auseGetAllDisabledSubCatalogstest with, for example, anOperatorextension andenabled: ['operator'], asserting that it is excluded from disabled sub-catalogs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/packages/console-shared/src/components/catalog/utils/__tests__/catalog-utils.spec.tsx` around lines 482 - 532, Add a test for useGetAllDisabledSubCatalogs that configures an Operator extension with developerCatalogTypes enabled for operator, then assert the extension is excluded from the returned disabled sub-catalogs. Keep the existing isCatalogTypeEnabled tests unchanged and use the hook’s established test setup and result assertion patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@frontend/packages/console-shared/src/components/catalog/utils/__tests__/catalog-utils.spec.tsx`:
- Around line 482-532: Add a test for useGetAllDisabledSubCatalogs that
configures an Operator extension with developerCatalogTypes enabled for
operator, then assert the extension is excluded from the returned disabled
sub-catalogs. Keep the existing isCatalogTypeEnabled tests unchanged and use the
hook’s established test setup and result assertion patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c5339755-f023-4555-a9e3-c19944dc9d90
📒 Files selected for processing (2)
frontend/packages/console-shared/src/components/catalog/utils/__tests__/catalog-utils.spec.tsxfrontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
d36e63a to
b053d13
Compare
|
Added test coverage for |
openshift-ci-robot
added
the
jira/valid-bug
|
@ericahinkleRH: This pull request references Jira Issue OCPBUGS-98943, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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-robot
removed
the
jira/invalid-bug
b053d13 to
0992066
Compare
|
Fixed test failures - removed the The hook test was using improper mocking that broke all other tests in the file. Since
The hook's case-insensitive filtering is validated through the function it calls. |
|
@ericahinkleRH: This pull request references Jira Issue OCPBUGS-98943, which is valid. 3 validation(s) were run on this bug
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. |
|
/test frontend |
|
/test analyze |
0992066 to
15b889a
Compare
|
Found and fixed the test issue! Problem: The tests were using Solution: Rewrote tests to follow the existing pattern in the codebase:
The tests now follow the same pattern as |
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@frontend/packages/console-shared/src/components/catalog/utils/__tests__/catalog-utils.spec.tsx`:
- Around line 474-480: Update the test setup around beforeEach and afterEach to
preserve an unmodified snapshot of window.SERVER_FLAGS before deleting
developerCatalogTypes. Restore that independent snapshot in afterEach so the
original flags object is not mutated and later tests remain isolated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 99c5955e-ce69-4bbf-bcf5-61c8e2a561c1
📒 Files selected for processing (2)
frontend/packages/console-shared/src/components/catalog/utils/__tests__/catalog-utils.spec.tsxfrontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/packages/console-shared/src/components/catalog/utils/catalog-utils.tsx
| beforeEach(() => { | ||
| originalServerFlags = window.SERVER_FLAGS; | ||
| delete window.SERVER_FLAGS.developerCatalogTypes; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| window.SERVER_FLAGS = originalServerFlags; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restore the original flags without mutating them.
delete window.SERVER_FLAGS.developerCatalogTypes mutates the object referenced by originalServerFlags; Line 480 reassigns that already-mutated object. This leaks the deleted configuration into later tests and makes their result order-dependent.
Proposed fix
beforeEach(() => {
originalServerFlags = window.SERVER_FLAGS;
+ window.SERVER_FLAGS = { ...originalServerFlags };
delete window.SERVER_FLAGS.developerCatalogTypes;
});📝 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.
| beforeEach(() => { | |
| originalServerFlags = window.SERVER_FLAGS; | |
| delete window.SERVER_FLAGS.developerCatalogTypes; | |
| }); | |
| afterEach(() => { | |
| window.SERVER_FLAGS = originalServerFlags; | |
| beforeEach(() => { | |
| originalServerFlags = window.SERVER_FLAGS; | |
| window.SERVER_FLAGS = { ...originalServerFlags }; | |
| delete window.SERVER_FLAGS.developerCatalogTypes; | |
| }); | |
| afterEach(() => { | |
| window.SERVER_FLAGS = originalServerFlags; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@frontend/packages/console-shared/src/components/catalog/utils/__tests__/catalog-utils.spec.tsx`
around lines 474 - 480, Update the test setup around beforeEach and afterEach to
preserve an unmodified snapshot of window.SERVER_FLAGS before deleting
developerCatalogTypes. Restore that independent snapshot in afterEach so the
original flags object is not mutated and later tests remain isolated.
The developerCatalog.types.enabled configuration had inconsistent case sensitivity - "operator" (lowercase) worked but "Operator" (capitalized) did not, while "HelmChart" and "Template" worked with capitals. This change makes catalog type matching case-insensitive so users don't have to guess the correct capitalization. Changes: - Update isCatalogTypeEnabled() to use case-insensitive comparison - Update useGetAllDisabledSubCatalogs() to use case-insensitive comparison - Add comprehensive unit tests for case-insensitive matching - Both enabled and disabled type filtering now work regardless of capitalization This fixes the inconsistency where: - developerCatalog.types.enabled: ["Operator"] now works (previously failed) - developerCatalog.types.enabled: ["operator"] continues to work - developerCatalog.types.enabled: ["OPERATOR"] now works - All existing configurations (HelmChart, Template, Devfile, etc.) continue to work Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
15b889a to
0fea936
Compare
|
Fixed test isolation issue identified by CodeRabbit: Problem: Was saving a reference to Solution: Now properly save just the This ensures tests are properly isolated and don't affect each other. |
|
/test backend |
|
The backend test failure is a test infrastructure flake unrelated to our frontend changes. Error: chartmuseum TLS server failed to start on port 9443 (process died with exit status 2) Our changes: Only modified frontend TypeScript files - zero Go code touched. Already triggered |
|
/ok-to-test |
openshift-ci
Bot
added
the
ok-to-test
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-playwright |
1 similar comment
|
/test e2e-playwright |
|
@ericahinkleRH: 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. |
4 participants
Analysis / Root cause:
The
developerCatalog.types.enabledconfiguration inconsole.operator.openshift.ioCR had inconsistent case sensitivity. The catalog typeoperator(lowercase) worked, butOperator(capitalized) did not, even thoughHelmChartandTemplateworked with capitals.Root cause: The filtering logic used exact string matching (
.includes()), so users had to know the exact capitalization of each catalog type identifier to configure it correctly.Solution description:
Made catalog type matching case-insensitive by normalizing both the configured types and the comparison types to lowercase before matching.
Changes made:
isCatalogTypeEnabled()to use.some()with case-insensitive comparison instead of.includes()useGetAllDisabledSubCatalogs()to use case-insensitive filteringScreenshots / screen recording:
Before:
developerCatalog.types.enabled: ["Operator"]→ Operators category missingAfter:
developerCatalog.types.enabled: ["Operator"]→ Operators category displays correctlyUser can now use any of these and they all work:
["operator"]✅["Operator"]✅ (newly fixed)["OPERATOR"]✅ (newly fixed)Test setup:
oc edit console.operator.openshift.iodeveloperCatalog.types.state: Enabledwithenabled: ["Operator"](capitalized)Test cases:
enabled: ["Operator"]now displays Operators categoryenabled: ["operator"]continues to workenabled: ["OPERATOR"]now worksenabled: ["HelmChart", "Template", "Operator"]all display correctlydisabled: ["Operator"]hides Operators regardless of capitalizationBrowser conformance:
Additional info:
Summary by CodeRabbit
Bug Fixes
Tests