Skip to content

PRD: Configuration wizard for cluster and VM resources - #57

Merged
avishayt merged 16 commits into
osac-project:mainfrom
batzionb:prd/OSAC-1421
Jun 23, 2026
Merged

avishayt merged 16 commits into
osac-project:mainfrom
batzionb:prd/OSAC-1421

Conversation

@batzionb

@batzionb batzionb commented Jun 14, 2026 •

Copy link
Copy Markdown
Contributor

PRD: Configuration wizard for cluster and VM resources

Jira: https://redhat.atlassian.net/browse/OSAC-1421

Summary

This PRD defines the provisioning wizard in the OSAC UI for ComputeInstance and Cluster resources. Tenant users select a published catalog offering, complete five guided steps (catalog selection -> General -> Compute -> Networking -> Review), and submit a create request shaped by the configured fields.

Requesting Review On

  • Requirements completeness and accuracy
  • Scope (goals and non-goals)
  • Acceptance criteria clarity
  • Open questions that need resolution

How to Review

  • Comment inline on specific sections
  • Review the open questions — these need your input
  • Approve when the PRD accurately reflects the agreed requirements

Summary by CodeRabbit

  • Documentation
    • Added comprehensive PRD documentation for a Configuration Wizard covering cluster and VM resources with a guided five-step workflow (Catalog → Access → Configuration → Networking → Review/Submit), including detailed field mapping, validation behavior, UX rules, and acceptance criteria.

Assisted-by: Claude Code <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 14, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

A new PRD document is added defining a five-step configuration wizard (Catalog → Access → Configuration → Networking → Review) for ComputeInstance and Cluster resources. It specifies static wizard fields, catalog field_definitions overlay rules, picker-backed APIs for VM networking and instance types, step-level validation behavior, acceptance criteria, API dependencies, and a set of open design decisions.

Changes

Configuration Wizard PRD

Layer / File(s) Summary
Document metadata, scope, goals, and non-goals
enhancements/cluster-and-vm-provisioning-wizard/prd.md
Front-matter with authors and timestamps, wizard goals for a unified five-step flow, and explicit non-goals (dynamic field additions, disk management, node set edits, template/provisioning configuration).
Static wizard field model and catalog overlay rules
enhancements/cluster-and-vm-provisioning-wizard/prd.md
Per-resource, per-step static field lists with payload paths, hardcoded values (e.g., image.source_type=registry), and rules for applying field_definitions overlays on Configuration/Networking steps only while ignoring Access overlays.
Picker-backed networking and instance type APIs
enhancements/cluster-and-vm-provisioning-wizard/prd.md
VM networking cascade (VirtualNetwork → Subnet → SecurityGroups), network_attachments payload structure, and instance type picker with deprecation/warning behavior, load order, and single-option auto-select rule.
Wizard navigation, validation, and acceptance criteria
enhancements/cluster-and-vm-provisioning-wizard/prd.md
Next always enabled but triggers full current-step validation including untouched fields, blocking progression on failure with an alert. Acceptance criteria covering payload, UX steps, overlay/default, and compute/cluster provisioning expectations.
API dependencies and open design decisions
enhancements/cluster-and-vm-provisioning-wizard/prd.md
Enumerated external API dependencies and unresolved decisions on required-vs-optional fields, catalog default vs picker auto-select precedence, default-not-in-options handling, nested networking overlay paths, empty node_sets, and additional-disks scope.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

  • avishayt

Poem

A wizard in five steps appears,
Catalog first, then networks clear 🧙‍♂️
Fields align through overlays true,
Pickers cascade, auto-select too,
Questions remain—bold open doors,
A PRD blueprint for what's in store ✨

🚥 Pre-merge checks | ✅ 10 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Ai-Attribution ⚠️ Warning Commit cd51448 uses Co-authored-by trailer for AI tool Cursor; only Assisted-by/Generated-by should be used for AI attribution per check guidelines. Remove the Co-authored-by: Cursor line and ensure only Assisted-by: Claude Code trailer is present in commit messages for AI-assisted work.
✅ Passed checks (10 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: introducing a PRD for a configuration wizard feature.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed PRD document contains no hardcoded secrets, API keys, tokens, passwords, or credentials—only legitimate references to field paths and configuration concepts in requirements tables.
No-Weak-Crypto ✅ Passed PR contains only a markdown requirements document with no code files. No weak cryptographic algorithms, custom crypto implementations, or non-constant-time secret comparisons are present.
No-Injection-Vectors ✅ Passed PR adds only a markdown PRD file with no executable code or injection vectors; all security patterns checked returned no matches.
Container-Privileges ✅ Passed This PR adds only a markdown PRD document with no Kubernetes/container manifests, Dockerfiles, or container configurations. The check for privileged container settings is not applicable to document...
No-Sensitive-Data-In-Logs ✅ Passed PRD document contains no sensitive data: author email is standard YAML metadata attribution, field references (ssh keys, pull secrets) are UI specifications not logged credentials, and no actual pa...

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Assisted-by: Claude Code <noreply@anthropic.com>
@batzionb
batzionb marked this pull request as ready for review June 14, 2026 13:21

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@enhancements/configuration-wizard-OSAC-1421/prd.md`:
- Around line 25-79: The Review step requirements (FR-11 and FR-12) create a
contradiction with the Basics step requirements (FR-5 through FR-7). FR-11 and
FR-12 state that Review displays only entries from field_definitions, but Basics
fields (metadata.name, SSH key, and pull secret when applicable) are collected
in step 2 and are not part of field_definitions — they are hardcoded wizard
fields. This means users cannot review the values they entered in Basics before
submitting. Update FR-11 to clarify that the Review step must display both the
Basics fields collected in step 2 (per FR-5, FR-6, and FR-7) and all entries
from field_definitions, so users can confirm all their inputs before submission.
Ensure FR-12 is understood to apply only to field_definitions entries, not to
the fixed Basics fields.
🪄 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: Enterprise

Run ID: b64dfe07-dd2f-4b56-be4e-ff8352aecea6

📥 Commits

Reviewing files that changed from the base of the PR and between 39a76ae and e95e18d.

📒 Files selected for processing (1)
  • enhancements/configuration-wizard-OSAC-1421/prd.md

Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
batzionb added 2 commits June 14, 2026 16:26
…item

Assisted-by: Claude Code <noreply@anthropic.com>
Assisted-by: Claude Code <noreply@anthropic.com>
@batzionb

Copy link
Copy Markdown
Contributor Author

@coderabbitai help

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

CodeRabbit commands

These commands are invoked using PR/Issue comments.

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai rate limit to show your current review rate limit status.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai evaluate custom pre-merge check --instructions <custom-checks-instructions> --name <custom-checks-title> [--mode <error|warning>] to validate the custom pre-merge checks instructions. Defaults to error when --mode is omitted.
  • @coderabbitai ignore pre-merge checks to override pre-merge checks and get an approval on PR.
  • @coderabbitai run pre-merge checks to run pre-merge checks on the pull request.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai emit path instructions to emit generated path-specific review instructions for this repository.
  • @coderabbitai resolve merge conflict to automatically resolve merge conflicts.
  • @coderabbitai autofix to automatically fix issues identified in unresolved review comments.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai generate configuration to create a PR that adds the current resolved configuration as .coderabbit.yaml (or show it if already present).
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, support, documentation and community

  • Visit our status page to check the current availability of CodeRabbit.
  • Create a ticket on our support page for assistance with any issues or questions.
  • Visit our documentation site for detailed information on how to use CodeRabbit.
  • Join our Discord community to connect with other users and get help from the community.
  • Follow us on X/Twitter for updates and announcements.

#### Basics (step 2)

- **FR-5:** Step 2 (Basics) must always collect `metadata.name`.
- **FR-6:** Step 2 must collect SSH credentials per resource type: cluster — `spec.ssh_public_key`; VM — `spec.ssh_key`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to the PRD: @ygalblum let's align VMs to ssh_public_key

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for BareMetalInstance

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's finalize the PRD for VM and cluster, we will have a separate one for baremetal

The following are explicitly out of scope and **will not be supported**:

- Dynamic Template parameters
- Fetching select or list options from separate fulfillment list APIs (`virtual_networks`, `subnets`, etc.) when not defined in the catalog item's `field_definitions`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine for now but sounds really useful for a future version

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this is a non goal. We should give the user the ability to choose a virtual_network, subnet and security_groups.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I'll change the PRD to include this, we need the API for each of these fields

Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated

## 6. Open Questions

### 6.1 How should required fields be specified?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding "required" to the schema makes sense - please be in touch with whoever is responsible for it on the backend

Co-authored-by: Avishay Traeger <avishayt@users.noreply.github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
enhancements/configuration-wizard-OSAC-1421/prd.md (2)

102-115: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Section 6.1 is an implementation-blocking open question — elevate resolution priority.

The required-field semantics question is correctly identified as open with assigned ownership (API / catalog authoring). However, its resolution is a critical blocker for implementation start:

  • Security/data-quality risk: If this is unresolved at code-review time, the wizard will either silently accept incomplete Configuration fields (data integrity risk) or impose validation logic that contradicts the actual FieldDefinition schema (correctness bug).
  • Current state: Three options are presented (implicit required, schema constraints only, explicit flag), but none is chosen. The wizard implementation cannot proceed without knowing which rule applies.

High severity — functional correctness; implementation blocker. Recommend:

  1. Document which option is chosen (lines 112 needs a decision, not a question).
  2. Ensure API ownership completes the FieldDefinition schema change (if option 3 is selected) before the wizard code review.
  3. Add an acceptance criterion that validates the chosen validation behavior end-to-end.
🤖 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 `@enhancements/configuration-wizard-OSAC-1421/prd.md` around lines 102 - 115,
Section 6.1 currently presents three unresolved options for how required fields
should be handled in the wizard, which blocks implementation. Replace the open
question at the end of the section (the "Still open:" paragraph) with a clear
decision on which approach will be used: either (1) treat all shown
Configuration fields as required by default, (2) rely solely on per-value schema
constraints like minLength to enforce non-emptiness, or (3) add an explicit
required flag to the FieldDefinition API. Document the chosen option explicitly,
ensure any needed API/schema changes are captured in the ownership section with
clear dependencies, and add an acceptance criterion that specifies how the
chosen validation behavior will be tested end-to-end to prevent silent
acceptance of incomplete fields or validation contradictions.

56-57: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clarify default value handling for non-editable fields.

FR-9 states that non-editable fields' defaults "must be included silently in the create payload" (line 56), and FR-16 requires "default values for all non-editable field_definitions" (line 73). However, FR-9 does not specify what happens if a non-editable field has no default value defined. This creates a data-integrity risk: the payload construction logic could fail or silently omit fields if defaults are missing.

Medium severity — data integrity before create. If a catalog item defines a non-editable field without a default, the wizard cannot safely construct the create request.

Recommend clarifying:

  • Are non-editable fields required to have a default value in the catalog item's field_definitions?
  • If not, what should the wizard do (skip the field, use empty string, reject the catalog item, etc.)?
🤖 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 `@enhancements/configuration-wizard-OSAC-1421/prd.md` around lines 56 - 57, The
PRD contains an ambiguity regarding non-editable fields without default values:
FR-9 and FR-16 require that non-editable field defaults be included in the
create payload, but neither requirement explicitly states whether a non-editable
field is allowed to lack a default value or what the expected behavior should be
in such cases. Update the PRD to clarify two critical points: (1) state whether
non-editable fields are required to have a default value defined in the catalog
item's field_definitions, and (2) if not required, specify the exact behavior
the wizard must implement (such as skipping the field, rejecting the catalog
item, or using a fallback value). This clarification should be added near FR-9
or as a new functional requirement to eliminate the data-integrity risk
described in the review comment.
🤖 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 `@enhancements/configuration-wizard-OSAC-1421/prd.md`:
- Line 85: The acceptance criterion at line 85 in prd.md incorrectly lists
"submit" as a separate step when describing the wizard flow, conflicting with
the flowchart (lines 32-38) and FR-2 requirement (line 42) which clearly show
four steps with submit as an action triggered from the Review step, not a step
itself. At line 85, reword the criterion to clearly state the four steps are
"catalog selection → Basics → Configuration → Review" and clarify that submit is
an action performed from the Review step. Apply the same clarification to line
42 in FR-2 to ensure consistency across the document.

---

Outside diff comments:
In `@enhancements/configuration-wizard-OSAC-1421/prd.md`:
- Around line 102-115: Section 6.1 currently presents three unresolved options
for how required fields should be handled in the wizard, which blocks
implementation. Replace the open question at the end of the section (the "Still
open:" paragraph) with a clear decision on which approach will be used: either
(1) treat all shown Configuration fields as required by default, (2) rely solely
on per-value schema constraints like minLength to enforce non-emptiness, or (3)
add an explicit required flag to the FieldDefinition API. Document the chosen
option explicitly, ensure any needed API/schema changes are captured in the
ownership section with clear dependencies, and add an acceptance criterion that
specifies how the chosen validation behavior will be tested end-to-end to
prevent silent acceptance of incomplete fields or validation contradictions.
- Around line 56-57: The PRD contains an ambiguity regarding non-editable fields
without default values: FR-9 and FR-16 require that non-editable field defaults
be included in the create payload, but neither requirement explicitly states
whether a non-editable field is allowed to lack a default value or what the
expected behavior should be in such cases. Update the PRD to clarify two
critical points: (1) state whether non-editable fields are required to have a
default value defined in the catalog item's field_definitions, and (2) if not
required, specify the exact behavior the wizard must implement (such as skipping
the field, rejecting the catalog item, or using a fallback value). This
clarification should be added near FR-9 or as a new functional requirement to
eliminate the data-integrity risk described in the review comment.
🪄 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: Enterprise

Run ID: 0b04c7c5-9881-4b49-b803-dc377ce6812c

📥 Commits

Reviewing files that changed from the base of the PR and between e95e18d and cfe6e7d.

📒 Files selected for processing (1)
  • enhancements/configuration-wizard-OSAC-1421/prd.md

Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
@batzionb

Copy link
Copy Markdown
Contributor Author

@coderabbitai autofix

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

This command requires write access to the repository. Only users with write or admin permissions can trigger CodeRabbit to commit or create pull requests.

@adriengentil

Copy link
Copy Markdown
Contributor

Should this PRD cover BaaS as well?

### 2.1 Goals

- Tenant users provision VMs and clusters by selecting a catalog offering, completing a short guided wizard, and submitting — without seeing the full resource spec.
- All configurable fields and select options are driven by the selected catalog item's `field_definitions`; the wizard adds no resource-specific form fields beyond the fixed Basics step.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fields should be derived from both the ComputeInstance spec (what called here "basic") and the template parameter fields defined in the catalog item.

For fields that come from the ComputeInstance spec, the corresponding catalog item field, when defined, can provide:

  1. A default value
  2. Whether the field is editable
  3. A validation pattern

If no matching field definition exists in the catalog item, the ComputeInstance "basic" spec field should still be displayed and should remain editable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlonaKaplan Regarding the UI implementation: we should avoid exposing all API fields directly to the user. Some fields are intended for internal use only. For instance, instead of showing network_attachments in its raw format, we should provide a more intuitive workflow - such as selecting a virtual network, subnet, security group, and IP - to keep the user experience clean and manageable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @AlonaKaplan and @danmanor here. The wizard should derive fields from multiple sources, not just field_definitions:

  1. Resource spec — core fields like cores, memory_gib, ssh_key, network_attachments (per resource type, as @danmanor suggested)
  2. Catalog item field_definitions — overrides for editability, defaults, and validation_schema
  3. Template parameters — the catalog item has a template reference, and the UI can call Templates.Get to fetch the template's ParameterDefinition array, which already carries required, type, and default for each Ansible/AAP parameter

This also addresses the open question in §6.1 about required-field semantics — the template parameter definitions already have a required flag, so the wizard doesn't need to invent its own mechanism. Where a field_definition exists for a given path, it provides additional constraints (editability, validation_schema, display_name); where it doesn't, the resource spec field should still be shown and remain editable (as @AlonaKaplan noted).

The PRD should be updated to reflect this three-source model instead of the current pure field_definitions-driven approach.

The following are explicitly out of scope and **will not be supported**:

- Dynamic Template parameters
- Fetching select or list options from separate fulfillment list APIs (`virtual_networks`, `subnets`, etc.) when not defined in the catalog item's `field_definitions`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this is a non goal. We should give the user the ability to choose a virtual_network, subnet and security_groups.


- **FR-5:** Step 2 (Basics) must always collect `metadata.name`.
- **FR-6:** Step 2 must collect SSH credentials per resource type: cluster — `spec.ssh_public_key`; VM — `spec.ssh_key`.
- **FR-7:** Step 2 must collect `spec.pull_secret` for cluster catalog items; pull secret must be omitted for VM catalog items where not applicable.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this sentence.


#### Configuration (step 3)

- **FR-8:** Step 3 must render every **editable** entry in the selected catalog item's `field_definitions` array in array order, excluding fields already collected in Basics (`metadata.name`, SSH key, pull secret when shown).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the basic list should contain more fields/all ComputeInstance required fields - cores, memoryGiB , bootDisk, image , runStrategy , networkAttachments. @ygalblum @avishayt wdyt? Currently the template hard code most of those values.
@danmanor why networkAttachments is not defined as required in ComputeInstance?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think each type - (computeInstance, BaremetalInstance, cluster) should have a set of fields that are rendered in the UI regardless of the catalog item, and also there should be few fields that are shared across all types (name, network, etc.)

@danmanor danmanor Jun 17, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danmanor why networkAttachments is not defined as required in ComputeInstance?

@AlonaKaplan Where do you mean ? in the catalog item ? API ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API/CRD

#### Configuration (step 3)

- **FR-8:** Step 3 must render every **editable** entry in the selected catalog item's `field_definitions` array in array order, excluding fields already collected in Basics (`metadata.name`, SSH key, pull secret when shown).
- **FR-9:** Step 3 must not render **non-editable** `field_definitions`; their `default` values must be included silently in the create payload.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where the tenant user ill have a chance to see those values?


#### Review (step 4)

- **FR-11:** Step 4 (Review) must always display the Basics fields collected in step 2 with the values the user entered: `metadata.name`, SSH key (`spec.ssh_public_key` for cluster; `spec.ssh_key` for VM), and `spec.pull_secret` for cluster catalog items (pull secret omitted for VM when not applicable).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I"m not sure I understand, when a pull_secret should be omitted?

#### Field rendering

- **FR-14:** For each editable field in Configuration, the wizard must derive the input widget from the field's `validation_schema` (JSON Schema draft 2020-12): `type: integer` → number input; `enum` present → select; all other types → plain text input.
- **FR-15:** Select and list options for every select field must come from the `enum` values in that field's `validation_schema` within `field_definitions`. The wizard must not call separate fulfillment list APIs to populate options.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not accurate for the networkAttachments fields. A separate API will need to be invoked to determine the options.


- **FR-8:** Step 3 must render every **editable** entry in the selected catalog item's `field_definitions` array in array order, excluding fields already collected in Basics (`metadata.name`, SSH key, pull secret when shown).
- **FR-9:** Step 3 must not render **non-editable** `field_definitions`; their `default` values must be included silently in the create payload.
- **FR-10:** The Configuration step must include only fields defined in the selected catalog item's `field_definitions`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • the "basic" values.

@batzionb

batzionb commented Jun 17, 2026 •

Copy link
Copy Markdown
Contributor Author

@danmanor @avishayt @AlonaKaplan
Please provide a list of the fields that are in the API spec of ComputeInstance and cluster that the UI must always show, that you all agree on

In addition, please provide for each select field the API to use to fetch the dropdown options.

Let's leave out of this PRD the following 2 features:

  1. Baremetal - will be in a separate PRD
  2. Template parameters are for 0.2

batzionb added 4 commits June 17, 2026 15:46
Replace catalog-driven field discovery with a fixed per-resource-type
field set. Catalog field_definitions override display, editability,
defaults, required, and validation for matching paths. Add ComputeInstance
networking pickers (virtual network, subnet, security groups) and resolve
required-field semantics. Template parameters and BareMetal remain out
of scope.

Assisted-by: Claude Code <noreply@anthropic.com>
…view

Assisted-by: Claude Code <noreply@anthropic.com>
Streamline wizard requirements, document VM networking list APIs,
read-only catalog overlay fields, field defaults, and shortened
open-decision section for requiredness and additional disks.

Assisted-by: Claude Code <noreply@anthropic.com>
Assisted-by: Claude Code <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@enhancements/configuration-wizard-OSAC-1421/prd.md`:
- Around line 115-140: When the virtual network selection changes, reset and
clear the currently selected subnet and security group values before loading the
new filtered subnet and security group lists. This ensures that stale subnet or
security group selections cannot persist and produce invalid network_attachments
in the payload. Update the load order documentation to clarify that virtual
network selection triggers a clear of dependent subnet/security group
selections, followed by loading the filtered lists and auto-selecting when
exactly one item is available.
🪄 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: Enterprise

Run ID: a60977e8-d84f-40df-ac2c-fbbe6dc0cb88

📥 Commits

Reviewing files that changed from the base of the PR and between cfe6e7d and 6baea49.

📒 Files selected for processing (1)
  • enhancements/configuration-wizard-OSAC-1421/prd.md

Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
@batzionb

Copy link
Copy Markdown
Contributor Author

@avishayt @AlonaKaplan @danmanor @ygalblum
PRD is ready now - PTAL :)

Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
| Label | `display_name` or wizard default | Wizard default |
| Editable | `editable: false` → read-only on wizard step (requires `default`; see [§3.2](#32-wizard-behavior)) | `true` |
| Default | Catalog `default` if set; else blank | Blank |
| Validation | `validation_schema` | API/wizard validation |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it also affect the options the user is shown? Not sure I have an example here. But, let's say a field is an enum of 3 values but the validation allows only for 2 of them. Will the user be able to pick the third value (which will lead to failure on submit)?

@batzionb batzionb Jun 18, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the validation_schema contains an enum with three values, we will show the 3 values, if in addition there's a regex that contradict those enum values, that's a defective schema and I'm not sure the UI needs to handle that
Regarding the network attachement fields - I'm not sure what we want to do if the field_definitions contain validation schemas for subnet and security groups because the UI takes the dropdown values from the API. I think for 0.1 we can accept this as a gap

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example - release image can have an enum defined with three different release images user can choose from

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the validation_schema contains an enum with three values, we will show the 3 values, if in addition there's a regex that contradict those enum values, that's a defective schema and I'm not sure the UI needs to handle that

That's not what I meant. So, I'll try to clarify with an example.
Let's say the field in the API itself is an ENUM with 3 options. Just for example let's call it color and the enum is red, green, blue. But, the validation scheme is an enum of red and green. That is the user can choose between red and green, but they cannot chose blue. Will the UI show blue in the drop-down menu and show an error upon selection, or not show it at all? I would hope for the latter.

As for the network attachement fields, there will be additional fields with similar behaviour - in v1 that's InstanceType, in the future also ComputeImage

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that is one of the open questions

https://github.com/batzionb/enhancement-proposals/blob/cd514489f963df65cc502d32e61aa6e07f1dae0a/enhancements/cluster-and-vm-provisioning-wizard/prd.md?plain=1#L232

I'm not sure what is the correct approach, we should just choose something simple and see later
The most simple solution is to ignore the catalogitem field_definitions for the backend driven dropdowns and disable defining these in the catalogitem API
This approach will make the catalogitem pretty meaningless, because there'll be barely anything else to configure... Which brings up the question of the catalogitem usecase

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make the catalogitem pretty meaningless

Not at all. First, keep in mind that we're talking about whether the UI is going to validate the input or not. The backend will have to validate the value against the validation schema. The reason, the UI may chose not to, is thae fact that the backend has to run the validation because the UI is not the only way to call the API. Second, the UI, even in this stage, will still get the default values whether the fields are editable.

Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
Comment thread enhancements/configuration-wizard-OSAC-1421/prd.md Outdated
Move the PRD to cluster-and-vm-provisioning-wizard, add YAML frontmatter,
renumber sections from §1, adopt Access/Configuration steps with image on
Configuration, and ignore catalog field_definitions for network_attachments.

Assisted-by: Claude Code <noreply@anthropic.com>
batzionb and others added 2 commits June 21, 2026 16:22
Add VM instance type picker and OS family fields, open decisions for catalog
overlay and cluster node_sets, Review mirroring wizard fields, Access step
exempt from field_definitions, and validate-on-Next step navigation.

Assisted-by: Claude Code <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Assisted-by: Claude Code <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@enhancements/cluster-and-vm-provisioning-wizard/prd.md`:
- Line 219: Remove the trailing whitespace (space character) that appears at the
end of the line containing "Resolve before implementation." in the prd.md file.
Locate the text "Resolve before implementation." and ensure there are no spaces
after the period at the end of that line.
- Around line 109-110: The "Instance type (VM)" row in the markdown table on
line 109 contains an extra empty cell marker at the end, creating an unintended
third column when the table only has two columns. Remove the trailing pipe and
spaces after the description text in the "Instance type (VM)" row to align with
the 2-column structure, making it consistent with the "Networking pickers" row
below it which has the correct format.
🪄 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: Enterprise

Run ID: 26da277d-f2f1-4474-a914-024a0cfd2a71

📥 Commits

Reviewing files that changed from the base of the PR and between 6baea49 and 5572848.

📒 Files selected for processing (1)
  • enhancements/cluster-and-vm-provisioning-wizard/prd.md

Comment thread enhancements/cluster-and-vm-provisioning-wizard/prd.md Outdated
Comment thread enhancements/cluster-and-vm-provisioning-wizard/prd.md
Assisted-by: Claude Code <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@batzionb

Copy link
Copy Markdown
Contributor Author

@ygalblum
I think I addressed all the comments
Can you PTAL?
Also - can you please address open questions?

Assisted-by: Claude Code <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@batzionb

Copy link
Copy Markdown
Contributor Author

@rawagner @ElayAharoni FYI

Comment on lines +47 to +57
| Step | Path | Label | Widget | Required |
| --------------- | ------------------------- | ---------------------------------------- | -------------------------------------- | -------- |
| Access | `metadata.name` | Name | Text | Required |
| Access | `spec.ssh_key` | SSH public key | Text (multiline) | ? |
| Configuration | `spec.image.source_ref` | VM image (OCI reference) | Text | Required |
| Configuration | `spec.is_windows` | OS family | Radio (`Linux`, `Windows`) | Required |
| Configuration | `spec.instance_type` | Instance type | Picker ([§2.1.5](#215-vm-instance-type-picker-api)) | Required |
| Configuration | `spec.user_data` | User data (cloud-init / Ignition) | Text (multiline) | Optional |
| Configuration | `spec.boot_disk.size_gib` | Boot disk size (GiB) | Number | ? |
| Configuration | `spec.run_strategy` | Run strategy | Select (`Always`, `Halted`) | Required |
| Networking | `spec.network_attachments` | Virtual network, subnet, security groups | Pickers ([§2.1.4](#214-vm-networking-picker-apis)) | Required |

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think first step should be called Access and specifying resource name should be part of it.

All (?) resources that we will be creating, will require a name - should the first step then be called something like General info ? (We use that in Flight Control).

We should allow adding labels in this step too.

Maybe like this ?

Suggested change
| Step | Path | Label | Widget | Required |
| --------------- | ------------------------- | ---------------------------------------- | -------------------------------------- | -------- |
| Access | `metadata.name` | Name | Text | Required |
| Access | `spec.ssh_key` | SSH public key | Text (multiline) | ? |
| Configuration | `spec.image.source_ref` | VM image (OCI reference) | Text | Required |
| Configuration | `spec.is_windows` | OS family | Radio (`Linux`, `Windows`) | Required |
| Configuration | `spec.instance_type` | Instance type | Picker ([§2.1.5](#215-vm-instance-type-picker-api)) | Required |
| Configuration | `spec.user_data` | User data (cloud-init / Ignition) | Text (multiline) | Optional |
| Configuration | `spec.boot_disk.size_gib` | Boot disk size (GiB) | Number | ? |
| Configuration | `spec.run_strategy` | Run strategy | Select (`Always`, `Halted`) | Required |
| Networking | `spec.network_attachments` | Virtual network, subnet, security groups | Pickers ([§2.1.4](#214-vm-networking-picker-apis)) | Required |
| Step | Path | Label | Widget | Required |
| --------------- | ------------------------- | ---------------------------------------- | -------------------------------------- | -------- |
| General info | `metadata.name` | Name | Text | Required |
| General info | `metadata.labels` | Labels | Text array | Optional |
| Configuration | `spec.ssh_key` | SSH public key | Text (multiline) | ? |
| Configuration | `spec.image.source_ref` | VM image (OCI reference) | Text | Required |
| Configuration | `spec.is_windows` | OS family | Radio (`Linux`, `Windows`) | Required |
| Configuration | `spec.instance_type` | Instance type | Picker ([§2.1.5](#215-vm-instance-type-picker-api)) | Required |
| Configuration | `spec.user_data` | User data (cloud-init / Ignition) | Text (multiline) | Optional |
| Configuration | `spec.boot_disk.size_gib` | Boot disk size (GiB) | Number | ? |
| Configuration | `spec.run_strategy` | Run strategy | Select (`Always`, `Halted`) | Required |
| Networking | `spec.network_attachments` | Virtual network, subnet, security groups | Pickers ([§2.1.4](#214-vm-networking-picker-apis)) | Required |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposal: reorder the five steps to:

Catalog Item → Configuration → Networking → Access → Review and Submit

Configuration (resource shape + identity):

metadata.name first (required for all resources)
Then the fields that are Configuration in the PRD today:
VM: spec.image.source_ref, spec.is_windows, spec.instance_type, spec.user_data, spec.boot_disk.size_gib, spec.run_strategy
Cluster: spec.release_image, spec.node_sets

Networking — unchanged from the PRD (spec.network_attachments for VM; spec.network.pod_cidr / spec.network.service_cidr for cluster).

Access — credentials only, after the user has chosen catalog, sizing/platform, and networking:

VM: spec.ssh_key
Cluster: spec.ssh_public_key, spec.pull_secret

Review and Submit — unchanged.

@rawagner @ygalblum WDYT?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the metadata.labels ? Are we completely ignoring them in the UI for now ? IMO we should expose them for every resource, same as metadata.name.

Im still more inclined to have name/labels in a separate step from the rest of the configuration. We wouldnt want to have too many fields in one step.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point about labels
Should we have:
Catalog Item → Identity -> Configuration → Networking → Access → Review and Submit

Identity: metadata.name and metadata.labels (we can also add annotations later)

Configuration

VM: spec.image.source_ref, spec.is_windows, spec.instance_type, spec.user_data, spec.boot_disk.size_gib, spec.run_strategy
Cluster: spec.release_image, spec.node_sets

Networking — unchanged from the PRD (spec.network_attachments for VM; spec.network.pod_cidr / spec.network.service_cidr for cluster).

Access — sshkey and pull secret

VM: spec.ssh_key
Cluster: spec.ssh_public_key, spec.pull_secret

Review and Submit — unchanged.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you prefer just renaming Access to General?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catalog Item → Identity (or General) -> Configuration → Networking → Access → Review and Submit

sounds good to me

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I won't be adding the labels for 0.1 and don't want a step with just one field - name, I'm reverting back to Catalog Item -> General -> Configuration -> Networking -> Review and Submit
We can move the credentials to access step in later release when we have more fields for the first step

Comment thread enhancements/cluster-and-vm-provisioning-wizard/prd.md Outdated
Assisted-by: Claude Code <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@avishayt
avishayt merged commit c853add into osac-project:main Jun 23, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants