OSAC-2553: Design: Catalog Items — UI Management - #128
openshift-merge-bot[bot] merged 28 commits into
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a design proposal for administering Cluster, ComputeInstance, and BareMetalInstance catalog items through role-gated admin pages, shared kind-aware API hooks, reusable field and validation editors, and documented security, testing, and operational requirements. ChangesCatalog Items UI Management
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AI Design Review: EP-128Score: 6/8 | Verdict: PASS
Verdict: A strong UI-focused design with excellent implementation detail and architecture, held back slightly by missing formal user stories and an incomplete test strategy that lacks unit test coverage. Feedback: Add a User Stories subsection under Motivation with formal 'As a [role], I want to [action] so that I can [goal]' stories for each persona workflow — the detailed workflows are already there, they just need the structured format the template requires. Strengthen the test plan by adding a unit test section covering Yup validation schemas, CatalogItemKindConfig mapping logic, and FieldMask construction in the update hook; remove the 'if adopted' hedge on component-level testing. Address the Documentation dimension explicitly, even if just to defer it (e.g., 'Admin documentation for catalog management deferred to docs follow-up task'). Critical (0)None. Important (5)
Suggestions (3)
Review costModel: claude-opus-4-6 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/catalog-items/ui-design.md`:
- Around line 541-545: Update the “Failure detection” support procedure to
prohibit logging full API response bodies. In the Go proxy API failure logging,
retain status codes but log the request ID and a sanitized error code instead,
with response bodies redacted by default.
- Around line 196-205: The useAllCatalogItems hook currently loads every catalog
item from all three endpoints without bounds. Update useAllCatalogItems and the
underlying per-kind queries to use pagination or bounded infinite queries with
server-side search and status filters, while preserving the unified
CatalogItemWithKind results and kind discriminator.
- Around line 336-337: The path validation regex in the Yup schema must reject
empty segments and trailing dots while preserving valid dot-notation paths.
Update the path rule to validate each segment independently, or reuse the
server’s canonical path validator, so values like “a..b” and “a.” fail
client-side validation.
- Around line 149-151: Update the AdminRoute guard to allow access only for
providerAdmin and tenantAdmin roles. Redirect tenantUser and any other
authenticated or unexpected roles to /catalog, and explicitly handle
unauthenticated users according to the existing authentication flow before
rendering admin pages.
- Around line 42-43: Align the catalog UI design with the tenancy contract: at
enhancements/catalog-items/ui-design.md lines 42-43, remove the blanket
private-API prohibition; at lines 97-104, route CRUD through role-appropriate
private or public endpoints; at lines 243-246, replace heuristic scope detection
with an explicit authoritative scope field or API; and at lines 404-409,
document provider-admin/private-API versus tenant-admin/public-API authorization
boundaries.
- Around line 270-280: Update the create payload documentation near the payload
construction to include the selected provider-admin scope as an explicit tenant
field. Ensure tenant-scoped submissions send the selected tenant identifier and
Global selections use the API’s designated global representation, preserving the
existing POST endpoint behavior.
- Around line 294-296: Clarify the PATCH contract for the repeated
field_definitions field in the form submission description: specify whether
updates replace the entire list or support item-level changes, including reorder
and removal behavior. Ensure the documented update_mask semantics match that
choice rather than implying diff-based field updates cover all list edits.
🪄 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: 502dab04-52c8-4903-8041-8ff813910a5b
📒 Files selected for processing (1)
enhancements/catalog-items/ui-design.md
AI Design Review: EP-128Score: 6/8 | Verdict: PASS
Verdict: A well-detailed UI design with strong architecture and feasibility, held back by a missing User Stories section required by the template, unaddressed cross-cutting dimensions, and an uncertain component-level testing commitment for the most complex new components. Feedback: Add a User Stories subsection under Motivation with stories for Cloud Provider Admin, Tenant Admin, and Tenant User (the workflow descriptions already contain the content — just reformat). Explicitly address or defer the tenant onboarding, documentation, and installation dimensions from osac-dimensions.md. Commit to component-level tests for FieldDefinitionsEditor and ValidationConstraintsEditor — remove the 'if adopted' hedge and specify what unit tests cover (e.g., add/remove/reorder field definitions, Yup validation for non-editable fields without defaults, JSON Schema construction from structured inputs). Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 553-555: Make ValidationConstraintsEditor component testing
mandatory rather than conditional, and require coverage for serialization,
nested constraints, empty schemas, and resource-reference enforcement, including
its recursive schema and custom resourceRef behavior.
- Around line 362-365: Define a single wire format for validationSchema across
the editor and form/API contract, explicitly choosing the serialized
representation at the boundary. Specify how structured validation data is
serialized for create/update requests and parsed back for editing, ensuring
persisted values and submitted payloads use the same type consistently.
- Around line 375-382: Update the resourceRef constraint design in the
surrounding validation/provisioning flow to require server-side enforcement by
the fulfillment service. Define a validation and rejection path for unsupported
or violated resourceRef constraints, ensuring provisioning cannot silently
ignore resource-type restrictions while preserving the existing UI
resource-selection behavior.
🪄 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: 3b3b37d2-574c-4318-a4b7-14aacb003c6e
📒 Files selected for processing (1)
enhancements/catalog-items/ui-design.md
AI Design Review: EP-128Score: 6/8 | Verdict: PASS
Verdict: A thorough, implementation-ready UI design with strong architecture and feasibility, held back by a missing User Stories section and a hedged testing strategy for the most complex new components. Feedback: Add the template-required User Stories subsection under Motivation with formal 'As a [role]...' stories for each persona workflow — the workflow descriptions are excellent but the structured format helps reviewers confirm persona coverage. Commit to a concrete component/unit test plan for FieldDefinitionsEditor and ValidationConstraintsEditor instead of qualifying it with 'if adopted' — these are the riskiest new components and need specified test scenarios (e.g., add/remove/reorder field definitions, JSON Schema construction from structured inputs, Yup validation for dot-notation paths). Address the Documentation dimension from osac-dimensions.md explicitly, even if deferred. Critical (0)None. Important (4)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-128Score: 6/8 | Verdict: PASS
Verdict: A well-crafted UI design document with exceptional implementation detail and sound architectural choices, held back by a missing User Stories section and a conditional (rather than committed) component test strategy. Feedback: Add a User Stories subsection under Motivation with formal 'As a [role], I want to [action] so that I can [goal]' stories for Cloud Provider Admin, Tenant Admin, and Tenant User — the workflow descriptions are excellent but don't replace structured user stories per the template. Commit to component-level tests for FieldDefinitionsEditor and ValidationConstraintsEditor rather than hedging with 'if adopted' — these are the most complex new components and need dedicated test coverage. Address or explicitly defer the Tenant Onboarding, Installation (Go proxy route configuration), and Documentation cross-cutting dimensions from osac-dimensions.md. Critical (0)None. Important (5)
Suggestions (3)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
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/catalog-items/ui-design.md (1)
201-219: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftExpose pagination state in the aggregate hook contract.
useAllCatalogItems(): UseQueryResult<CatalogItemWithKind[]>describes a flat query, but the design requires three independently paginated streams plusLoad more/infinite-scroll behavior. Define an aggregate return type exposing per-kind continuation state,hasNextPage, fetch actions, and deterministic merge ordering; otherwise later pages can be omitted or duplicated.🤖 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/catalog-items/ui-design.md` around lines 201 - 219, Update the useAllCatalogItems contract in catalog-item-admin.ts to return an aggregate result rather than a flat UseQueryResult. Expose independently tracked pagination state for each CatalogItemKind, aggregate hasNextPage status, fetch-more actions, and deterministic merged ordering so Load more/infinite-scroll requests append each stream exactly once without omissions or duplicates.
🤖 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 282-292: Update the create-payload documentation section around
the nested JSON code fence by adding a blank line immediately before the opening
fence and after the closing fence, without changing the payload content.
- Around line 362-368: Define an explicit, enforceable subset of JSON Schema
constraints for Tenant Admin tighten-only validation, including patterns, enums,
bounds, nested schemas, and resourceRef. Specify canonical normalization and
comparison rules for determining equality or stricter constraints, and require
the server to reject unsupported or unprovably tighter schemas rather than
accepting them. Ensure the UI and fulfillment service use the same contract.
---
Outside diff comments:
In `@enhancements/catalog-items/ui-design.md`:
- Around line 201-219: Update the useAllCatalogItems contract in
catalog-item-admin.ts to return an aggregate result rather than a flat
UseQueryResult. Expose independently tracked pagination state for each
CatalogItemKind, aggregate hasNextPage status, fetch-more actions, and
deterministic merged ordering so Load more/infinite-scroll requests append each
stream exactly once without omissions or duplicates.
🪄 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: 92161c3f-85f8-4527-b7fa-e4c58db7a495
📒 Files selected for processing (1)
enhancements/catalog-items/ui-design.md
AI Design Review: EP-128Score: 6/8 | Verdict: PASS
Verdict: A well-structured UI design with deep implementation detail and sound architecture, held back by missing user stories, incomplete test strategy (no unit/integration test plan), and gaps in cross-cutting dimension coverage (documentation, installation). Feedback: Add a User Stories subsection under Motivation with 'As a [persona], I want to [action] so that [goal]' stories for Cloud Provider Admin, Tenant Admin, and Tenant User — the workflows describe HOW but not the user motivation WHY. Expand the test plan to include unit tests (catalogItemKinds config, FieldMask construction, Yup schema validation) and commit to component-level tests for FieldDefinitionsEditor and ValidationConstraintsEditor rather than making them conditional. Address the documentation and installation dimensions explicitly — even if deferred, state that admin guides and Go proxy configuration changes are out of scope for this design. Critical (0)None. Important (4)
Suggestions (5)
Review costModel: claude-opus-4-6 |
|
|
||
| - Reuse existing osac-ui patterns (ListPage, OsacForm, Formik + Yup, TanStack React Query hooks, PatternFly table/kebab actions) wherever possible. [Codebase: libs/ui-components/] | ||
| - Establish a role-gated navigation pattern using the existing `navRowsForRole()` function and `useSession()` hook that future admin features can follow. | ||
| - Use a single polymorphic component set for all three catalog item types (Cluster, ComputeInstance, BareMetalInstance) rather than separate implementations per type. |
There was a problem hiding this comment.
Even though we used this pattern for catalog item provisioning wizard, it was just a short-term solution.
Lets not do this for the new stuff.
Instead use more React-idiomatic approach using JSX composition which is easier to read and handles future per-kind divergence naturally.
So instead of one big component, lets create multiple shared ones - like common Wizard pages/Form fields where each kind-specific wizard uses these pages to build the full Wizard.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Line 432: Update the test plan and graduation criteria in the design document
to make fulfillment-service backend enforcement a release gate: require
implementation of the resourceRef validator or pre-validation step and an
integration test demonstrating that CLI/API provisioning rejects invalid
resource references before the feature graduates.
- Around line 67-73: Update the provider-admin flow’s template-detail population
references and row typing to use fields and types derived from the selected
resource spec, not template parameter definitions or parameter types. Keep
templates limited to supplying compatible defaults or metadata, and ensure the
design consistently preserves the resource-spec-driven field set.
- Around line 223-228: Update UseAllCatalogItemsResult and its hook
implementation to expose pagination state for each resource kind, including
per-kind hasNextPage and fetchNextPage behavior, so callers can advance only the
relevant paginated query. Alternatively, explicitly define aggregate semantics
for the existing fields and add coverage confirming no items are missed or
requests duplicated.
🪄 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: 0fb8e22a-9eec-4b4e-850f-4a2fea8e93f1
📒 Files selected for processing (1)
enhancements/catalog-items/ui-design.md
|
|
||
| For fields with a `resourceRef` constraint, the UI fetches available resources from the corresponding API endpoint and presents them as selectable options. `resourceRef` is an OSAC-specific custom keyword within the JSON Schema `validation_schema`; standard JSON Schema validators ignore it. | ||
|
|
||
| **Dependency: server-side enforcement.** The `resourceRef` keyword is only enforced by the UI dropdown today. For the feature to be safe to ship, fulfillment-service must register a custom JSON Schema keyword validator (or a dedicated pre-validation step) that resolves `resourceRef` against the actual resource type inventory during provisioning. Without this backend enforcement, resource-type restrictions set through the UI are cosmetic — they constrain the dropdown in the browser but are not enforced when users submit via CLI or API directly. The UI work can proceed in parallel, but the feature must not ship without the backend `resourceRef` validator landing first. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Make backend resourceRef enforcement a release gate.
The design correctly says the feature must not ship without fulfillment-service enforcement, but the test plan and graduation criteria do not require the validator or an integration test proving that CLI/API provisioning rejects invalid resource references. Add both before this feature can graduate.
🤖 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/catalog-items/ui-design.md` at line 432, Update the test plan
and graduation criteria in the design document to make fulfillment-service
backend enforcement a release gate: require implementation of the resourceRef
validator or pre-validation step and an integration test demonstrating that
CLI/API provisioning rejects invalid resource references before the feature
graduates.
AI Design Review: EP-128Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured UI design document (662 lines) that provides deep implementation detail, concrete test plans, and honest open questions — one of the stronger designs in the OSAC enhancement-proposals corpus. Feedback: Two items worth tracking before implementation: (1) Open Question #1 on scope visibility in public API responses should be resolved with the API team early, since it affects the CSP Admin list page's Scope column — the design may need revision depending on the answer. (2) Consider enumerating the initial spec fields per resource type (at least top-level fields from ComputeInstanceSpec, ClusterSpec, BareMetalInstanceSpec) in the design or a linked reference to reduce implementation ambiguity — currently the specFields.ts file is mentioned but its contents are undefined. Also, the prd frontmatter references 'README.md' which is a generic filename — update to the actual PRD path in enhancement-proposals. Critical (0)None. Important (2)
Suggestions (4)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-128Score: 8/8 | Verdict: PASS
Verdict: A thorough, implementation-ready UI design that covers all CRUD workflows for three resource types across three personas, with detailed component specifications, comprehensive error handling, and a concrete test strategy — the strongest areas are the FieldDefinitionsEditor specification and the four well-reasoned alternatives. Feedback: Two open questions need resolution before implementation begins: (1) how CSP Admin derives scope from public API responses (Open Question 1 — the Tenant Admin scope derivation via 'server-authored capability metadata or creators/tenants fields' is vague and needs a concrete answer from the API team), and (2) whether CEL filter support exists for the Provisioned Resources tab (Open Question 3 — define a fallback or degrade gracefully if unsupported). Additionally, the tighten-only enforcement being UI-only in 0.2 is a security gap worth tracking — until server-side enforcement lands in 0.3, CLI/API users can bypass constraint restrictions entirely, so consider adding a Jira ticket for the 0.3 server-side work as a hard dependency. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
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 397-406: Update the Advanced mode flow for tenant-derived items so
Tenant Admins cannot bypass tighten-only restrictions: either disable Advanced
editing for these items or validate the submitted schema against the base schema
before submission. Ensure empty, removed, or weakened constraints are rejected
while preserving Advanced mode for items not derived from a tenant base.
- Line 464: Update Advanced mode JSON validation to require a non-null object
root, rejecting arrays and scalar values before form submission serialization.
Preserve acceptance of empty content as no validation and keep the existing
inline error behavior for invalid input, ensuring the parsed value passed as
validationSchema is always compatible with google.protobuf.Struct.
🪄 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: 5ae529bc-cfb7-4c7c-b6eb-630f75ec7ad8
📒 Files selected for processing (1)
enhancements/catalog-items/ui-design.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Feedback: CatalogItemForm is still too monolithic — decompose into JSX compositionSection §2 ("Catalog Item Type Abstraction — Shared Components via JSX Composition") says it uses JSX composition, but the actual example still delegates everything to a single polymorphic export const ClusterCatalogItemCreatePage = () => (
<CatalogItemForm
kind="cluster"
apiRoute="v1/cluster_catalog_items"
templateSelector={<TemplateSelector apiRoute="v1/cluster_templates" />}
specFields={CLUSTER_SPEC_FIELDS}
/>
);This is config-driven polymorphism with JSX syntax — the What I expect instead: each kind-specific page explicitly composes the form structure, making Formik, sections, and per-kind differences visible at the page level. Each page also owns its data fetching — shared components like // ClusterCatalogItemCreatePage.tsx
const ClusterCatalogItemCreatePage = () => {
const { data: templates, isLoading } = useClusterTemplates();
const { mutateAsync: createClusterCatalogItem } = useCreateClusterCatalogItem();
return (
<Formik
initialValues={clusterCatalogItemInitialValues}
validationSchema={clusterCatalogItemSchema}
onSubmit={(values) => createClusterCatalogItem(buildClusterPayload(values))}
>
<CatalogItemGeneralFields />
<TemplateSelector templates={templates} isLoading={isLoading} />
<FieldDefinitionsEditor fields={CLUSTER_SPEC_FIELDS} />
</Formik>
);
};
// BareMetalCatalogItemCreatePage.tsx
const BareMetalCatalogItemCreatePage = () => {
const { data: templates, isLoading } = useBareMetalInstanceTemplates();
const { mutateAsync: createBmCatalogItem } = useCreateBareMetalCatalogItem();
return (
<Formik
initialValues={bareMetalCatalogItemInitialValues}
validationSchema={bareMetalCatalogItemSchema}
onSubmit={(values) => createBmCatalogItem(buildBareMetalPayload(values))}
>
<CatalogItemGeneralFields />
<TemplateSelector templates={templates} isLoading={isLoading} />
<FieldDefinitionsEditor fields={BARE_METAL_SPEC_FIELDS} />
<BareMetalSpecificSection />
</Formik>
);
};The shared building blocks ( Why this matters:
Action: remove |
AI Design Review: EP-128Score: 8/8 | Verdict: PASS
Verdict: A strong, implementation-ready UI design document with exceptional detail across all criteria — specific component architecture, thorough lifecycle coverage, concrete test scenarios, and clear scope boundaries with real alternatives analyzed. Feedback: Resolve Open Question 1 (scope visibility in public API) before implementation begins — the CSP Admin list page's Scope column depends on this and could require architectural changes to the Go proxy if scope isn't derivable from public responses. Consider elevating the tighten-only UI-only enforcement (deferred server-side to 0.3) into the Risks table, since a Tenant Admin could bypass restrictions via direct API calls — this is a security-relevant gap worth highlighting for reviewers. Goal #4 about JSX composition is implementation-focused rather than user-visible; reframe as an outcome like 'Ensure consistent behavior across all three resource types.' Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-128Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured UI design document (830 lines) with deep implementation detail, comprehensive test coverage, and clear scope boundaries — one of the stronger designs in the reference library calibration range. Feedback: Two items to clean up before merge: (1) The Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Default values for pod_cidr, service_cidr, ssh_public_key, and pull_secret come only from the template or admin input — the UI only pre-populates validation schemas when the template does not provide them. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
AI Design Review: EP-128Score: 8/8 | Verdict: PASS
Verdict: A thorough, well-structured UI design document with exceptional implementation detail — specific file paths, component hierarchies, TypeScript interfaces, validation schemas, and error handling for all six failure modes — covering all lifecycle operations across three resource types with concrete test plans at three levels. Feedback: Two structural improvements: the PRD frontmatter reference points to 'README.md' instead of the OSAC-conventional 'prd.md', and user stories appear in the design's Motivation section when they should live in the PRD per OSAC conventions (the design should reference the PRD for persona coverage). Consider making the run_strategy enum casing consistent — VM uses 'Always'/'Halted' while Bare Metal uses 'ALWAYS'/'HALTED', which may reflect actual API values but should be verified and documented if intentional. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
AI EP Review: EP-128Score: 10/10 | Verdict: PASS
Verdict: A strong, comprehensive UI design document that clearly describes catalog item admin management across three resource types with well-defined persona workflows, concrete motivation, and an extremely detailed implementation approach including component architecture, validation schemas, and thorough test plans. Feedback: The PR includes an unrelated deletion of enhancements/storage-control-plane-osac-2872/prd.md — this should be in a separate PR or explained in the PR description to avoid confusion. Open Questions 1 (scope visibility in public API) and 3 (CEL filter for provisioned resources) are API team dependencies that could block core UX features (scope badges and the Provisioned Resources tab); consider resolving these before or in parallel with implementation. Minor inconsistency: run_strategy enum values differ between VM ('Always'/'Halted') and BareMetalInstance ('ALWAYS'/'HALTED') — verify this matches the proto definitions. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
Reverts the directory rename from commit 15533a3 so this branch only contains catalog-items changes. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Elay Aharoni <elayaha@gmail.com>
AI EP Review: EP-128Score: 9/10 | Verdict: PASS
Verdict: A thorough and well-structured UI design document with clear user outcomes, detailed implementation guidance, and strong testability. The only weakness is a generic business justification that describes the gap without quantifying the impact. Feedback: Strengthen the Motivation section by quantifying the admin pain: how many catalog item operations are performed today via CLI, what errors or friction does CLI-only management cause, and how does this block broader catalog adoption? Even a sentence like 'CSP Admins currently manage N catalog items via grpcurl, which requires proto expertise and offers no validation feedback' would elevate the WHY from 'gap exists' to 'gap hurts.' The technical design itself is exemplary — the per-kind composition pattern, field definition primitives, and failure handling table are exactly the right level of detail. Critical (0)None. Important (2)
Suggestions (2)
Review costModel: claude-opus-4-6 |
ElayAharoni
changed the title
|
@ElayAharoni: This pull request references OSAC-2553 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 task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: batzionb, ElayAharoni, rawagner The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…sac-project#128) catalog-items/ui-design.md (merged via osac-project#128 after our earlier passes) still linked to the pre-rename '/enhancements/cluster-and-vm-provisioning-wizard' path. Caught by a full-repo sweep (all file types, not just .md) across every retired directory name from the whole OSAC-2870 effort. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
…sac-project#128) catalog-items/ui-design.md (merged via osac-project#128 after our earlier passes) still linked to the pre-rename '/enhancements/cluster-and-vm-provisioning-wizard' path. Caught by a full-repo sweep (all file types, not just .md) across every retired directory name from the whole OSAC-2870 effort. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
…sac-project#128) catalog-items/ui-design.md (merged via osac-project#128 after our earlier passes) still linked to the pre-rename '/enhancements/cluster-and-vm-provisioning-wizard' path. Caught by a full-repo sweep (all file types, not just .md) across every retired directory name from the whole OSAC-2870 effort. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
4 participants
Design: Catalog Items — UI Management
EP: enhancements/catalog-items
Summary
This design adds admin management screens to osac-ui for catalog items across all three resource types (Cluster, ComputeInstance, BareMetalInstance). It introduces role-gated navigation, a FieldDefinitionsEditor component with structured validation constraints, and role-differentiated pages for Cloud Provider Admins and Tenant Admins. The design uses a single polymorphic component set for all three types via a CatalogItemKindConfig abstraction.
Requesting Review On
tenantfield? The design assumes scope is derivable from API responses.this.spec.catalog_item == "<id>"for the detail page's "Provisioned Resources" tab?How to Review
Summary by CodeRabbit
update_masksemantics, including publish/unpublish behavior.