CONSOLE-4976: Add ClusterExtension creation form and unit tests #15955
Conversation
- Create ClusterExtensionForm with form/YAML view toggle - Add SchemaFieldHelp component to display API schema documentation - Add TextInputModal as reusable generic modal in console-shared - Implement UI-only state for auto-generated namespace and service account names - Add comprehensive field help descriptions with semantic versioning syntax examples - Add ServiceAccountDropdown with create option - Support auto-detection of existing namespaces and service accounts 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add TextInputModal.spec.tsx with 12 tests for modal validation and behavior - Add SchemaFieldHelp.spec.tsx with 5 tests for schema description rendering - Add ClusterExtensionForm.spec.tsx with 14 tests covering: - Form field rendering and initialization - Auto-generation of namespace and service account names - Radio button toggling between create new and select from cluster - Channel and catalog label management - Version and package name updates - Pencil icon editing for auto-generated names All 31 unit tests passing 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@TheRealJon: This pull request references CONSOLE-4976 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.22.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. |
openshift-ci
bot
added
component/core
|
@TheRealJon: This pull request references CONSOLE-4976 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.22.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. |
📝 WalkthroughWalkthroughThis pull request introduces UI components and hooks for managing ClusterExtension creation in the Operator Lifecycle Manager v1 package. New reusable components include TextInputModal for validated text input, SchemaFieldHelp for displaying property descriptions, ClusterExtensionForm for form-based resource creation, and modals for creating ServiceAccounts. The refactored CreateClusterExtension component now uses SyncedEditor to coordinate form and YAML editing. Supporting hooks enable modal launching, and localization keys are added for form fields, help text, and error messages. SyncedEditor's context prop becomes optional. Comprehensive test coverage is included. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@frontend/packages/console-shared/src/components/modals/TextInputModal.tsx`:
- Line 3: Replace the deprecated Modal import and composed-props usage with the
PatternFly v6 composable Modal API: change the import to "import { Modal,
ModalHeader, ModalBody, ModalFooter } from '@patternfly/react-core'", remove any
use of ModalVariant (e.g., ModalVariant.small) and instead use the Modal's
size/width props, and refactor the Modal JSX in TextInputModal (where title,
actions or ModalVariant are currently passed) to render
<Modal><ModalHeader>...</ModalHeader><ModalBody>...</ModalBody><ModalFooter>...</ModalFooter></Modal>
so the header, body and footer are explicit and action buttons move into
ModalFooter.
In
`@frontend/packages/operator-lifecycle-manager-v1/src/components/cluster-extension/ClusterExtensionForm.tsx`:
- Around line 418-423: The modal's initialValue is incorrectly using the
original name variable which discards any in-progress edits; change the
textInputModal call in ClusterExtensionForm (the invocation that passes
title/label/initialValue/onSubmit) to set initialValue to the current namespace
variable instead of name so the modal seeds with the current namespace value
(keep onSubmit as handleNamespaceChange).
- Around line 139-165: When the user edits the extension name in
handleNameChange, detect if the existing values for 'spec.namespace' and
'spec.serviceAccount.name' still match the previous auto-generated pattern
(e.g., derived from the old metadata.name); if they do, update them to the new
auto-generated values too by calling updateFormDataPath for 'spec.namespace' and
'spec.serviceAccount.name' (or invoke handleNamespaceChange /
handleServiceAccountNameChange) so the auto-generated namespace/ServiceAccount
stay in sync with the edited name; use the current form data (formData) to
compute whether the fields match the old pattern and to build the new derived
names before updating.
In
`@frontend/packages/operator-lifecycle-manager-v1/src/components/cluster-extension/CreateServiceAccountModal.tsx`:
- Around line 109-131: The Name field uses a nested FormGroup and neither
FormGroup includes fieldId, breaking label-input association; in
CreateServiceAccountModal update the Name block to use a single FormGroup with
fieldId="input-name" that wraps the TextInput (id="input-name") and add
fieldId="input-namespace" to the Namespace FormGroup to match the TextInput
(id="input-namespace"); ensure you leave submit, setName, namespace, and the
TextInput props unchanged while only adjusting FormGroup usage and adding the
fieldId attributes.
🧹 Nitpick comments (4)
frontend/packages/console-shared/src/components/modals/__tests__/TextInputModal.spec.tsx (1)
36-44: Solid test coverage—consider adding keyboard submission test.The suite covers the critical paths well: validation, error states, custom validators, and button interactions.
One optional enhancement: since the modal wraps inputs in a
<form>, users may expect to submit via Enter key. Consider adding a test for keyboard submission (e.g.,fireEvent.submit(form)or pressing Enter on the input).frontend/packages/console-shared/src/components/modals/TextInputModal.tsx (2)
37-55: Consider error boundary for onSubmit failures.If
onSubmitthrows synchronously, the modal won't close but also won't display the error to users. For robustness, consider wrapping in try/catch:♻️ Optional: wrap onSubmit in try/catch
+ try { onSubmit(value); closeModal(); + } catch (err) { + setErrorMessage(String(err)); + }
87-114: A11y consideration: link error to input for screen readers.The error Alert is visually associated but not programmatically linked to the input. For better screen reader support, consider using PatternFly's FormGroup
validatedandhelperTextInvalidprops, or addaria-describedbyto the input referencing the error's id.This is optional but aligns with WCAG 3.3.1 (Error Identification).
frontend/packages/operator-lifecycle-manager-v1/src/components/cluster-extension/ClusterExtensionForm.tsx (1)
73-86: UseCATALOG_LABEL_KEYwhen parsing selectors.Parsing uses a hard-coded label key string while updates use
CATALOG_LABEL_KEY. This risks drift if the constant changes.♻️ Consistency tweak
- const catalogLabel = selector.matchLabels?.['olm.operatorframework.io/metadata.name']; + const catalogLabel = selector.matchLabels?.[CATALOG_LABEL_KEY]; ... - expr.key === 'olm.operatorframework.io/metadata.name' && expr.operator === 'In', + expr.key === CATALOG_LABEL_KEY && expr.operator === 'In',
frontend/packages/console-shared/src/components/modals/TextInputModal.tsx
Outdated
Show resolved
Hide resolved
...ages/operator-lifecycle-manager-v1/src/components/cluster-extension/ClusterExtensionForm.tsx
Show resolved
Hide resolved
...ages/operator-lifecycle-manager-v1/src/components/cluster-extension/ClusterExtensionForm.tsx
Show resolved
Hide resolved
...operator-lifecycle-manager-v1/src/components/cluster-extension/CreateServiceAccountModal.tsx
Outdated
Show resolved
Hide resolved
|
/retest |
|
QE Approver Docs Approver: PX Approver: |
|
@TheRealJon: GitHub didn't allow me to assign the following users: jseseCCS. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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. |
frontend/packages/console-shared/src/components/modals/TextInputModal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/modals/TextInputModal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/hooks/useTextInputModal.ts
Outdated
Show resolved
Hide resolved
...operator-lifecycle-manager-v1/src/components/cluster-extension/CreateServiceAccountModal.tsx
Outdated
Show resolved
Hide resolved
...operator-lifecycle-manager-v1/src/components/cluster-extension/CreateServiceAccountModal.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/operator-lifecycle-manager-v1/src/hooks/useCreateServiceAccountModal.ts
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/modals/TextInputModal.tsx
Outdated
Show resolved
Hide resolved
|
/assign @XiyunZhao |
Address PR feedback by migrating TextInputModal and CreateServiceAccountModal to use non-deprecated PatternFly v6 Modal components and the useOverlay hook. Changes: - Replace deprecated Modal with composable Modal API (ModalHeader, ModalBody, ModalFooter) - Change from ModalComponent to OverlayComponent - Replace useModal with useOverlay hook - Wrap submit handlers in useCallback for performance - Use TextInputProps['type'] instead of hardcoded union type - Replace className div with FormHelperText components - Add fieldId to FormGroups in CreateServiceAccountModal - Update tests to use closeOverlay instead of closeModal 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@TheRealJon: This pull request references CONSOLE-4976 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.22.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. |
|
@TheRealJon: This pull request references CONSOLE-4976 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.22.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. |
frontend/packages/console-shared/src/components/modals/TextInputModal.tsx
Outdated
Show resolved
Hide resolved
…nputModal Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Hi @TheRealJon , I have a few questions and would appreciate your help confirming them:
|
|
Thanks for pointing these out @XiyunZhao ! |
…reation logic Make TextInputModal's required validation configurable via isRequired prop. Rename auto-creation state variables for clarity and add explicit useEffect hooks to manage namespace/service account auto-creation behavior. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@XiyunZhao Thank you for the detailed verification! Looks like I missed a few edge cases!
|
|
@TheRealJon Thanks for the quick fix — the changes look good to me, and will wait @kevinhatchoua confirm |
Update page title and help text to use "Create" instead of "Install" for consistency with the form action. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
TheRealJon
force-pushed
the
CONSOLE-4976
branch
from
139c76d to
6e3da28
Compare
|
@XiyunZhao I actually just removed the placeholder values since we don't use them in most other forms. Please take another look and let me know if there's any other issues! |
|
@TheRealJon Reviewed all new/updated user-facing strings across ClusterExtension creation flow (form, YAML view, service account modal, dropdown actions). Suggested minor wording updates for clarity, consistency, user-facing terminology. Everything else looks aligned w/existing console patterns/shared strings. |
| "Actions": "Actions", | ||
| "An error occurred": "An error occurred", | ||
| "Note: Some fields may not be represented in this form view. Please select \"YAML view\" for full control.": "Note: Some fields may not be represented in this form view. Please select \"YAML view\" for full control.", | ||
| "Some fields might not be represented in this form view. Select \"YAML view\" for full control.": "Some fields might not be represented in this form view. Select \"YAML view\" for full control.", |
There was a problem hiding this comment.
Suggest revising for clarity, task focus, style consistency. This removes vague wording (“represented”, “full control”), uses direct user-task language, bolds the UI element per style guidance.
suggested change:
Some fields might not display in this form. Select YAML view to edit all fields.
| @@ -96,6 +96,7 @@ | |||
| "Select {{title}}": "Select {{title}}", | |||
| "A form is not available for this resource. Please use the YAML view.": "A form is not available for this resource. Please use the YAML view.", | |||
There was a problem hiding this comment.
global: IBM Style says no "please."
rationale: please - adverb
Do not use in IBM (+ Red Hat) technical information or in instructions. Terms of politeness are superfluous, convey the wrong tone for technical material, and are not regarded the same way in all cultures. In marketing information, terms of politeness might be appropriate.
There was a problem hiding this comment.
Could this be addressed separately since it's not a part of the current diff?
| isInline | ||
| title={t( | ||
| 'console-shared~Note: Some fields may not be represented in this form view. Please select "YAML view" for full control.', | ||
| 'console-shared~Some fields might not be represented in this form view. Select "YAML view" for full control.', |
There was a problem hiding this comment.
Line 12: Suggest “Some fields might not be displayed in this form. Select YAML view to edit all fields.”
...clearer, task-focused wording; aligns w/updated console messaging and style guidance (bold UI elements).
There was a problem hiding this comment.
This is a large new user-facing form, so I’m focusing review on visible labels, help text, validation, button text, and error messaging. Please make sure all user-facing strings follow console conventions: clear task-focused language, consistent terminology across flow, no internal-only phrasing, and reuse of existing shared strings where applicable. Also confirm that required-field errors, empty states, inline help are consistent with existing console patterns.
| <PageHeading | ||
| title={t('olm-v1~Create ClusterExtension')} | ||
| helpText={t( | ||
| 'olm-v1~Create a ClusterExtension to add functionality to your cluster. ClusterExtensions are managed by Operator Lifecycle Manager v1.', |
There was a problem hiding this comment.
Line 68–70: Suggest revising to "Create a cluster extension to add functionality to your cluster. Operator Lifecycle Manager manages cluster extensions." to avoid repeated use of “ClusterExtension” as proper noun in explanatory copy. Using sentence-style phrasing (“cluster extension”) improves readability while preserving technical meaning.
There was a problem hiding this comment.
Per our discussion, I'll keep the kind-style references intact for consistency and focus on other changes.
|
|
||
| return ( | ||
| <Modal variant="small" isOpen onClose={closeOverlay}> | ||
| <ModalHeader title={t('olm-v1~Create ServiceAccount')} /> |
There was a problem hiding this comment.
Line 89: Suggest changing modal title to “Create service account” to keep UI user-focused, avoid CamelCase kind-style terminology in primary modal title.
| const actionItems = namespace | ||
| ? [ | ||
| { | ||
| actionTitle: t('olm-v1~Create ServiceAccount'), |
There was a problem hiding this comment.
Line 26: Suggest changing action label to “Create service account” bc “ServiceAccount” reads like internal kind name and this is user-visible UI text. This also keeps dropdown action consistent w/modal title.
| "Select {{title}}": "Select {{title}}", | ||
| "A form is not available for this resource. Please use the YAML view.": "A form is not available for this resource. Please use the YAML view.", | ||
| "There is some issue in this form view. Please select \"YAML view\" for full control.": "There is some issue in this form view. Please select \"YAML view\" for full control.", | ||
| "Note: Some fields may not be represented in this form view. Please select \"YAML view\" for full control.": "Note: Some fields may not be represented in this form view. Please select \"YAML view\" for full control.", |
There was a problem hiding this comment.
suggested change: Some fields might not be displayed in this form. Select YAML view to edit all fields.
...to remove vague error language, clarify user action, align w/style guidance.
openshift-ci
bot
added
the
docs-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jseseCCS, TheRealJon 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 |
TheRealJon
force-pushed
the
CONSOLE-4976
branch
from
218f447 to
df6434a
Compare
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
TheRealJon
force-pushed
the
CONSOLE-4976
branch
from
df6434a to
7b80f86
Compare
|
@TheRealJon new issues were found:
|
… changes - Update TextInputModal tests for isRequired prop behavior - Update ClusterExtensionForm tests for new auto-create wording - Add tests for Create/Cancel button text - Improve validation retry test coverage Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add CreateClusterExtension tests verifying page title, help text, and URL parameter initialization - Add SwitchToYAMLAlert tests for rendering and close button behavior - Tests confirm "Create" wording (not "Install") is used correctly Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ount fields Track auto-generated namespace and service account names in separate state to enable debounced updates (500ms) when the name field changes. Values are retained when switching between "Automatically create" and "Select from cluster" modes, improving UX by preventing data loss during form interaction. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Hi @XiyunZhao! Thanks for another detailed review! I've pushed some changes to address what you found. I wasn't able to reproduce (1) before or after my changes, and I've fixed (2). PTAL when you have a chance, thanks! |
|
@TheRealJon: all tests passed! 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. |
|
/verified by @XiyunZhao |
openshift-ci-robot
added
the
verified
|
@XiyunZhao: 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. |
|
@logonoff Could you give this one final tag? |
logonoff
left a comment
logonoff
left a comment
There was a problem hiding this comment.
some more nitpicks
few differences when compared to existing forms:
(existing form for reference)
- no way to dismiss "some fields might not be displayed in this form"
- form takes up full width of the screen, which makes dropdown menus very long
- tooltip content can be horizontally scrolled on which feels a little weird
- Tooltips gets cut off on first open in specific scroll positions, but then scrolling it fixes that
| export * from './useCopyLoginCommands'; | ||
| export * from './useQuickStartContext'; | ||
| export * from './useUser'; | ||
| export * from './useTextInputModal'; |
There was a problem hiding this comment.
no new additions to console-shared barrel files please
| export { default as ConsolePluginRadioInputs } from './ConsolePluginRadioInputs'; | ||
| export { default as ConsolePluginWarning } from './ConsolePluginWarning'; | ||
| export { default as FallbackImg } from './FallbackImg'; | ||
| export { SchemaFieldHelp } from './SchemaFieldHelp'; |
There was a problem hiding this comment.
no new additions to console-shared barrel files please
| > | ||
| {t('olm-v1~Cancel')} | ||
| </Button> | ||
| {inProgress && <LoadingInline />} |
There was a problem hiding this comment.
there is an isLoading variant of Button we should use here -- appending LoadingInline at the end is the old pattern
| <Button | ||
| variant="plain" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| textInputModal({ | ||
| title: t('olm-v1~Edit Namespace Name'), | ||
| label: t('olm-v1~Namespace'), | ||
| initialValue: autoGeneratedNamespace || name, | ||
| onSubmit: (value) => { | ||
| setAutoGeneratedNamespace(value); | ||
| }, |
There was a problem hiding this comment.
i'm able to set the clear the value and click save, maybe we can disable the button in this case?
Screencast.From.2026-02-10.11-36-40.mp4
| <span | ||
| style={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| gap: '0.5rem', | ||
| }} |
There was a problem hiding this comment.
I'd prefer PF flex here if possible
| <span | ||
| style={{ | ||
| display: 'flex', | ||
| alignItems: 'center', | ||
| gap: '0.5rem', | ||
| }} | ||
| > | ||
| <span> | ||
| {t('olm-v1~Automatically create a new ServiceAccount: ')} | ||
| <strong>{autoGeneratedServiceAccountName}</strong> | ||
| </span> | ||
| {autoCreateServiceAccount && ( | ||
| <Button | ||
| variant="plain" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| textInputModal({ | ||
| title: t('olm-v1~Edit ServiceAccount Name'), | ||
| label: t('olm-v1~Service Account Name'), | ||
| initialValue: autoGeneratedServiceAccountName, | ||
| onSubmit: (value) => { | ||
| setAutoGeneratedServiceAccountName(value); | ||
| handleServiceAccountNameChange(value); | ||
| }, | ||
| }); | ||
| }} | ||
| aria-label={t('olm-v1~Edit service account name')} | ||
| style={{ | ||
| padding: '0', | ||
| minWidth: 'auto', | ||
| }} | ||
| > | ||
| <PencilAltIcon /> | ||
| </Button> |
There was a problem hiding this comment.
feels like we can extract the flexbox and button into a component since we are duplicating a lot of this from the namespace field
| <TextInputGroupMain | ||
| value={channelInputValue} | ||
| onChange={handleChannelInputChange} | ||
| onKeyDown={handleChannelKeyDown} | ||
| id="channel" | ||
| aria-label={t('olm-v1~Channels')} | ||
| > | ||
| <LabelGroup> | ||
| {channels.map((ch: string) => ( | ||
| <Label key={ch} onClose={() => handleChannelRemove(ch)} isCompact> | ||
| {ch} | ||
| </Label> | ||
| ))} | ||
| </LabelGroup> | ||
| </TextInputGroupMain> |
There was a problem hiding this comment.
I am able to put spaces in channels (when it is not allowed), and I also found it a little unintuitive that you have to press enter in order to submit a label.
Can we change the logic such that if you press space, it creates a new label?
| <FormGroup label={label} isRequired={isRequired} fieldId="input-value"> | ||
| <TextInput | ||
| id="input-value" | ||
| data-test="input-value" |
There was a problem hiding this comment.
input-value is kind of a generic id, can we do something more specific like "text-input-modal-value"?
8 participants
Summary
Adds ClusterExtension creation form with comprehensive unit test coverage. The form supports auto-generated namespace and service account configuration, with the ability to edit names via modal, select existing resources from cluster, and configure version constraints, channels, and catalogs.
Components Added
ClusterExtensionForm: Main form component for creating ClusterExtensions
TextInputModal: Reusable modal for editing text values
CreateServiceAccountModal: Modal for creating ServiceAccounts
SchemaFieldHelp: Popover help icon displaying API schema descriptions with fallback support
Unit Tests (31 total)
🤖 Generated with Claude Code