OSAC-1421: node_sets are template-driven, not tenant-composed

[ElayAharoni]

Summary

• fulfillment-service's cluster create validation (PrivateClustersServer.validateNodeSets, internal/servers/private_clusters_server.go) rejects any node_sets entry whose map key or host type isn't defined in the associated ClusterTemplate.spec.node_sets — a cluster's node sets are entirely determined by the template; only size is user-configurable per template-defined slot.
• This supersedes the PRD/design's prior resolution ("Cluster node_sets composition" open decision, and the "Cluster template node_sets defaults" non-goal), which had the wizard ignore ClusterTemplate.spec.node_sets entirely and let tenants freely add/remove host-type rows from the full HostTypes.List.
• Revises the field table, the host-type-picker section (renamed to reflect ClusterTemplates.Get as the source of truth, with HostTypes.List now used for display-name resolution only), dependencies, acceptance criteria, and test plan across both prd.md and design.md. Prior text preserved in a collapsed <details> block for traceability.
• Confirmed consistent with the equivalent fix already applied to the CSP Admin catalog item wizard (OSAC-2936, OSAC-2936: add catalog item create wizard for Cluster, VM, and Bare Metal osac-ui#102).

Context

This surfaced from a review discussion on osac-project/osac-ui#99 and osac-project/osac-ui#102 — batzionb flagged that this PRD's node_sets requirement had flip-flopped and no longer matched the backend's actual enforced behavior.

Test plan

• batzionb / rawagner confirm this matches the intended tenant-wizard behavior for PR OSAC-2094: Fix design review post-gate rejecting valid design scores #99
• No further changes needed to internal/servers/private_clusters_server.go reference — verified against current main in fulfillment-service

Assisted-by: Claude Code noreply@anthropic.com

[openshift-ci-robot]

@ElayAharoni: This pull request references OSAC-1421 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 feature to target the "5.0.0" version, but no target version was set.

Details

In response to this:

Summary

• fulfillment-service's cluster create validation (PrivateClustersServer.validateNodeSets, internal/servers/private_clusters_server.go) rejects any node_sets entry whose map key or host type isn't defined in the associated ClusterTemplate.spec.node_sets — a cluster's node sets are entirely determined by the template; only size is user-configurable per template-defined slot.
• This supersedes the PRD/design's prior resolution ("Cluster node_sets composition" open decision, and the "Cluster template node_sets defaults" non-goal), which had the wizard ignore ClusterTemplate.spec.node_sets entirely and let tenants freely add/remove host-type rows from the full HostTypes.List.
• Revises the field table, the host-type-picker section (renamed to reflect ClusterTemplates.Get as the source of truth, with HostTypes.List now used for display-name resolution only), dependencies, acceptance criteria, and test plan across both prd.md and design.md. Prior text preserved in a collapsed <details> block for traceability.
• Confirmed consistent with the equivalent fix already applied to the CSP Admin catalog item wizard (OSAC-2936, OSAC-2936: add catalog item create wizard for Cluster, VM, and Bare Metal osac-ui#102).

Context

This surfaced from a review discussion on osac-project/osac-ui#99 and osac-project/osac-ui#102 — batzionb flagged that this PRD's node_sets requirement had flip-flopped and no longer matched the backend's actual enforced behavior.

Test plan

• batzionb / rawagner confirm this matches the intended tenant-wizard behavior for PR OSAC-2094: Fix design review post-gate rejecting valid design scores #99
• No further changes needed to internal/servers/private_clusters_server.go reference — verified against current main in fulfillment-service

Assisted-by: Claude Code noreply@anthropic.com

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ElayAharoni
Once this PR has been reviewed and has the lgtm label, please assign avishayt for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a1b6cb3c-562a-43a8-aa00-4b753bbf976d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

AI EP Review: EP-166

Score: 8/10 | Verdict: PASS

Criterion	Score	Notes
WHAT (clear need)	1/2	The PRD describes a clear, specific product capability: a provisioning wizard for VMs and clusters with detailed field tables, step definitions, and validation rules. The revision (template-driven node sets) is user-observable and well-specified. However, the diff does not show per-persona user stories (required for a 2); they may exist in unchanged sections since this is a targeted revision PR. Tenant persona is referenced throughout but no 'As a ...' formulations are visible in the changed con
WHY (justification)	2/2	The revision's justification is concrete: fulfillment-service's server-side validation now rejects node sets that don't match the template, meaning without this change cluster creation via the wizard would always fail. The causal chain is clear (server validation -> request rejection -> users can't provision clusters). The superseded non-goal note and re-resolved open decision in section 5 both articulate this constraint well.
User-Facing Focus	1/2	The core requirements describe user-observable wizard behavior (fixed rows, read-only host types, size input, validation). However, the PR introduces three references to 'PrivateClustersServer.validateNodeSets' — an internal Go server struct — and one reference to 'internal/servers/private_clusters_server.go' (a file path) directly in the PRD. These are implementation details that belong in the design document. The review-patterns.md explicitly lists 'PRD names controllers, CRD fields, playbooks
Right-Sized	2/2	Well-scoped and tightly coupled. The VM and cluster provisioning wizard shares an architecture (shared steps + adapter pattern). The revision narrows scope further by making node_sets template-driven (simpler). Non-goals are clearly stated (BareMetalInstance, template parameters, multi-NIC, additional_disks). All capabilities require each other to function — a wizard without Configuration or Networking is incomplete.
Testability	2/2	The revised requirements are verifiable by a PM or QA engineer using the product: wizard shows one row per template node-set key (observable), host type is read-only (observable), size must be > 0 (testable via validation), all rows require positive size before advancing (testable), no add/remove controls (observable), payload uses template keys (verifiable via API). The design doc's test plan covers these scenarios comprehensively. Internal server references in the PRD are context/justification

Verdict: The PRD revision clearly describes a user-observable behavior change (template-driven node sets) with concrete justification, but introduces design leakage by naming internal server components (PrivateClustersServer.validateNodeSets) and file paths in the PRD.

Feedback: Remove the three references to 'PrivateClustersServer.validateNodeSets' and the 'internal/servers/private_clusters_server.go' file path from prd.md — these are implementation details that belong exclusively in design.md. Replace them with user-facing language like 'the server rejects cluster creation requests with node sets that don't match the template' or 'the wizard prevents invalid node-set configurations by construction.' Also ensure the User Stories section (if it exists elsewhere in the PRD) includes a story for the revised cluster provisioning flow, e.g., 'As a Tenant User, I can provision a cluster where node-set types are pre-determined by the template and I only specify the number of nodes per set.'

Critical (0)

None.

Important (2)

1. Design leakage: prd.md references 'PrivateClustersServer.validateNodeSets' (an internal Go server struct) three times and includes the file path 'internal/servers/private_clusters_server.go'. These are implementation details — the PRD should describe what the server does from the user's perspective ('rejects invalid node-set configurations'), not name the internal component that enforces it. See the superseded non-goal note (line ~33), section 2.1.6 notes (line ~234), and section 5 re-resolved d
2. The phrase 'so the wizard cannot construct an invalid payload by construction' in section 2.1.6 is a design assertion, not a user-observable requirement. Reframe as 'tenants cannot submit a cluster with node sets that differ from the template' — testable by a PM without reading code.

Suggestions (2)

1. Consider adding a brief revision summary at the top of section 1.2 (Non-Goals) that explains the 2026-07-27 change in one sentence, so readers scanning non-goals immediately understand the superseded item without reading the full collapsible note.
2. The 'Revised 2026-07-27' inline annotations throughout the PRD are helpful for tracking changes but may accumulate over multiple revisions. Consider consolidating revision history into the frontmatter or a changelog section to keep the requirements text clean.

---

Review cost

Model: claude-opus-4-6
Cost: $0.9490
Tokens: 7 in / 13.6k out
Cache: 250.0k read
Active time: 4m 32s
API calls: 0

[github-actions]

AI Design Review: EP-166

Score: 8/8 | Verdict: PASS

Criterion	Score	Notes
Feasibility	2/2	Implementation details are highly specific: names exact components (ClusterConfigurationStep, buildClusterCreatePayload), API endpoints (ClusterTemplates.Get, HostTypes.List), fallback behavior for host type display names, parallel load order, and JSON payload examples. Edge cases addressed in the risks table: empty template (empty-state message, blocks proceed), template lookup failure (load error, blocks Next), template deleted after catalog item creation. References specific server-side code
Testability	2/2	Test plan updated with specific scenarios matching the new template-driven behavior: 'Cluster template node set left at zero/blank size' (blocks Next, inline error), 'Selected template has no node_sets' (empty-state, cannot proceed), back navigation verifying read-only host types are preserved, happy path cluster with template-driven rows rendering automatically. Adapter-specific test updated to assert payload map key = template's node-set key (not host type id). Manual smoke test updated. Compo
Scope	2/2	Tightly bounded revision — changes exactly one aspect (node_sets composition model from tenant-composed to template-driven) and updates all affected sections consistently across both PRD and design. PRD frontmatter prd: field present. Superseded non-goal clearly documented with rationale (server-side validation rejects non-template entries). Prior resolution preserved in a collapsed block for traceability. References the equivalent fix already applied (OSAC-2936, osac-ui#102). No scope creep.
Architecture	2/2	Correctly aligns the UI wizard with fulfillment-service's validation behavior — the design is driven by an actual server-side constraint (PrivateClustersServer.validateNodeSets rejects non-template node sets), not an arbitrary preference. Clean separation of concerns: ClusterTemplates.Get provides structure (keys, host types), HostTypes.List provides display names only. Create payload structure uses template's own keys as map keys (not host type id), matching what the server expects. Dependencie

Verdict: A well-executed, narrowly scoped revision that aligns the design with the actual server-side contract — architecturally sound, feasible, well-tested, and cleanly bounded.

Feedback: This revision is strong across all dimensions. Two minor improvements worth considering: (1) add a test scenario for switching between catalog items that reference different templates (verifying node-set rows re-render with the new template's keys/host types), and (2) briefly document the caching/re-fetch strategy for the ClusterTemplates.Get response — the design mentions 'reuse it if already fetched' but doesn't specify whether this is a React Query cache, a Formik-level memoization, or manual state. Neither is a blocker for merge.

Critical (0)

None.

Important (0)

None.

Suggestions (3)

1. Add a test scenario for switching catalog items that reference different templates — the current test plan covers 'Change catalog item after editing' (resets to getInitialValues) but doesn't explicitly assert that node-set rows re-render with the new template's keys and host types when the new catalog item references a different ClusterTemplate.
2. Clarify the caching mechanism for ClusterTemplates.Get — the design says 'reuse it if already fetched for catalog/General display' but doesn't specify whether this is a React Query cache hit, manual component state, or Formik-level storage. Specifying this would help implementers avoid redundant API calls or stale template data.
3. Consider noting behavior if a template is updated server-side while a wizard session is in progress — the current design doesn't mention this edge case, though it's unlikely to be hit in practice since wizard sessions are short-lived.

---

Review cost

Model: claude-opus-4-6
Cost: $0.4579
Tokens: 7 in / 4.5k out
Cache: 255.1k read
Active time: 1m 46s
API calls: 0