Skip to content

NO-ISSUE: catalog-items-ui: node_sets are template-driven, not freely composed - #167

Merged
openshift-merge-bot[bot] merged 1 commit into
osac-project:mainfrom
ElayAharoni:catalog-items-node-sets-template-scoped
Jul 28, 2026
Merged

NO-ISSUE: catalog-items-ui: node_sets are template-driven, not freely composed#167
openshift-merge-bot[bot] merged 1 commit into
osac-project:mainfrom
ElayAharoni:catalog-items-node-sets-template-scoped

Conversation

@ElayAharoni

@ElayAharoni ElayAharoni commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

  • fulfillment-service's PrivateClustersServer.validateNodeSets (internal/servers/private_clusters_server.go) rejects any node_sets entry whose map key isn't a key on the associated ClusterTemplate, or whose host type doesn't match the template's value for that key.
  • This design (enhancements/catalog-items/ui-design.md, the design doc for OSAC-2936 / OSAC-2936: add catalog item create wizard for Cluster, VM, and Bare Metal osac-ui#102) previously had the map key "auto-derived from the host type" and had NodeSetsFieldEditor reuse the tenant wizard's freely add/remove ClusterNodeSetsArrayField — neither holds once that backend validation is accounted for.
  • Revises the NodeSetsFieldEditor section to match what PR OSAC-2402: add AGENTS.md for AI agent onboarding #102 actually implements: the editor takes the selected ClusterTemplate as a prop and renders one fixed row per template node-set key, host type shown read-only, only size admin-editable. Removes the "Allow add/remove" toggle (no longer a real capability) and updates the Formik state shape, payload example, Yup validation, and test plan.

Context

Surfaced from a review comment on osac-project/osac-ui#102 (batzionb) and confirmed against fulfillment-service's current validation logic. Companion doc update for the tenant-facing wizard (OSAC-1421) is in #166.

Assisted-by: Claude Code noreply@anthropic.com

Summary by CodeRabbit

  • Documentation
    • Updated catalog item administration guidance for template-driven node set configuration.
    • Clarified that node set rows and host types are inherited from the selected template, while administrators provide only default sizes.
    • Documented validation and serialization rules for template-aligned node set data.
    • Added test coverage requirements for the updated editor behavior, empty states, validation, and payload formatting.

fulfillment-service's PrivateClustersServer.validateNodeSets (internal/
servers/private_clusters_server.go) rejects any node_sets entry whose map
key isn't a key on the associated ClusterTemplate, or whose host type
doesn't match the template's value for that key. This design previously had
the map key "auto-derived from the host type" and had NodeSetsFieldEditor
reuse the tenant wizard's freely add/remove ClusterNodeSetsArrayField —
neither holds once that backend validation is in effect.

Revises the NodeSetsFieldEditor section to match what OSAC-2936 (PR osac-project#102)
actually implements: the editor takes the selected ClusterTemplate as a
prop and renders one fixed row per template node-set key, host type shown
read-only, only size admin-editable. Removes the "Allow add/remove" toggle
(no longer a real capability once node sets can't be added or removed) and
updates the Formik state shape, payload example, Yup validation, and test
plan accordingly.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The catalog item UI design now defines node_sets as fixed, template-derived rows with read-only host types, size-only input, template-keyed serialization, dynamic Yup validation, and updated component tests.

Changes

Catalog node set editor

Layer / File(s) Summary
Template-driven editor contract
enhancements/catalog-items/ui-design.md
Updates the wizard, field definitions, NodeSetsFieldEditor, validation, drawbacks, metadata, and test plan to describe template-keyed node set rows, read-only host types, positive-size validation, and serialized payloads using template keys.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: tchughesiv, rawagner

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
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 Scanned the only changed file; found no hardcoded secrets, embedded-credential URLs, private keys, or suspicious secret literals.
No-Weak-Crypto ✅ Passed Only a markdown design doc changed; no MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or custom crypto code appears.
No-Injection-Vectors ✅ Passed Only a Markdown design doc changed; no executable code paths or injection-prone constructs were introduced.
Container-Privileges ✅ Passed Only a markdown design doc changed; no container/K8s manifests or privilege settings were present.
No-Sensitive-Data-In-Logs ✅ Passed Changed file is design docs only; no new logging of secrets/PII was introduced, and the only log references explicitly mention status codes and redacted bodies.
Ai-Attribution ✅ Passed AI use is explicitly mentioned, and the commit includes the required Red Hat 'Assisted-by' trailer; no AI-related Co-Authored-By was found.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: node_sets are now template-driven rather than freely composed.
✨ 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.

@openshift-ci openshift-ci Bot added the lgtm label Jul 28, 2026
@batzionb
batzionb marked this pull request as ready for review July 28, 2026 07:20
@openshift-ci
openshift-ci Bot requested review from adriengentil and eranco74 July 28, 2026 07:20
coderabbitai[bot]
coderabbitai Bot previously requested changes Jul 28, 2026

@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/catalog-items/ui-design.md`:
- Around line 510-516: Update the NodeSetsFieldEditor example to pass the
selected ClusterTemplate through its required template prop instead of rendering
<NodeSetsFieldEditor /> without arguments. Use the example’s existing
selected-template value so the editor can render the template’s fixed node-set
rows.
- Around line 556-573: Update the dynamically generated nodeSetsSchema
validation so each size value must be a positive integer, not a fractional
number. Pass the selected template’s sizeMin and sizeMax into the schema and
enforce those configured bounds for every key, then add component tests covering
fractional values and below/above-bound values.
🪄 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: fa988f56-04a3-4983-8290-25e150233701

📥 Commits

Reviewing files that changed from the base of the PR and between bb96154 and fc08dbb.

📒 Files selected for processing (1)
  • enhancements/catalog-items/ui-design.md

Comment thread enhancements/catalog-items/ui-design.md
Comment thread enhancements/catalog-items/ui-design.md
@ElayAharoni ElayAharoni changed the title catalog-items-ui: node_sets are template-driven, not freely composed NO-ISSUE: catalog-items-ui: node_sets are template-driven, not freely composed Jul 28, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@ElayAharoni: This pull request explicitly references no jira issue.

Details

In response to this:

Summary

  • fulfillment-service's PrivateClustersServer.validateNodeSets (internal/servers/private_clusters_server.go) rejects any node_sets entry whose map key isn't a key on the associated ClusterTemplate, or whose host type doesn't match the template's value for that key.
  • This design (enhancements/catalog-items/ui-design.md, the design doc for OSAC-2936 / OSAC-2936: add catalog item create wizard for Cluster, VM, and Bare Metal osac-ui#102) previously had the map key "auto-derived from the host type" and had NodeSetsFieldEditor reuse the tenant wizard's freely add/remove ClusterNodeSetsArrayField — neither holds once that backend validation is accounted for.
  • Revises the NodeSetsFieldEditor section to match what PR OSAC-2402: add AGENTS.md for AI agent onboarding #102 actually implements: the editor takes the selected ClusterTemplate as a prop and renders one fixed row per template node-set key, host type shown read-only, only size admin-editable. Removes the "Allow add/remove" toggle (no longer a real capability) and updates the Formik state shape, payload example, Yup validation, and test plan.

Context

Surfaced from a review comment on osac-project/osac-ui#102 (batzionb) and confirmed against fulfillment-service's current validation logic. Companion doc update for the tenant-facing wizard (OSAC-1421) is in #166.

Assisted-by: Claude Code noreply@anthropic.com

Summary by CodeRabbit

  • Documentation
  • Updated catalog item administration guidance for template-driven node set configuration.
  • Clarified that node set rows and host types are inherited from the selected template, while administrators provide only default sizes.
  • Documented validation and serialization rules for template-aligned node set data.
  • Added test coverage requirements for the updated editor behavior, empty states, validation, and payload formatting.

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

openshift-ci Bot commented Jul 28, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: batzionb, ElayAharoni

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-merge-bot
openshift-merge-bot Bot merged commit 3a00a0b into osac-project:main Jul 28, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants