Skip to content

OSAC-1110: UI design for storage tier management - #183

Merged
openshift-merge-bot[bot] merged 9 commits into
osac-project:mainfrom
ElayAharoni:design/OSAC-1110-storage-tier-ui
Aug 4, 2026
Merged

openshift-merge-bot[bot] merged 9 commits into
osac-project:mainfrom
ElayAharoni:design/OSAC-1110-storage-tier-ui

Conversation

@ElayAharoni

@ElayAharoni ElayAharoni commented Aug 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Adds enhancements/OSAC-1110-storage-tier/ui-design.md: the osac-ui implementation design for StorageTier (OSAC-1110) admin management, following this repo's existing ui-design.md companion-file convention (see OSAC-1002-catalog-items/ui-design.md, OSAC-1030-organizations/ui-design.md).
  • Scope: a single "Storage tiers" admin page. StorageBackend (OSAC-1111) gets no admin UI of its own in this design — neither PRD states a UI requirement, and it's consumed read-only (backend picker, name lookup), mirroring how NetworkClass is already handled with zero CRUD UI in osac-ui today.
  • Reintroduces role-gated admin navigation and routing in osac-ui — this codebase currently has no "Administration" nav section at all (it existed for an unrelated feature and was fully reverted); this design restores the same shape for "Storage tiers."
  • One open question remains for the OSAC-1110/1111 design owner (Roy Golan): whether the server enforces DNS-label formatting on StorageTier.metadata.name.

Context

  • Neither StorageBackend nor StorageTier has merged in fulfillment-service yet; both design.md docs are treated as a fixed contract. This UI work is blocked on that merge + a pnpm gen-types run in osac-ui, and is submitted now as a design-only doc for review.
  • Supersedes draft PR OSAC-1110, OSAC-1111: UI design for Storage Backend and Tier catalogs osac-ui#112, which published this content directly in the osac-ui repo before the decision to publish it here instead.

Test plan

  • Design doc reviewed by OSAC-1110/OSAC-1111 design owner (Roy Golan) for the remaining open question
  • No code changes in this PR — nothing to test

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added a complete design for Storage administration with Backends and Tiers views.
    • Defined full-page listing, creation, editing, and deletion workflows with validation and lifecycle status labels.
    • Limited tier creation and editing to a single backend association, while preserving future expansion options.
    • Added scoped backend lookup for tier listings and documented credential masking and deletion safeguards.
  • Documentation

    • Updated testing, deployment, version compatibility, security, support, and operational guidance for the Storage experience.

Specifies the osac-ui implementation for StorageTier (OSAC-1110) admin
management: a single "Storage tiers" admin page for composing named
tier offerings from registered StorageBackend (OSAC-1111) infrastructure.
StorageBackend gets no admin UI of its own -- neither PRD states a UI
requirement, and it is consumed read-only, mirroring how NetworkClass
is already handled with zero CRUD UI in osac-ui.

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

openshift-ci-robot commented Aug 3, 2026 •

Copy link
Copy Markdown

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

Details

In response to this:

Summary

  • Adds enhancements/OSAC-1110-storage-tier/ui-design.md: the osac-ui implementation design for StorageTier (OSAC-1110) admin management, following this repo's existing ui-design.md companion-file convention (see OSAC-1002-catalog-items/ui-design.md, OSAC-1030-organizations/ui-design.md).
  • Scope: a single "Storage tiers" admin page. StorageBackend (OSAC-1111) gets no admin UI of its own in this design — neither PRD states a UI requirement, and it's consumed read-only (backend picker, name lookup), mirroring how NetworkClass is already handled with zero CRUD UI in osac-ui today.
  • Reintroduces role-gated admin navigation and routing in osac-ui — this codebase currently has no "Administration" nav section at all (it existed for an unrelated feature and was fully reverted); this design restores the same shape for "Storage tiers."
  • One open question remains for the OSAC-1110/1111 design owner (Roy Golan): whether the server enforces DNS-label formatting on StorageTier.metadata.name.

Context

  • Neither StorageBackend nor StorageTier has merged in fulfillment-service yet; both design.md docs are treated as a fixed contract. This UI work is blocked on that merge + a pnpm gen-types run in osac-ui, and is submitted now as a design-only doc for review.
  • Supersedes draft PR OSAC-1110, OSAC-1111: UI design for Storage Backend and Tier catalogs osac-ui#112, which published this content directly in the osac-ui repo before the decision to publish it here instead.

Test plan

  • Design doc reviewed by OSAC-1110/OSAC-1111 design owner (Roy Golan) for the remaining open question
  • No code changes in this PR — nothing to test

🤖 Generated with Claude Code

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.

@coderabbitai

coderabbitai Bot commented Aug 3, 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

The proposal updates the osac-ui administration design for StorageBackend and StorageTier. It specifies full-page CRUD routes, single-backend tier interaction, scoped backend lookup, access control, validation, failure handling, testing, and version-skew requirements.

Changes

Storage administration UI

Layer / File(s) Summary
Administration scope and API integration
enhancements/OSAC-1110-storage-tier/ui-design.md
Defines full admin CRUD workflows, private service usage, one-backend tier associations, optimistic concurrency, immutable fields, and ID-scoped backend lookup.
Routing, resource pages, and forms
enhancements/OSAC-1110-storage-tier/ui-design.md
Specifies existing admin navigation, unconditional route registration, full-page backend and tier screens, list-shaped payloads, masked credentials, and lifecycle labels.
Validation, security, and failure handling
enhancements/OSAC-1110-storage-tier/ui-design.md
Defines client validation, credential handling, tenant invisibility of backend details, OPA authorization, permission errors, referential-integrity failures, and deferred multi-backend support.
Test and operational readiness
enhancements/OSAC-1110-storage-tier/ui-design.md
Updates unit, component, and E2E coverage, documentation, release and downgrade guidance, version-skew requirements, support procedures, and recovery guidance.

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

Possibly related PRs

Suggested reviewers: ronniel1, akshaynadkarni, batzionb

🚥 Pre-merge checks | ✅ 11
✅ Passed checks (11 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely identifies the OSAC-1110 change for the storage tier management UI design.
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 The PR adds one design document; scans found no API-key/token/password assignments, private-key material, credential-bearing URLs, or long base64/hex literals.
No-Weak-Crypto ✅ Passed Design document contains no recommendations for weak crypto (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom implementations, or insecure secret comparisons. The only encryption reference is `enc...
No-Injection-Vectors ✅ Passed PR adds only markdown design documentation (ui-design.md). No executable code, SQL, Python, JavaScript, or shell scripts present. No injection vectors possible in pure documentation.
Container-Privileges ✅ Passed The patch changes only a Markdown design document; it adds no container or Kubernetes manifest and contains no privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, or allowPrivilegeEscalation sett...
No-Sensitive-Data-In-Logs ✅ Passed PR adds only design documentation (.md files) with no executable code. No logging code patterns exist to review; check not applicable to documentation-only changes.
Ai-Attribution ✅ Passed The PR discloses Claude Code use, and five PR commits carry Assisted-by: Claude Code; no Co-Authored-By trailer appears in the PR commit range.
✨ 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.

Both StorageBackend (PR #728) and StorageTier (PR #832, restructured
by #887) merged in fulfillment-service weeks before this design was
drafted -- corrects the design's flat-field assumption to the actual
spec/status shape, resolves the DNS-label-validation open question by
reading the real server validation code (confirmed: not enforced
server-side), clarifies that the tenant-reference-blocks-delete path
is still genuinely deferred to OSAC-23, and notes the already-existing
osac CLI support for StorageBackend registration. No remaining
external blocker -- fulfillment-service protos and osac-ui's generated
types both already exist.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
@ElayAharoni
ElayAharoni marked this pull request as ready for review August 3, 2026 10:36
@openshift-ci
openshift-ci Bot requested review from mhrivnak and trewest August 3, 2026 10:36

@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: 7

🤖 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/OSAC-1110-storage-tier/ui-design.md`:
- Around line 256-258: Update the Version Skew Strategy to define the minimum
supported fulfillment-service version, or specify a capability gate covering
READY filtering, the one-backend constraint, field-mask semantics, and error
codes. Document and test the supported UI/service version skew matrix rather
than relying solely on protobuf additive compatibility.
- Around line 179-182: Add shared RFC 1035 DNS-label validation to the
server-side CreateStorageTier and UpdateStorageTier RPC handling, rejecting
invalid tier names while preserving the existing client-side validation. Update
the relevant server validation path, such as validateStorageTierCreate and its
update equivalent, and add API-level negative tests covering invalid names
through both RPCs.
- Line 153: Update the data-model code fence in the markdown document to specify
its language, using text or the exact syntax after the opening fence so
markdownlint no longer reports MD040.
- Around line 137-139: Update the StorageTierCreateModal design and acceptance
criteria to address FR-3: either define explicit release acceptance criteria
documenting the v0.1 single-backend limitation and deviation, or expand the
backend picker and submit flow to support one or more backend associations. Keep
the request payload’s backend association structure aligned with the selected
scope and update the related validation expectations.
- Around line 137-139: Standardize the storage-tier quota contract on the PRD’s
byte-based field by replacing quotaGib with the chosen byte-valued
BackendAssociation field across the generated proto, server validation/API
handling, StorageTierCreateModal payload, UI documentation, and affected tests.
Update labels, validation, serialization, and assertions consistently while
preserving positive-integer validation and the existing single-backend
submission shape.
- Around line 115-116: The useUpdateStorageTier() design must not send indexed
spec.backends[0].* paths in update_mask. For the v0.1 single-backend constraint,
update the request to send the complete spec.backends value with a valid
update_mask path such as spec.backends, or provide verified server-side support
and tests for indexed paths before using them.
- Around line 126-129: Update the navRowsForRole entry and matching AppShell
route to allow only providerAdmin, removing tenantAdmin unless the OPA policy
explicitly defines it as an equivalent platform role. Adjust the role-matrix
tests to verify tenantAdmin is denied and providerAdmin retains access.
🪄 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: bb69bb59-0c75-41ee-beeb-d0132797287d

📥 Commits

Reviewing files that changed from the base of the PR and between 2d8f396 and cd7f1cc.

📒 Files selected for processing (1)
  • enhancements/OSAC-1110-storage-tier/ui-design.md

Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
@ElayAharoni
ElayAharoni requested review from akshaynadkarni, batzionb, rawagner and ronniel1 and removed request for mhrivnak and trewest August 3, 2026 10:50
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated

## Motivation

OSAC has no API-managed inventory of storage tier offerings today: tier configuration lives in the `STORAGE_TIERS` environment variable plus Kubernetes label conventions, invisible to the OSAC API and to any UI. `StorageTier` (this EP) replaces that with a DB-backed private gRPC resource binding a named offering to a registered `StorageBackend` with QoS properties. `osac-ui` is the only *graphical* interface Cloud Provider Admins have to manage this data — neither `StorageTier` nor `StorageBackend` (OSAC-1111) has a public API, though both already have private `osac` CLI support (`osac create/describe storagetier`, `osac create/describe storagebackend`, confirmed merged in `fulfillment-service`).

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.

Worth mentioning that storage tiers and backends are merged and available in the private api , and that this is PR [1] will wire the storagetier info into AAP templates.

[1] https://github.com/osac-project/osac-operator/pull/375/changes - this change will get ported to the mono-repo.

… primitive

- StorageTierStateLabel now wraps the shared ResourceStatusLabel/StatusKind
  primitive (ACTIVE -> ready, UNSPECIFIED -> unspecified) instead of a
  standalone component, per user direction.
- Fix invalid FieldMask usage: standard protobuf FieldMask has no syntax
  for indexed repeated-element paths (spec.backends[0].*) -- corrected to
  submitting the complete spec.backends array with update_mask:
  ["spec.backends"].
- Restrict the "Storage tiers" nav entry and route to providerAdmin only;
  tenantAdmin is a tenant-scoped role in osac-ui's role model and has no
  legitimate access to a platform-only resource like StorageTier.
- Add a minimum-fulfillment-service-version note to Version Skew Strategy,
  since additive proto compatibility doesn't guarantee the specific
  validation/filter/error-code behaviors this design depends on.
- Flag missing server-side DNS-label validation as a backend follow-up
  (out of this UI design's scope) rather than silently accepting the gap.
- Acknowledge the v0.1 single-backend UI as a documented deviation from
  OSAC-1110 FR-3's "one or more backends" language; whether to build
  multi-select ahead of the server is left open pending reviewer discussion.
- Fix markdownlint MD040 (unlabeled code fence).

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Per PR review consensus (ronniel1/rgolangh confirming multiple backends
per tier is the intended design, backed by storage_tier_type.pb.go's
repeated Backends field), replace the single-select backend picker
with a repeatable BackendAssociationsArrayField in both Create and Edit
forms, following the existing ClusterNodeSetsArrayField.tsx pattern
(hand-rolled setFieldValue array manipulation, FormFieldGroup rows,
add/remove buttons, rowId-based keying) -- no other repeatable-array
pattern exists in osac-ui to diverge from.

This targets the full OSAC-1110 FR-3 requirement ("one or more backend
associations"), ahead of the server: private_storage_tiers_server.go's
validateBackends still rejects more than one entry as of this commit.
Documented as an explicit, expected interim gap (Risks and Mitigations,
new Failure Handling row) rather than silently building past it.

Also updates: List page BACKENDS/PROTOCOL(S) columns to show all
associations per tier, duplicate-backend-across-rows validation, and
corresponding Alternatives/Test Plan sections.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.com>
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
- Fix stale OSAC-23 references (closed) -> OSAC-2872 (Storage Control
  Plane, the live epic that owns tenant-to-tier assignment and the
  deferred referential-integrity trigger).
- Fix the mermaid sequence diagram, which wasn't rendering on GitHub --
  adopted the plain-text-safe syntax from review (no colons/braces/
  parens inside arrow messages).
- Incorporate OSAC-3014 (Public Storage Tier API): a tenant-facing
  Get/List API for StorageTier is planned separately to support VMaaS
  disk-tier selection (OSAC-1710). Clarified throughout that this
  design's "no public API" framing is current-state, not permanent,
  and that a future public surface is explicitly out of this design's
  scope rather than contradicting it.
- Clarify two wording nits: "hand-crafting API requests" and the
  UI-agnostic meaning of "catalog" in this document.
- Add Open Questions with one entry: whether StorageBackend should
  also get an admin UI, per OSAC-1111 design owner's PR feedback that
  the real admin workflow is register-backend-then-compose-tier and a
  Tier-only UI is incomplete. Cross-referenced from the relevant
  Non-Goals/Alternatives/Drawbacks entries as contested, not settled.

Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Elay Aharoni <elayaha@gmail.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

Caution

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

⚠️ Outside diff range comments (1)
enhancements/OSAC-1110-storage-tier/ui-design.md (1)

112-115: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Define bounded pagination for the tier list.

Line [112] says that the list page renders every tier without pagination, although the API contract supports offset and limit. A default or maximum service limit can hide tiers. Fetching all tiers without a bound can also create an unbounded request and render cost. Use page controls or fetch all pages with an explicit bound, and test a result set larger than one service page.

🤖 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/OSAC-1110-storage-tier/ui-design.md` around lines 112 - 115,
Update the storage-tier list flow around usePrivateStorageTiers(params) so it
does not render an unbounded, single-page result. Add page controls or bounded
multi-page fetching with an explicit maximum, and ensure tiers beyond one
service page remain handled according to that bound. Add coverage using a result
set larger than one service page.
♻️ Duplicate comments (1)
enhancements/OSAC-1110-storage-tier/ui-design.md (1)

194-195: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Enforce the DNS-label rule in the service.

Line [194] makes client validation the only guard. CLI and direct private API callers can persist invalid names, which can later break StorageClass generation. Add the same RFC 1035 validation to CreateStorageTier and UpdateStorageTier, with API-level negative tests. Keep client validation for early feedback.

🤖 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/OSAC-1110-storage-tier/ui-design.md` around lines 194 - 195, Add
RFC 1035 DNS-label validation to the service-layer CreateStorageTier and
UpdateStorageTier handlers, rejecting invalid metadata.name values before
persistence while retaining the existing client-side validation. Reuse the
project’s established DNS-label validator if available, and add API-level
negative tests covering invalid names for both operations.
🤖 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/OSAC-1110-storage-tier/ui-design.md`:
- Around line 264-269: Add coverage for successful create and update operations
using two backend associations, asserting the complete ordered backend array is
preserved. Add a component or transport contract test and, behind the
fulfillment-service capability gate, an integration or E2E case; retain the
existing interim multi-backend INVALID_ARGUMENT rejection test.
- Line 137: Replace quotaGib with the PRD-aligned quota field and byte unit
throughout StorageTierCreateModal, its submitted payload, generated types,
fulfillment-service contract, validation, and tests. Ensure validation and UI
labels/input handling represent bytes consistently, and update assertions and
fixtures to use the standardized field without changing other
backend-association behavior.

---

Outside diff comments:
In `@enhancements/OSAC-1110-storage-tier/ui-design.md`:
- Around line 112-115: Update the storage-tier list flow around
usePrivateStorageTiers(params) so it does not render an unbounded, single-page
result. Add page controls or bounded multi-page fetching with an explicit
maximum, and ensure tiers beyond one service page remain handled according to
that bound. Add coverage using a result set larger than one service page.

---

Duplicate comments:
In `@enhancements/OSAC-1110-storage-tier/ui-design.md`:
- Around line 194-195: Add RFC 1035 DNS-label validation to the service-layer
CreateStorageTier and UpdateStorageTier handlers, rejecting invalid
metadata.name values before persistence while retaining the existing client-side
validation. Reuse the project’s established DNS-label validator if available,
and add API-level negative tests covering invalid names for both operations.
🪄 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: 59c04521-a2ae-4480-9769-af54d141c74f

📥 Commits

Reviewing files that changed from the base of the PR and between fad1362 and b268aa5.

📒 Files selected for processing (1)
  • enhancements/OSAC-1110-storage-tier/ui-design.md


#### 4. Create Form

A modal (`StorageTierCreateModal`) modeled on `osac-ui`'s existing `VirtualNetworkCreateModal` (Formik + Yup, single mutation on submit): `name` (DNS-label validated, §8), `description` (optional), and a repeatable **backend associations** section — one or more rows, each with `backend` (select, populated from `usePrivateStorageBackends({ filter: STORAGE_BACKEND_READY_LIST_FILTER })`), `protocol` (`NFS`/`BLOCK`), `maxReadBandwidthMbs` / `maxWriteBandwidthMbs` / `quotaGib` (positive-integer numeric fields), and `encryptionEnabled` (checkbox). Submits `{ metadata: { name }, spec: { description, backends: [{ backendId, protocol, maxReadBandwidthMbs, maxWriteBandwidthMbs, quotaGib, encryptionEnabled }, ...] } }` — one array entry per row.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Align quota with the PRD byte contract.

Line [137] uses quotaGib, while enhancements/OSAC-1110-storage-tier/prd.md defines quota in bytes. Standardize the field name and unit across the UI payload, generated types, fulfillment-service contract, validation, and tests. Otherwise the service can reject the field or interpret values with a factor-of-2³⁰ difference.

🤖 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/OSAC-1110-storage-tier/ui-design.md` at line 137, Replace
quotaGib with the PRD-aligned quota field and byte unit throughout
StorageTierCreateModal, its submitted payload, generated types,
fulfillment-service contract, validation, and tests. Ensure validation and UI
labels/input handling represent bytes consistently, and update assertions and
fixtures to use the standardized field without changing other
backend-association behavior.

Comment on lines +264 to +269
- Create form: DNS-label and positive-integer validation, backend picker excludes non-`READY` backends, `ALREADY_EXISTS`/`NOT_FOUND`/multi-backend `INVALID_ARGUMENT` error surfacing.
- `BackendAssociationsArrayField`: adding a row appends an empty one with a fresh `rowId`; removing a row is unavailable on the first row and removes any other row without affecting sibling rows' state (verifying `rowId`-based keying, not index-based, avoids cross-row state bleed); a backend already selected in one row is excluded from every other row's options; array-level Yup validation rejects zero rows and duplicate `backendId`s.
- Edit form: prefill renders one row per existing `spec.backends` entry, `name` rendered disabled, each row's backend picker includes that row's non-`READY` currently-assigned backend, QoS-change alert triggers correctly, stale-version conflict handling, adding/removing rows on an existing tier submits the complete updated array.

**E2E tests** (owned by QE, authored in `osac-test-infra` via the `/e2e` workflow — `osac-ui` has no e2e tests of its own):
- Full create → list → edit → delete flow against a real (or kind-deployed) fulfillment-service, including duplicate-name rejection and delete-blocked-by-tenant-reference.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy lift

Add a successful multi-backend contract test.

The test plan covers duplicate exclusion and the interim INVALID_ARGUMENT, but it does not verify successful create or update with two backend associations. Add a component or transport test for the complete ordered array, plus an integration or E2E case behind the fulfillment-service capability gate. Keep the interim rejection test.

🤖 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/OSAC-1110-storage-tier/ui-design.md` around lines 264 - 269, Add
coverage for successful create and update operations using two backend
associations, asserting the complete ordered backend array is preserved. Add a
component or transport contract test and, behind the fulfillment-service
capability gate, an integration or E2E case; retain the existing interim
multi-backend INVALID_ARGUMENT rejection test.

Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
Reverses the earlier Tier-only scoping (which treated StorageBackend
as read-only support data, like NetworkClass). The OSAC-1111 design
owner confirmed in review that the real Cloud Provider Admin workflow
is incomplete without backend registration, editing, and deletion
being reachable through the same UI: register a StorageBackend first,
then compose StorageTiers on top of it. This UI now builds full CRUD
for both resources under a combined "Storage" admin section.
…r review

Verified rawagner's review comments against the current osac-ui codebase
(this branch was ~30 commits behind origin/main and missing the recent
UserRole/Tenant-admin-nav work):

- No per-route role guard exists anywhere in osac-ui; /admin/tenants/*
  is registered unconditionally, gated only by hiding the nav entry.
  Removed the route-level guard this design had proposed and rewrote
  Navigation and Routing, RBAC/Tenancy, and Security Considerations to
  match, extending the live nav-administration section instead of
  "reintroducing removed code."
- Replaced the stale providerAdmin/tenantAdmin/tenantUser role model
  throughout with the current, real UserRole type.
- Every create flow built since the original Networking feature is a
  full page, not a modal (VmCreatePage, ClusterCreatePage,
  BareMetalCreatePage, TenantCreatePage). Converted the Backend/Tier
  Create and Edit forms from modals to pages to match.
- The Tiers list page's backend-name lookup now scopes its List call
  to referenced IDs via a new CEL filter instead of fetching every
  registered backend; flagged the Create/Edit forms' backend picker
  as inheriting an app-wide unpaginated-List limitation, not unique
  to this design.

Also applied CodeRabbit's outstanding "add a successful multi-backend
contract test" finding to the Test Plan.
@ElayAharoni
ElayAharoni requested a review from rawagner August 4, 2026 12:15
…sion

Storage-scope call (2026-08-04) confirmed:
- CSP admin manages both StorageBackend and StorageTier via UI (already
  reflected in this design).
- spec.backends stays list-shaped in the data model and API payload, but
  the UI should only let the admin choose one backend per tier for now,
  matching the server's current single-backend restriction rather than
  building ahead of it. Replaced the repeatable BackendAssociationsArrayField
  component with direct backends[0].* field bindings, keeping the Formik
  state and submitted payload list-shaped for a UI-only migration path
  when multi-select is eventually prioritized.
- This phase covers CSP admin only; tenant consumption of storage tiers
  (OSAC-1710/OSAC-3014) is explicitly a later phase.
- Tenants must remain completely unaware of the StorageBackend concept —
  only StorageTier. Recorded as a binding constraint on OSAC-3014's future
  public StorageTier API (must never expose spec.backends/backendId to
  tenants), and folded into Motivation and Security Considerations.

@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/OSAC-1110-storage-tier/ui-design.md`:
- Line 191: Define the UpdateStorageBackend credential semantics for
spec.credentials so blank fields preserve existing values or, alternatively,
require both username and password whenever credentials are changed. Update the
relevant update-handler or service logic and add tests covering username-only,
password-only, both fields, and blank credential submissions, while preserving
the documented edit behavior.
🪄 Autofix

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: CHILL

Plan: Pro Plus

Run ID: c0a21eb9-3b86-4d1b-abca-e48527365963

📥 Commits

Reviewing files that changed from the base of the PR and between c68a5fd and 42232c9.

📒 Files selected for processing (1)
  • enhancements/OSAC-1110-storage-tier/ui-design.md

Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md Outdated
CodeRabbit flagged that blank username/password on the Backend edit
form was documented as independently optional (blank = unchanged),
but UpdateStorageBackend's update_mask addresses spec.credentials as
a whole nested message, not its individual leaf fields -- so a
username-only or password-only submission had undefined behavior.

Resolved by treating credentials.username/credentials.password as an
all-or-nothing pair on edit: both blank omits spec.credentials from
the payload entirely (unchanged), both filled submits a complete
replacement object, and filling in only one is rejected client-side
before submission. This matches the same whole-value-under-the-mask
convention already used for spec.backends elsewhere in this design.
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md
Comment thread enhancements/OSAC-1110-storage-tier/ui-design.md

@akshaynadkarni akshaynadkarni left a comment

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.

Left a couple of comments. Nothing to block the PR. PTAL.

Overall LGTM.

@openshift-ci

openshift-ci Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akshaynadkarni, 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-ci openshift-ci Bot added the approved label Aug 4, 2026
@openshift-merge-bot
openshift-merge-bot Bot merged commit 2be106c into osac-project:main Aug 4, 2026
5 checks passed
openshift-merge-bot Bot added a commit that referenced this pull request Aug 5, 2026
…llowups

OSAC-1110: storage UI design doc follow-ups (post-#183)
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.

6 participants