NO-ISSUE: Refactor create vm/cluster/bm wizards - #99
Conversation
|
@rawagner: This pull request explicitly references no jira issue. 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: rawagner 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 |
|
Important Review skippedToo many files! This PR contains 109 files, which is 9 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (166)
You can disable this status message by setting the ✨ 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 |
|
Please update the title and description of the PR, currently seems they don't reflect the scope |
ff5f6bc to
ed4e3a6
Compare
rawagner
changed the title
sorry about that, title/desc updated :) |
|
Now the PRD change is merged |
you are right, we are diverging from the current PRD. however this is based on what BE allows. It currently enforces us to use only the node sets from the template. If the template has none, then none are allowed. Note that the field definitions updates are due to https://redhat.atlassian.net/browse/OSAC-1416 |
|
I think issue might be the field_definition examples are incorrect |
which example exactly ? see https://redhat.atlassian.net/browse/OSAC-1416 and osac-project/fulfillment-service#59 |
|
also this is the BE behavior regarding the field_definitions now Field DefinitionsHow StructureEach
Editability and defaults
There is no explicit Pipeline orderWhen a resource is created via a catalog item, the backend processes steps in this order:
Field definitions are self-contained. They are fully evaluated before template defaults are merged. A field_definition cannot rely on template defaults to supply a missing value — the check at step 4 runs before step 6.
|
|
just for the reference, the is the node sets enforcement osac-project/enhancement-proposals#112 |
For the sake of closure, we discussed this offline, I understood that following a backend change indeed the node sets should accept only hosts that are in the template |
- use JSX composition instead of adapter - fix field_definitions handling - due to BE breaking changes - only fields in field_definitions are allowed, all other must not be in the request - enable/disable wizard fields based on the presence of the field path + enabled=true - parse JSON Schema validation and add it to Yup schema - fix cluster node sets - only node sets defined in the cluster template are allowed - host type cannot be selected, only node set size can be adjusted (if field_definition allows it) - Add generic, reusable Wizard Footer component
ElayAharoni
left a comment
ElayAharoni
left a comment
There was a problem hiding this comment.
Good refactor overall — the move from adapter pattern to JSX composition is clean, field_definitions-to-Yup conversion is solid, and the router migration / protobuf registry / Go proxy changes look correct.
A few issues to address before merging, organized by severity below.
| [ClusterState.READY]: { status: 'ready', text: 'Ready' }, | ||
| [ClusterState.FAILED]: { status: 'failed', text: 'Failed' }, | ||
| [ClusterState.DELETING]: { status: 'progressing', text: 'Deleting' }, | ||
| [ClusterState.DELETE_FAILED]: { status: 'failed', text: 'Delete failed' }, |
There was a problem hiding this comment.
HIGH — Correctness: CLUSTER_STATUS_MAP now includes DELETING and DELETE_FAILED, but the resolveClusterStatus switch statement below was not updated with matching cases. Clusters in these states will fall through to the default case and display as "Unknown" instead of "Deleting" / "Delete failed".
Either add case ClusterState.DELETING: and case ClusterState.DELETE_FAILED: to the switch, or refactor to use a direct map lookup:
return CLUSTER_STATUS_MAP[state] ?? CLUSTER_STATUS_MAP[ClusterState.UNSPECIFIED];There was a problem hiding this comment.
the resolveClusterStatus function was actually not needed - removed. Im accessing the CLUSTER_STATUS_MAP directly now.
| name: buildMetadataNameSchema(t), | ||
| }), | ||
| spec: Yup.object({ | ||
| runStrategy: fieldSchema('run_strategy'), |
There was a problem hiding this comment.
MEDIUM — Correctness: runStrategy uses fieldSchema('run_strategy') with no base schema, which defaults to Yup.mixed() — no required() check. The initial value is undefined and SelectField has 3 options (no auto-select), so a user can submit without selecting a run strategy.
The VM wizard correctly uses:
runStrategy: fieldSchema(
'run_strategy',
Yup.string().required(t('Run strategy is required')),
),Consider applying the same pattern here.
There was a problem hiding this comment.
in case of VM, runStrategy is required field.
in case of BM, it is optional - so the validation & the fom is valid.
it is aligned with the API
| <Spinner aria-label={t('Loading catalog')} /> | ||
| </Bullseye> | ||
| ); | ||
| } |
There was a problem hiding this comment.
MEDIUM — Dead code + UX inconsistency: This unconditional if (isLoading) returns a spinner for all loading states, making the if (catalogItemId && isLoading) guard at line 65 unreachable.
The VM wizard uses only if (catalogItemId && isLoading) and the Cluster wizard uses if ((catalogItemId && isLoading) || templatesLoading). This broader check means the BM wizard blocks rendering the entire wizard shell during any loading — unlike the other two which render immediately and let CatalogStepContent handle its own loading state.
Suggestion: remove this unconditional check and keep only the if (catalogItemId && isLoading) guard below.
There was a problem hiding this comment.
I aligned all wizards to block until we load catalog items / templates
| <Button variant="link" isInline onClick={() => navigate('/vms')}> | ||
| {t('Virtual Machines')} | ||
| </Button> | ||
| </BreadcrumbItem> |
There was a problem hiding this comment.
MEDIUM — Pattern consistency / Accessibility: Two inconsistencies with the BM and Cluster wizards:
- This header
PageSectiondoesn't wrap Breadcrumb/Title/Content in<Stack hasGutter>, causing tighter vertical spacing than the other wizards. - The wizard
PageSectionat line 102 lacksaria-label— BM wizard hasaria-label={t('Bare metal provisioning wizard')}and Cluster hasaria-label={t('Create cluster wizard')}. Missingaria-labelreduces accessibility for screen readers navigating by landmark.
There was a problem hiding this comment.
fixed. The Stack component is removed from all.
| provisionError: | ||
| err instanceof Error ? err.message : t('Provisioning failed. Please try again.'), | ||
| }); | ||
| } |
There was a problem hiding this comment.
LOW — UX regression: The old VmCreatePage called qc.setQueryData(apiQueryKey('v1/compute_instances', [instance.id]), instance) before navigating, so the details page rendered immediately. Now only navigate() is called — the details page will show a brief loading flash. Same applies to ClusterCreateWizard.
Nice-to-have: add qc.setQueryData() before navigating using useApiQueryClient().
There was a problem hiding this comment.
The VM wizard was exception. No other wizard did this. I prefer to remove this behavior to have all wizards aligned.
We can introduce it later but on the hook level - anytime a create hook is called, it should handle adding the response to cache. This is something we can do in a followup.
| acc[curr.name] = { | ||
| size: curr.size, | ||
| }; | ||
| return acc; |
There was a problem hiding this comment.
LOW — Code quality: When isFieldEditable('node_sets', fds) is true, isFieldEditable('node_sets.${name}.size', fds) also resolves to true (path prefix inheritance). Both this block and the per-set loop at line 68 fire, with the loop redundantly overwriting the same data.
No data corruption, but this should be an if/else: write all node sets if top-level is editable, otherwise fall through to per-set size checks.
There was a problem hiding this comment.
Right, this should be fixed now.
|
|
||
| const defaultSshKey = getStringDefaultValue('ssh_public_key', fds); | ||
| if (defaultSshKey) { | ||
| base.spec.sshPublicKey = defaultSshKey; |
There was a problem hiding this comment.
LOW — Latent correctness: if (defaultBootDisk) uses a truthiness check, but getNumberDefaultValue can return 0 which is falsy. Works today because the empty form default for sizeGib is also 0, but will break if that changes.
The cluster wizard correctly uses if (defSize !== undefined) for the same pattern. Same issue at line 95 with defaultRunStrategy.
Suggestion: use !== undefined checks instead of truthiness.
There was a problem hiding this comment.
Fixed - using !== undefined check
|
|
||
| const fieldName = 'spec.sshPublicKey'; | ||
|
|
||
| const SshKeyField = ({ fieldDefinitions }: SshKeyFieldProps) => { |
There was a problem hiding this comment.
LOW — Dead code: path?: string is declared but never consumed — the component destructures only { fieldDefinitions } and uses the hardcoded constant fieldName = 'spec.sshPublicKey'. No call site passes this prop.
There was a problem hiding this comment.
fixed, path prop is removed.
| import { clearSchemaCache } from '../../catalogProvision/validation'; | ||
|
|
||
| export interface ComputeInstanceWizardValues { | ||
| catalogItem: ComputeInstanceCatalogItem | undefined; |
There was a problem hiding this comment.
LOW — Conventions: ComputeInstanceCatalogItem is used only in type positions — should be import type { ComputeInstanceCatalogItem }. Same pattern in several other new files:
BareMetalConfigurationStep.tsx:import { TFunction }→import typessh-public-key.ts:import { TFunction }→import typeSshKeyField.tsx,ClusterGeneralStep.tsx,UserDataField.tsx,validation.ts:import { FieldDefinition }→import type
There was a problem hiding this comment.
these should be fixed now
|
/hold |
|
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. |
4 participants
use JSX composition instead of adapter
fix field_definitions handling - due to BE breaking changes
fix cluster node sets
Add generic, reusable Wizard Footer component