OSAC-2936: add catalog item create wizard for Cluster, VM, and Bare Metal - #102
Open
ElayAharoni wants to merge 33 commits into
Open
OSAC-2936: add catalog item create wizard for Cluster, VM, and Bare Metal#102ElayAharoni wants to merge 33 commits into
ElayAharoni wants to merge 33 commits into
Conversation
|
@ElayAharoni: This pull request references OSAC-2936 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 "5.0.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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ElayAharoni 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 |
|
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:
WalkthroughAdds routed catalog creation wizards for cluster, compute instance, and bare metal items, with role-aware API hooks, Formik field editors, validation, protobuf serialization, transport registry wiring, translations, and tests. ChangesCatalog creation flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 16
🤖 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 `@apps/app-frontend/src/shell/AdminCatalogRoutes.test.tsx`:
- Around line 1-5: Update the test module’s React imports and the ListPage props
typing: import ReactNode from react, then replace the React.ReactNode reference
in ListPage with the imported ReactNode type.
In `@libs/ui-components/src/api/v1/baremetal-instance-templates.test.ts`:
- Around line 19-41: Extract the duplicated hook test harnesses into
libs/ui-components/src/api/v1/test-utils.tsx, exporting the list/admin
renderWithTransport variant with optional role and the mutation variant with
required role. In
libs/ui-components/src/api/v1/baremetal-instance-templates.test.ts (lines
19-41), cluster-templates.test.ts (lines 16-38), and
compute-instance-templates.test.ts (lines 19-41), remove local harnesses and
import the shared list/admin helper; do the same with the mutation helper in
baremetal-instance.test.ts (lines 16-38), cluster-catalog-item.test.ts (lines
16-38), and compute-instance-catalog-item.test.ts (lines 16-38). Replace the
inline wrapper in projects.test.ts (lines 19-25) with the shared no-role helper.
In
`@libs/ui-components/src/components/catalogManagement/CatalogItemGeneralFields.tsx`:
- Around line 26-35: Normalize the initial scope level for non-providerAdmin
roles so it matches an available option: update the wizard pages’ scope.level
initialization or the scopeOptions handling in CatalogItemGeneralFields to use
organization instead of general when general is not offered. Preserve general
for providerAdmin and ensure the radio group always has a selected valid scope.
In
`@libs/ui-components/src/components/catalogManagement/fieldDefinitions/BooleanFieldDefinition.tsx`:
- Around line 12-32: Create a shared field-definition wrapper for the repeated
FormFieldGroup/FormFieldGroupHeader layout, fieldDefinitions.${path} naming, and
Editable SwitchField, accepting label, fieldId, path, and children. Update
BooleanFieldDefinition.tsx lines 12-32 to use it with only the boolean default
SwitchField as children, and update ResourceSelectorFieldDefinition.tsx lines
15-44 to use it with only the SelectField as children; preserve each field’s
existing labels and field names.
In
`@libs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.tsx`:
- Around line 51-60: Update NodeSetsFieldEditor to consume the error state
returned by useHostTypes, display an appropriate error message when the
host-types query fails, and prevent the “Add node set” action while the fetch is
unsuccessful. Preserve the existing loading and successful-data behavior.
- Around line 37-43: Update NodeSetsFormValues to reuse the exported
NodeSetsFieldValue for fieldDefinitions.node_sets instead of defining a narrower
inline object type. Preserve the existing Formik field paths while ensuring
editable, allowAddRemove, sizeMin, sizeMax, and entries remain fully typed.
- Around line 122-131: Update the remove button rendered in the node-set row
actions to generate a unique aria-label for each row, incorporating its
row-specific identifier such as rowIndex or the node set name, while preserving
the existing conditional rendering and removeRow(rowIndex) behavior.
In `@libs/ui-components/src/components/catalogProvision/protobuf-value.test.ts`:
- Around line 37-51: Add a composite round-trip case to the
“plainToProtobufValue / protobufValueToPlain round trip” tests that nests arrays
and objects together, such as an object containing an array of objects, and
assert the converted result equals the original value.
In
`@libs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.tsx`:
- Around line 216-233: Update the payload construction in the
createBareMetalInstanceCatalogItem flow to assign the object literal to a typed
MessageInitShape variable before calling the function, removing the blanket
Parameters<typeof createBareMetalInstanceCatalogItem>[0] assertion. Let
TypeScript validate fields such as template, tenant, and metadata.project, and
retain only a narrowly scoped cast for any individual property that genuinely
requires it.
- Around line 149-165: The handleNextOrSubmit flow in
libs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.tsx
(lines 149-165) must validate all wizard fields before submitting, using a
full-form schema composed from all three step schemas; retain the active step
schema for Next navigation. Apply the same change in
libs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsx
(lines 191-207) so final submission cannot bypass earlier-step validation.
In `@libs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.tsx`:
- Around line 121-124: Update the 'configuration' schema and
buildNodeSetsDefault so node_sets.entries must contain at least one row with a
non-empty hostType and positive size, and duplicate host types are rejected
rather than overwritten. Remove the silent continue/overwrite behavior in
buildNodeSetsDefault, ensuring invalid or duplicate rows produce validation
feedback and valid rows are preserved in node_sets.default.
- Around line 82-84: Update CIDR_PATTERN to validate IPv4 octets within 0–255
and the prefix length within 0–32, rather than only limiting digit counts. Keep
the existing CIDR format and SSH_PUBLIC_KEY_PATTERN unchanged.
In
`@libs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.test.tsx`:
- Around line 74-85: Update the submission assertions in the create-instance
test to inspect serialized field-definition defaults, not only their paths.
Assert that a ref-backed field such as instance_type uses the expected
serialized default payload and that at least one scalar field has its expected
default value, while retaining the existing path assertion.
In
`@libs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsx`:
- Around line 164-231: Extract the duplicated
ComputeInstanceCatalogItemWizardFooter and
BareMetalInstanceCatalogItemWizardFooter into a generic
CatalogItemWizardFooter<TStepId> under components/catalogManagement/, preserving
their existing navigation, validation, pending-state, and submit behavior;
update both create pages to consume it. Move ScopeValues and the shared
role-based scope-to-payload mapping into a common helper, and replace both local
implementations with that helper while preserving each page’s existing behavior
and correcting any inconsistent run_strategy default casing.
- Line 129: Update the instance_type argument in buildFieldDefinition to pass
the underlying reference value (values.fieldDefinitions.instance_type.value)
rather than the full labeled resource-ref object, matching the flattening used
for values.template.value before serialization.
In `@libs/ui-components/src/utils/slug.ts`:
- Around line 4-17: Update slugify to Unicode-normalize input and remove
combining diacritic marks before generating the base slug, preserving the
existing trimming, prefixing, length, and fallback behavior. Also update the
three create-page call sites that derive metadata.name from slugify to append a
short random suffix, ensuring titles that produce the same slug remain unique.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 965e4ed6-811a-43d4-ba27-4263b4dcc04a
📒 Files selected for processing (59)
apps/app-frontend/src/shell/AdminCatalogRoutes.test.tsxapps/app-frontend/src/shell/AdminCatalogRoutes.tsxlibs/i18n/locales/en/translation.jsonlibs/types/src/index.tslibs/ui-components/src/api/types.tslibs/ui-components/src/api/v1/baremetal-instance-templates.test.tslibs/ui-components/src/api/v1/baremetal-instance-templates.tslibs/ui-components/src/api/v1/baremetal-instance.test.tslibs/ui-components/src/api/v1/baremetal-instance.tslibs/ui-components/src/api/v1/cluster-catalog-item.test.tslibs/ui-components/src/api/v1/cluster-catalog-item.tslibs/ui-components/src/api/v1/cluster-templates.test.tslibs/ui-components/src/api/v1/cluster-templates.tslibs/ui-components/src/api/v1/compute-instance-catalog-item.test.tslibs/ui-components/src/api/v1/compute-instance-catalog-item.tslibs/ui-components/src/api/v1/compute-instance-templates.test.tslibs/ui-components/src/api/v1/compute-instance-templates.tslibs/ui-components/src/api/v1/projects.test.tslibs/ui-components/src/api/v1/projects.tslibs/ui-components/src/components/Form/SwitchField.test.tsxlibs/ui-components/src/components/Form/SwitchField.tsxlibs/ui-components/src/components/catalogManagement/CatalogItemGeneralFields.test.tsxlibs/ui-components/src/components/catalogManagement/CatalogItemGeneralFields.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/BooleanFieldDefinition.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/BooleanFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NumberFieldDefinition.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NumberFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/ResourceSelectorFieldDefinition.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/ResourceSelectorFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/StringFieldDefinition.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/StringFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.test.tslibs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.tslibs/ui-components/src/components/catalogManagement/steps/baremetal-instance/BMAccessStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/baremetal-instance/BMAccessStep.tsxlibs/ui-components/src/components/catalogManagement/steps/baremetal-instance/BMConfigurationStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/baremetal-instance/BMConfigurationStep.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterAccessStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterAccessStep.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterConfigurationStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterConfigurationStep.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterNetworkingStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterNetworkingStep.tsxlibs/ui-components/src/components/catalogManagement/steps/compute-instance/VMAccessStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/compute-instance/VMAccessStep.tsxlibs/ui-components/src/components/catalogManagement/steps/compute-instance/VMConfigurationStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/compute-instance/VMConfigurationStep.tsxlibs/ui-components/src/components/catalogProvision/protobuf-value.test.tslibs/ui-components/src/components/catalogProvision/protobuf-value.tslibs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.test.tsxlibs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.tsxlibs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.test.tsxlibs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.tsxlibs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.test.tsxlibs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsxlibs/ui-components/src/utils/slug.test.tslibs/ui-components/src/utils/slug.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
libs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.tsx (1)
104-108: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftPersist
allowAddRemoveor remove this control.This editable Formik value is not serialized into the node-set payload, so admins can change it but the submitted catalog item silently drops the setting. Serialize it before release, or hide the control until the wire contract supports it. The PR objective explicitly lists this serialization as deferred.
🤖 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 `@libs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.tsx` around lines 104 - 108, Update the form submission and node-set payload serialization flow associated with NodeSetsFieldEditor and NODE_SETS_NAME so the allowAddRemove value is persisted in the submitted catalog item; if the wire contract cannot support it in this change, remove or hide the SwitchField control instead.libs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.ts (1)
23-32: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftValidate field-specific validation metadata before serializing it.
The shared schema checks only
editableanddefault, whileStringFieldDefinitionandNumberFieldDefinitionemitvalidation.pattern,minimum, andmaximum;buildFieldDefinitionthen JSON-stringifies them unchanged. Invalid regexes or non-finite/inverted numeric bounds can be persisted, and pathological regexes can cause ReDoS in a downstream backtracking evaluator. Add per-kind validation, including a safe regex policy andminimum <= maximum, and enforce the same contract server-side.Also applies to: 46-67
🤖 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 `@libs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.ts` around lines 23 - 32, Update buildFieldDefinition and the StringFieldDefinition/NumberFieldDefinition validation schemas to validate kind-specific metadata before JSON serialization: enforce a safe, compilable regex policy for validation.pattern and finite numeric minimum/maximum values with minimum no greater than maximum. Apply the identical validation contract server-side wherever these field definitions are accepted, while preserving existing editable/default validation.Source: Path instructions
libs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.test.tsx (1)
97-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the serialized
node_setsdefault, not justpublished/title.
fillFirstNodeSetis the only thing exercisingbuildNodeSetsDefault, yet nothing checks what it produced. A regression that drops the row or keys it by label instead of host-type id would still pass. Assert thenode_setsfield definition's default contains thesmall/size-3 entry. The new duplicate-host-type rule is also untested.🤖 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 `@libs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.test.tsx` around lines 97 - 115, Update the test around renderPage and the createFn request assertion to inspect the serialized node_sets field definition default produced by buildNodeSetsDefault, verifying it contains the small host-type entry with size 3. Also add coverage for the duplicate-host-type rule by exercising duplicate input and asserting the expected behavior.libs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsx (1)
97-97: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the correct
baremetal_instancerun_strategy enum value in this catalog default.BareMetalInstanceRunStrategy.ALWAYSserializes asAlways, so...default: 'ALWAYS'can create malformed bare-metal catalog items. Update this (and the matching tests) to use the canonicalAlwaysstring unless the backend intentionally uses the enum text.🤖 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 `@libs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsx` at line 97, Update the run_strategy defaults in ComputeInstanceCatalogItemCreatePage.tsx:97 and BareMetalInstanceCatalogItemCreatePage.tsx:79 to use the canonical serialized value Always rather than ALWAYS. Update the matching tests to assert Always, preserving the editable/default structure and backend enum contract.
🤖 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
`@libs/ui-components/src/components/catalogManagement/CatalogItemWizardFooter.tsx`:
- Around line 30-41: The isLast calculation in CatalogItemWizardFooter must not
compare activeStep.index with stepIds.length, since those values can diverge
when the wizard contains additional steps. Derive the active position using
stepIds.indexOf(activeStepId), then determine isLast from that position reaching
the final step while preserving the existing first-step behavior.
- Around line 65-75: Update the full-form validation flow around
fullFormSchema.isValid in CatalogItemWizardFooter so validation failures
populate Formik’s field errors using the schema’s validation details, rather
than only setting the generic validation alert. Ensure the user can identify and
navigate to the offending field or its owning step, while preserving the
existing submit path for valid forms.
In
`@libs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.test.ts`:
- Around line 83-108: Add regression tests alongside the existing
fieldDefinitionValueSchema format tests covering malformed regex patterns,
nonnumeric minimum/maximum bounds, and configurations where minimum exceeds
maximum. Assert each invalid validation metadata configuration is rejected with
the appropriate validation error.
In `@libs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.tsx`:
- Around line 80-85: Unify the CIDR validation used by isValidCidr and the
tenant-facing validation.pattern so both accept and reject exactly the same
inputs, including rejecting leading-zero octets such as 010.0.0.0/8. Prefer
deriving the pattern from the shared validation rule; otherwise validate both
against the same fixture set before allowing an admin default to be saved.
In
`@libs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsx`:
- Around line 124-133: Update the configuration-step and full-form schemas in
libs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsx
(lines 124-133) to validate additional_disks as an array of objects whose
size_gib uses fieldDefinitionValueSchema(t). Update both corresponding schemas
in
libs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.tsx
(lines 85-108) to include user_data with fieldDefinitionValueSchema(t). Keep the
existing step behavior unchanged; the longer-term schema composition refactor is
out of scope.
---
Outside diff comments:
In
`@libs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.ts`:
- Around line 23-32: Update buildFieldDefinition and the
StringFieldDefinition/NumberFieldDefinition validation schemas to validate
kind-specific metadata before JSON serialization: enforce a safe, compilable
regex policy for validation.pattern and finite numeric minimum/maximum values
with minimum no greater than maximum. Apply the identical validation contract
server-side wherever these field definitions are accepted, while preserving
existing editable/default validation.
In
`@libs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.tsx`:
- Around line 104-108: Update the form submission and node-set payload
serialization flow associated with NodeSetsFieldEditor and NODE_SETS_NAME so the
allowAddRemove value is persisted in the submitted catalog item; if the wire
contract cannot support it in this change, remove or hide the SwitchField
control instead.
In
`@libs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.test.tsx`:
- Around line 97-115: Update the test around renderPage and the createFn request
assertion to inspect the serialized node_sets field definition default produced
by buildNodeSetsDefault, verifying it contains the small host-type entry with
size 3. Also add coverage for the duplicate-host-type rule by exercising
duplicate input and asserting the expected behavior.
In
`@libs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsx`:
- Line 97: Update the run_strategy defaults in
ComputeInstanceCatalogItemCreatePage.tsx:97 and
BareMetalInstanceCatalogItemCreatePage.tsx:79 to use the canonical serialized
value Always rather than ALWAYS. Update the matching tests to assert Always,
preserving the editable/default structure and backend enum contract.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 109c3c1e-f438-415f-b892-3eead363058c
📒 Files selected for processing (29)
apps/app-frontend/src/shell/AdminCatalogRoutes.test.tsxlibs/ui-components/src/api/v1/baremetal-instance-templates.test.tslibs/ui-components/src/api/v1/baremetal-instance.test.tslibs/ui-components/src/api/v1/cluster-catalog-item.test.tslibs/ui-components/src/api/v1/cluster-templates.test.tslibs/ui-components/src/api/v1/compute-instance-catalog-item.test.tslibs/ui-components/src/api/v1/compute-instance-templates.test.tslibs/ui-components/src/components/catalogManagement/CatalogItemWizardFooter.test.tsxlibs/ui-components/src/components/catalogManagement/CatalogItemWizardFooter.tsxlibs/ui-components/src/components/catalogManagement/catalogItemScope.test.tslibs/ui-components/src/components/catalogManagement/catalogItemScope.tslibs/ui-components/src/components/catalogManagement/fieldDefinitions/BooleanFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/FieldDefinitionGroup.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NumberFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/ResourceSelectorFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/StringFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.test.tslibs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.tslibs/ui-components/src/components/catalogProvision/protobuf-value.test.tslibs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.tsxlibs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.test.tsxlibs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.tsxlibs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.test.tsxlibs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsxlibs/ui-components/src/test-utils/renderHookWithTransport.tsxlibs/ui-components/src/utils/slug.test.tslibs/ui-components/src/utils/slug.ts
Contributor
|
Seems node sets requirements changed, host types should match only what's in the template, this effects the design and implementation |
14 tasks
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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
`@libs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.ts`:
- Around line 41-48: Update isValidRegexPattern to reject unsafe or overly
complex regex patterns before validation.pattern is serialized into
validationSchema; use the project’s safe-regex validation mechanism or enforce
the established length/complexity limits. Preserve acceptance of valid safe
patterns and ensure only bounded patterns reach tenant catalog-value validation.
In
`@libs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.tsx`:
- Around line 111-134: Update the InputField instances rendered by the
templateNodeSetKeys map to use a unique accessible label containing the current
node-set key, while preserving the existing Nodes wording and field names.
Ensure each row’s input is distinguishable to screen readers without relying on
the surrounding FormFieldGroup heading.
In `@libs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.tsx`:
- Around line 130-144: Extend nodeSetsSchema to validate each node set’s sizeMin
and sizeMax relationship, ensuring that when both bounds are provided sizeMin is
less than or equal to sizeMax. Apply the cross-field check in the bespoke
node_sets schema while preserving the existing positive-number validation for
sizeByKey.
- Around line 118-123: Update templateRequiredSchema to validate the selected
value with one trimmed Yup string required check, removing the parent-level
template-selected test. Preserve the existing translated “Template is required”
message while ensuring blank and whitespace-only values produce a single
validation error on the value field.
In `@libs/ui-components/src/validation/cidr-validation.ts`:
- Around line 7-13: Align IPV4_CIDR_PATTERN and isValidCidr whitespace behavior
at their shared validation boundary: ensure surrounding whitespace is
consistently trimmed before validation or consistently rejected by both paths.
Preserve the existing CIDR range and leading-zero rules, and add a regression
fixture covering a CIDR with surrounding spaces.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98f0c4ef-5889-4d0b-b005-235ad789ba6f
📒 Files selected for processing (15)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/catalogManagement/CatalogItemWizardFooter.test.tsxlibs/ui-components/src/components/catalogManagement/CatalogItemWizardFooter.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.test.tslibs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.tslibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterConfigurationStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterConfigurationStep.tsxlibs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.tsxlibs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.test.tsxlibs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.tsxlibs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsxlibs/ui-components/src/validation/cidr-validation.test.tslibs/ui-components/src/validation/cidr-validation.ts
Contributor
|
add screenshots please |
ElayAharoni
force-pushed
the
OSAC-2936-catalog-item-create-wizard
branch
from
July 28, 2026 07:08
f8df773 to
47a0677
Compare
…ization Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
…tion Signed-off-by: Elay Aharoni <elayaha@gmail.com>
… admin wizard Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
CatalogItemCard, catalogItemSubtitle, and searchableCatalogItemText all treat title as the catalog item's displayed name; metadata.name is a separate identifier. NameField (RFC 1035 DNS-label validation) was the wrong binding for a free-text display name. Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
The public API's Organizations service was removed in favor of Tenants (OSAC-1532), but libs/types/src/index.ts still exported the stale organization_type/organizations_service barrel entries, so the catalog item wizard's tenant-scope lookup 404'd. Switch the wizard to a new useTenants hook backed by the private Tenants service (CSP Admin only, same as the other private lookups it already uses). Also stop polling the wizard's reference-data lookups (templates, projects) via the global 10s refetchInterval default — they only need to be fetched once while the wizard is open, and the interval refetch was visibly reloading the open template dropdown. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@libs/ui-components/src/api/v1/cluster-catalog-item.test.ts`:
- Around line 41-81: Extract the duplicated create-hook role-dispatch tests into
a shared createCatalogCreateHookTests helper, following the existing
createCatalogHookTests pattern. In
libs/ui-components/src/api/v1/cluster-catalog-item.test.ts:41-81, replace the
useCreateClusterCatalogItem describe block with the helper using
useCreateClusterCatalogItem, ClusterCatalogItems, and
PrivateClusterCatalogItems; make the equivalent replacements in
libs/ui-components/src/api/v1/baremetal-instance.test.ts:53-93 using
useCreateBareMetalInstanceCatalogItem, BareMetalInstanceCatalogItems, and
PrivateBareMetalInstanceCatalogItems, and
libs/ui-components/src/api/v1/compute-instance-catalog-item.test.ts:44-84 using
useCreateComputeInstanceCatalogItem, ComputeInstanceCatalogItems, and
PrivateComputeInstanceCatalogItems.
In `@libs/ui-components/src/api/v1/cluster-templates.test.ts`:
- Around line 26-58: Extract the duplicated admin-template role-dispatch tests
into a shared createAdminTemplateHookTests helper. In
libs/ui-components/src/api/v1/cluster-templates.test.ts lines 26-58, replace the
useAdminClusterTemplates block and parameterize it with
useAdminClusterTemplates, ClusterTemplates, and PrivateClusterTemplates; apply
the same replacement in
libs/ui-components/src/api/v1/compute-instance-templates.test.ts lines 29-65
using useAdminComputeInstanceTemplates, ComputeInstanceTemplates, and
PrivateComputeInstanceTemplates, preserving the providerAdmin private-client and
tenantAdmin public-client assertions.
In `@libs/ui-components/src/api/v1/tenant.ts`:
- Around line 9-17: Update useTenants to gate the private-tenants query on
provider-admin access, using the available role value or a caller-provided
condition so tenant admins do not issue the request. Preserve the existing
enabled behavior for provider admins and ensure CatalogItemGeneralFields passes
or supplies the role condition if useTenants cannot access it directly.
In
`@libs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.tsx`:
- Around line 85-112: Update getStepValidationSchema and
getFullFormValidationSchema to validate template.value as required and add scope
validation. Restrict scope.level to role-valid values, requiring
scope.tenant.value for organization scope and scope.project.value for project
scope, so buildScopePayloadFields cannot submit an empty or invalid target.
In
`@libs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsx`:
- Around line 104-135: Update both getStepValidationSchema’s configuration case
and getFullFormValidationSchema to validate fieldDefinitions.instance_type by
checking its nested value property, rather than applying
fieldDefinitionValueSchema directly to the {value, label} resource reference.
Ensure an empty instance_type.value is rejected in both step validation and
final submission, matching the flattening behavior in buildFieldDefinitions.
In `@libs/ui-components/src/validation/cidr-validation.test.ts`:
- Around line 32-50: Normalize CIDR input by trimming surrounding whitespace at
the form/serialization boundary before validation and submission, ensuring both
IPV4_CIDR_PATTERN and isValidCidr receive the same normalized value. Update the
IPV4_CIDR_PATTERN fixtures and assertions to cover whitespace-padded CIDRs using
that normalized value while preserving existing parity checks.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f3068ff0-4842-4a3f-9359-58e4f3dca080
📒 Files selected for processing (69)
apps/app-frontend/src/shell/AdminCatalogRoutes.test.tsxapps/app-frontend/src/shell/AdminCatalogRoutes.tsxlibs/i18n/locales/en/translation.jsonlibs/types/src/index.tslibs/ui-components/src/api/types.tslibs/ui-components/src/api/v1/baremetal-instance-templates.test.tslibs/ui-components/src/api/v1/baremetal-instance-templates.tslibs/ui-components/src/api/v1/baremetal-instance.test.tslibs/ui-components/src/api/v1/baremetal-instance.tslibs/ui-components/src/api/v1/cluster-catalog-item.test.tslibs/ui-components/src/api/v1/cluster-catalog-item.tslibs/ui-components/src/api/v1/cluster-templates.test.tslibs/ui-components/src/api/v1/cluster-templates.tslibs/ui-components/src/api/v1/compute-instance-catalog-item.test.tslibs/ui-components/src/api/v1/compute-instance-catalog-item.tslibs/ui-components/src/api/v1/compute-instance-templates.test.tslibs/ui-components/src/api/v1/compute-instance-templates.tslibs/ui-components/src/api/v1/organization.tslibs/ui-components/src/api/v1/projects.test.tslibs/ui-components/src/api/v1/projects.tslibs/ui-components/src/api/v1/tenant.tslibs/ui-components/src/components/Form/SwitchField.test.tsxlibs/ui-components/src/components/Form/SwitchField.tsxlibs/ui-components/src/components/catalogManagement/CatalogItemGeneralFields.test.tsxlibs/ui-components/src/components/catalogManagement/CatalogItemGeneralFields.tsxlibs/ui-components/src/components/catalogManagement/CatalogItemWizardFooter.test.tsxlibs/ui-components/src/components/catalogManagement/CatalogItemWizardFooter.tsxlibs/ui-components/src/components/catalogManagement/catalogItemScope.test.tslibs/ui-components/src/components/catalogManagement/catalogItemScope.tslibs/ui-components/src/components/catalogManagement/fieldDefinitions/BooleanFieldDefinition.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/BooleanFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/FieldDefinitionGroup.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NodeSetsFieldEditor.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NumberFieldDefinition.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/NumberFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/ResourceSelectorFieldDefinition.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/ResourceSelectorFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/StringFieldDefinition.test.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/StringFieldDefinition.tsxlibs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.test.tslibs/ui-components/src/components/catalogManagement/fieldDefinitions/fieldDefinitionValue.tslibs/ui-components/src/components/catalogManagement/steps/baremetal-instance/BMAccessStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/baremetal-instance/BMAccessStep.tsxlibs/ui-components/src/components/catalogManagement/steps/baremetal-instance/BMConfigurationStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/baremetal-instance/BMConfigurationStep.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterAccessStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterAccessStep.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterConfigurationStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterConfigurationStep.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterNetworkingStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/cluster/ClusterNetworkingStep.tsxlibs/ui-components/src/components/catalogManagement/steps/compute-instance/VMAccessStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/compute-instance/VMAccessStep.tsxlibs/ui-components/src/components/catalogManagement/steps/compute-instance/VMConfigurationStep.test.tsxlibs/ui-components/src/components/catalogManagement/steps/compute-instance/VMConfigurationStep.tsxlibs/ui-components/src/components/catalogProvision/protobuf-value.test.tslibs/ui-components/src/components/catalogProvision/protobuf-value.tslibs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.test.tsxlibs/ui-components/src/pages/admin/baremetal-instance/BareMetalInstanceCatalogItemCreatePage.tsxlibs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.test.tsxlibs/ui-components/src/pages/admin/cluster/ClusterCatalogItemCreatePage.tsxlibs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.test.tsxlibs/ui-components/src/pages/admin/compute-instance/ComputeInstanceCatalogItemCreatePage.tsxlibs/ui-components/src/test-utils/renderHookWithTransport.tsxlibs/ui-components/src/utils/slug.test.tslibs/ui-components/src/utils/slug.tslibs/ui-components/src/validation/cidr-validation.test.tslibs/ui-components/src/validation/cidr-validation.ts
💤 Files with no reviewable changes (1)
- libs/ui-components/src/api/v1/organization.ts
rawagner
reviewed
Jul 28, 2026
…decode The Connect transport's JSON codec has no type registry, so any response containing a google.protobuf.Any-packed value (e.g. a ClusterTemplate or ComputeInstanceTemplate parameter's default) throws "... is not in the type registry" while decoding — the request succeeds at the network level, but the query fails client-side, leaving the wizard's template dropdown empty even though the response body is fully populated. BareMetal templates worked only because none of the sampled data happened to have a populated Any default. Register the well-known wrapper, Struct/Value/ListValue, and Timestamp/ Duration types — the full set ClusterTemplateParameterDefinition.default is documented to pack — on the shared transport's jsonOptions.registry. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
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 `@apps/app-frontend/src/wellKnownTypeRegistry.test.ts`:
- Around line 8-18: Add table cases in the well-known type registry test’s
it.each dataset for google.protobuf.BytesValue, FloatValue, UInt32Value, and
UInt64Value, using representative values matching each type’s expected JSON
representation. Keep the existing cases unchanged so every schema registered by
the registry is exercised.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 639f129d-715c-4912-ad03-276f4c1628d9
📒 Files selected for processing (3)
apps/app-frontend/src/main.tsxapps/app-frontend/src/wellKnownTypeRegistry.test.tsapps/app-frontend/src/wellKnownTypeRegistry.ts
Fixes 7 issues found during manual testing of the catalog item creation wizard: - Require selecting a template for VM and Bare Metal catalog items, not just Cluster (shared templateRequiredSchema). - Stop calling the private Tenants API as a Tenant Admin, who has no permission for it — useTenants() is now gated to providerAdmin only. - Show a project's display title instead of its raw id in the project scope selector (Project has a separate spec.title, same split as ClusterTemplate.title vs metadata.name). - Add gap between scope radio buttons (RadioButtonField had none). - Require picking an organization/project once that scope level is selected, via a new scopeValidationSchema — previously the wizard let admins advance with an empty selection. - Wrap every catalogManagement wizard step in the shared OsacForm component, matching the pattern the tenant-user provisioning wizard already uses (proper PatternFly form spacing/width/grid). This also surfaces PatternFly's built-in red required-field asterisk on fields that are now correctly marked isRequired, with no custom CSS needed. Also fixes a duplicate DOM id (RadioButtonField's generated id for the "Project" scope option collided with the project SelectField's fieldId), found while adding test coverage for the project-title fix. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Correction to the earlier fix — projects display their human-entered name directly in metadata.name (unlike templates, which are auto-populated from ansible roles and split display title from metadata.name). spec.title is not the right field here. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
rawagner: stop deriving the catalog item's resource name from its
display title via slugify — show both fields explicitly instead, using
the same buildMetadataNameSchema (RFC 1035 DNS-label validation) the
tenant-provisioning wizard already uses for the equivalent field.
Removes the now-unused slug.ts/slug.test.ts.
Remaining fixes are still-valid CodeRabbit findings verified against
current code (several other CodeRabbit comments were already stale —
fixed independently earlier this session, or resolved as a side effect
of the resource-name change):
- Node-set size inputs all shared the same accessible label ("Nodes");
disambiguate with the node-set key.
- Cluster catalog items' node-set sizeMin/sizeMax were never
cross-validated, so an admin could ship an unsatisfiable size
constraint (min > max) to tenants.
- IPV4_CIDR_PATTERN (shipped verbatim as a tenant-facing validation
constraint) rejected surrounding whitespace that isValidCidr()
already trims, so a value the admin's own wizard accepted could still
fail for a tenant. Aligned both, added a regression fixture.
- VM catalog items' instance_type field was completely unvalidated —
a non-editable instance_type with an empty default silently shipped.
Extracted resourceRefRequiredSchema (generalizing the existing
templateRequiredSchema) to require it only when non-editable.
- wellKnownTypeRegistry's test table didn't exercise every well-known
type the registry actually registers (BytesValue, FloatValue,
UInt32Value, UInt64Value were missing).
A CodeRabbit-suggested test-harness DRY refactor (dedup the
admin-template/create-mutation hook test dispatch pattern across ~9
files) was attempted and reverted: the public/private generated types
are nominally distinct, so a single generic type param broke the
type-safety the existing createCatalogHookTests helper deliberately
preserves — not worth trading for a "Trivial"-tagged nice-to-have.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
- Move "Host type: X" into FormFieldGroupHeader's titleDescription slot
instead of a loose Content paragraph mixed in with the input — this is
the slot PatternFly's own styles already treat as a subtitle (muted
text under the group heading), so it now reads as context for the group
rather than an unstyled aside.
- Group the size-constraint controls (Editable switch, Minimum/Maximum
size) into their own labeled FormFieldGroup ("Size constraints") with a
one-line explanation of what they affect, separating "applies to every
node set" settings from the per-node-set list below.
- Put Minimum/Maximum size side by side via Flex instead of stacked full
width — they're a paired range, and stacking obscured that.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Every Configuration/Access/Networking field ("Release Image", "Cores",
"SSH Public Key", node sets, ...) rendered its FormFieldGroup heading as
plain text — PatternFly's FormFieldGroupHeader doesn't apply any
font-size/weight to titleText by default, so each field's name blended
into the surrounding form instead of reading as a section title. Wrap
each heading in PatternFly's own Title component (h4/md) instead, shared
via FieldDefinitionGroup so every field-definition primitive picks it up
uniformly, plus the two headings in NodeSetsFieldEditor for consistency.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Every other field-definition editor (Release Image, Cores, ...) starts with its own bold heading naming the field, so admins always know what they're configuring. NodeSetsFieldEditor was the exception — it jumped straight into a "Size constraints" heading with nothing identifying that the whole section is the node_sets field, making it read as an unrelated top-level block rather than something scoped to Node Sets. Wrap the whole editor in the same FieldDefinitionGroup scaffold every other field uses, labeled "Node Sets" — this also picks up its "Editable" switch for free, so the one that was manually added inside "Size constraints" is removed (it's a property of the node_sets field as a whole, not just the size bounds). "Size constraints" and the per-node-set groups now render as smaller (h5) sub-headings, visually nested under the "Node Sets" (h4) title instead of looking like peer sections. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Contributor
Author
added snapshots and fixed comments please review @batzionb |
rawagner
reviewed
Jul 29, 2026
These are now fully implied by the selected instance_type (whose own spec already carries cores/memory, shown right in the picker's option label via formatInstanceTypeOptionLabel), so they no longer need to be independently configurable catalog item field definitions. - VMConfigurationStep.tsx: drop the two NumberFieldDefinition fields. - ComputeInstanceCatalogItemCreatePage.tsx: remove cores/memory_gib from the form values interface, initial values, both validation schemas, and buildFieldDefinitions — no ComputeInstanceCatalogItem will ever carry these paths in field_definitions again. - catalogFieldDefinition.ts: drop cores/memory_gib from CATALOG_ITEM_RESOURCE_FIELD_PATHS — these paths can never match a field_definitions entry anymore, so the catalog card resource-summary line naturally drops CPU/memory (boot disk remains). Updated the matching fallback label map and regression test accordingly. Left untouched: instance type's own cores/memoryGib spec fields (vm/utils.ts, unrelated — that's what the picker label already surfaces), VM details-page display of a provisioned instance's actual spec, and the tenant-provisioning wizard's generic field_definitions handling (which must keep working for whatever paths any catalog item defines, using "cores" only as an arbitrary example fixture in tests, not asserting on this specific field). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
…e optional - "Display name" -> "Title" (maps to CatalogItem.title, unchanged: the free-text label shown when browsing the catalog), now optional. - "Resource name" -> "Name" (maps to metadata.name, the RFC 1035 DNS-label resource identifier), remains required — it's the only field that must be unique/valid enough to serve as the object's name. - Added a regression test proving the General step now advances with only Name filled in, no Title. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
… Ref - Remove the is_windows BooleanFieldDefinition from the VM configuration step entirely (deprecated) — dropped from the form values interface, initial values, and buildFieldDefinitions. BooleanFieldDefinition itself is left in place as shared field-definition primitive infrastructure, even though nothing currently uses it. - Rename "Image" to "Source Ref" and correct its path from the top-level scalar `image` to `image.source_ref` — ComputeInstanceSpec.image is actually a nested ComputeInstanceImage message (source_type + source_ref), not a plain string, so the field definition needs to target the specific leaf field the admin is setting. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Contributor
|
Node sets should be a list of items one item per host type in the template |
Contributor
|
This wizard should be consistent with the creation wizard:
|
rawagner: - Drop the CatalogItemCreateRoute :type switch in AdminCatalogRoutes in favor of three explicit static routes. - Move private-only query hooks (tenant, and the admin template hooks' private branch) into api/v1/private/, matching the existing usePrivate* convention for catalog-item hooks. - Drop refetchInterval: false from projects/tenant/template hooks so they inherit the app's default poll interval like every other query. - Convert the ComputeInstance and BareMetal catalog-item wizards from useFormik + FormikProvider to the <Formik> component. The Cluster wizard keeps useFormik + FormikProvider, documented inline: its validationSchema depends on formik.values.template.value, which <Formik>'s render-prop can't expose before the schema is built. batzionb: - Restructure node sets so each template-defined host type carries its own independent default/minimum/maximum, instead of one default per key plus a single global min/max pair applied to all of them. - Align field labels across the three admin catalog-item wizards with the tenant-facing resource wizard's sentence-case vocabulary, and reorder the VM configuration step to match (image before instance type). "Source Ref" is kept as-is per an explicit earlier request in this same work, for a field that's genuinely different from the resource wizard's higher-level "VM image" picker. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
- Drop the "Node set:" prefix from each node-set group header, showing
just the capitalized template key (e.g. "Workers", "Masters").
- Move the host type out of the header's muted description line into a
Label badge in the header actions slot, so it reads as a distinct,
visible attribute of the group rather than blending into the title.
- Drop the redundant "({{key}})" from the default/min/max field labels
now that the group header alone identifies which node set they
belong to.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
A node set always maps to exactly one host type, so showing both a key-based header (e.g. "Workers") and a separate "Host type: Small" badge duplicated the same identity. Collapse the two into a single header that names the host type directly, making it unambiguous which host type each set of default/min/max fields configures. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
3 tasks
|
PR needs rebase. 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OSAC-2936: Shared wizard components and CSP Admin create wizard
Jira: https://redhat.atlassian.net/browse/OSAC-2936
Story type: [UI]
Summary
Adds the catalog item create wizard at
/admin/catalog/:type/createfor CSP Admin and Tenant Admin: kind-specific wizards for Cluster (4 steps), VM (3 steps), and Bare Metal (3 steps), built from four shared field-definition authoring primitives (String/Number/Boolean/ResourceSelector) plus a Cluster-onlyNodeSetsFieldEditor, a shared General-info step, and role-aware data/create hooks for templates and catalog items.Branches off
main(not the still-open OSAC-2932 branch) since there's no formal Jira dependency between the two stories — see the Notes section below for the small number of places this creates temporary divergence from OSAC-2932's in-flight conventions.Changes
Shared field-definition system
StringFieldDefinition,NumberFieldDefinition,BooleanFieldDefinition,ResourceSelectorFieldDefinition— the four primitives every Configuration/Networking/Access field is built fromNodeSetsFieldEditor— Cluster-onlynode_setsmap editor (add/remove rows, host-type de-dup, admin editable/allow-add-remove/size-constraint controls)fieldDefinitionValue.ts—FieldDefinitionValue<T>,buildFieldDefinition(),fieldDefinitionValueSchema()(shared "default required when non-editable" Yup rule)plainToProtobufValue()— write-direction counterpart to the existingprotobufValueToPlain()SwitchField— new shared Formik-bound PatternFlySwitchShared General step
CatalogItemGeneralFields— Name (title), Description, Template dropdown, role-branched Scope (CSP Admin: General/Organization; Tenant Admin: Organization/Project)Per-kind step components
ClusterConfigurationStep,ClusterNetworkingStep,ClusterAccessStepVMConfigurationStep(incl. an additional-disks add/remove editor),VMAccessStepBMConfigurationStep,BMAccessStepAPI hooks
useClusterTemplates,useComputeInstanceTemplates,useBareMetalInstanceTemplates) plus role-awareuseAdmin*TemplatesvariantsuseProjects()(new)useCreate*CatalogItem()mutations for all three kindsBareMetalInstanceTemplates,Project,Projectsfrom the public@osac/typesbarrel — these existed on disk (generated from proto) but were never wired inWizard pages and routing
ClusterCatalogItemCreatePage,ComputeInstanceCatalogItemCreatePage,BareMetalInstanceCatalogItemCreatePage— each wires Formik, step-scoped Yup validation, the create payload, and navigationAdminCatalogRoutes.tsx—:type/createnow dispatches to the correct kind-specific page (was a placeholder<div/>)Testing
NodeSetsFieldEditor,CatalogItemGeneralFieldsfor both roles, all per-kind steps, all API hooks including role-based client selection,slugify(), the three wizard pages, and the routing dispatcher)osac-test-infra, OSAC-2943)Acceptance Criteria
/admin/catalog/:type/createNodeSetsFieldEditorClusterNetworkingStepwithpod_cidr/service_cidr; not shown for VM or Bare Metalssh_public_key/ssh_keyandpull_secret, default editablenetwork_attachmentsin the API payload (not shown in wizard); Bare Metal has no networking fieldspublished: falseby defaultNotes for reviewers
NodeSetsFieldEditor, so this PR builds them in full rather than deferring to OSAC-2937 ("Shared field definition primitives and per-kind step components"). Recommend re-triaging OSAC-2937's remaining scope once this merges.node_sets.allowAddRemoveis captured in the UI/Formik state but not sent in the create payload — the realFieldDefinitionproto message has no field for it (the design doc's own JSON example includes a key that doesn't exist on the wire type). Flagging for the design/API owner.image(VM) andrun_strategy(VM, Bare Metal) are plainStringFieldDefinition(free text) rather than the design's resource-picker/enum widgets — no images-list API exists in this codebase, and the shared primitives don't yet support enum constraints./admin/catalog), not the detail page the design doc describes — that route is still a placeholder, tracked under the OSAC-2929 epic.CatalogItemScopeBadge/CatalogItemStatusLabel/CatalogItemPublishToggleconventions where they overlap.Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
Cluster catalog creation:
nodeSets:
VM catalog creation:
BM catalog creation: