OSAC-3538: PRD - Catalog Items v2 — Field Governance Redesign - #195
Conversation
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Avishay Traeger <atraeger@redhat.com>
|
@avishayt: This pull request references OSAC-3538 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. 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. |
|
Warning Review limit reached
Next review available in: 55 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe PRD defines Catalog Items v2 field governance for compute, cluster, and bare-metal items. It covers typed locked or editable fields, overlay behavior, template validation, referential integrity, scope boundaries, assumptions, UI dependencies, and provenance. ChangesCatalog Items v2 requirements
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 EP Review: EP-195Score: 9/10 | Verdict: PASS
Verdict: Strong PRD with a clear capability, concrete justification, focused scope, and fully testable requirements — held back from a perfect score only by implementation terminology (proto fields, database-level referential integrity, typed map, behavior enum) leaking into what should be user-facing descriptions. Feedback: Replace implementation terms in In Scope with user-facing language: 'strongly-typed proto fields' → 'structured, typed fields'; 'Database-level referential integrity prevents deletion' → 'The system prevents deletion of resources referenced by catalog items'; 'typed map validated against' → describe the user-visible governance behavior; 'behavior enum' → 'behavior model'. Also remove the '[User]' markers on In Scope items and some user stories — these appear to be drafting artifacts that should be cleaned before merge. Critical (0)None. Important (2)
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: 4
🧹 Nitpick comments (1)
enhancements/OSAC-3538-catalog-items-v2/prd.md (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the non-UI dependencies.
The scope requires proto/API changes, template parameter definitions, and reference deletion enforcement, but the Dependencies section lists only
osac-ux.Add high-level owners for the catalog-item API/schema, template definitions, and resource lifecycle. Keep these dependencies at PRD level. Put implementation verification in the design document.
Based on learnings: dependency bullets in PRDs should remain high-level.
🤖 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-3538-catalog-items-v2/prd.md` around lines 61 - 64, Expand the Dependencies section in the PRD beyond osac-ux to include high-level owners for catalog-item API/schema changes, template parameter definitions, and reference deletion enforcement/resource lifecycle. Keep these as PRD-level dependencies and leave implementation verification details to the design document.Source: Learnings
🤖 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-3538-catalog-items-v2/prd.md`:
- Line 54: Update the catalog-item provisioning story to require redaction or
omission of sensitive resource fields such as pull_secret in the Tenant User
view, even when locked; define secret inputs as write-only, while keeping
non-sensitive locked values visible and read-only.
- Line 18: Update the template-parameter section of the PRD to define when
validation occurs across catalog-item creation, update, and provisioning.
Document the user-visible rejection behavior for invalid scalar values, missing
required parameters, unknown parameters, and parameter definitions that changed
after configuration.
- Around line 15-20: The PRD must define an explicit upgrade contract before
replacing field_definitions: document how persisted catalog fields map to the
new strongly typed fields, the migration/API compatibility behavior for existing
catalog items, precedence among catalog defaults, tenant input, locked values,
and template defaults during provisioning, and the outcome when a required value
is missing. Cover this contract for ComputeInstanceCatalogItem,
ClusterCatalogItem, and BareMetalInstanceCatalogItem.
- Around line 19-20: Replace the database-level referential-integrity
requirement in the catalog-item deletion rules with observable server behavior:
reject deletion of any referenced images, instance types, or
BareMetalInstanceCatalogItem templates. Explicitly define coverage for published
and unpublished global and tenant-scoped catalog items, and state when
protection ends after unpublishing or deleting a catalog item; apply the same
clarification to the related requirement at line 42.
---
Nitpick comments:
In `@enhancements/OSAC-3538-catalog-items-v2/prd.md`:
- Around line 61-64: Expand the Dependencies section in the PRD beyond osac-ux
to include high-level owners for catalog-item API/schema changes, template
parameter definitions, and reference deletion enforcement/resource lifecycle.
Keep these as PRD-level dependencies and leave implementation verification
details to the design document.
🪄 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: 86875f99-06c9-4a5e-b934-93886ca04869
📒 Files selected for processing (1)
enhancements/OSAC-3538-catalog-items-v2/prd.md
| - Catalog items become an overlay on existing resource creation — fields not mentioned in the catalog item behave as if no catalog item exists. [User] | ||
| - Spec fields on each catalog item type are strongly-typed proto fields with a per-field behavior (locked or editable with a default). [User] | ||
| - Per-field type customization: fields can use richer types than the underlying resource spec (e.g., instance type as an enum with curated options, image as a mandatory field with a reference selector). [User] | ||
| - Template parameters are governed separately via a typed map validated against the referenced template's parameter definitions. [User] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced PRD and nearby parameter validation mentions.
if [ -f "enhancements/OSAC-3538-catalog-items-v2/prd.md" ]; then
wc -l enhancements/OSAC-3538-catalog-items-v2/prd.md
sed -n '1,90p' enhancements/OSAC-3538-catalog-items-v2/prd.md
else
echo "Referenced PRD not found"
git ls-files | rg 'catalog|template|parameter|prd\.md|OSAC-3538' || true
fi
echo "---- focused search for validation/parameter/template definitions ----"
rg -n "Template parameters|parameter definitions|catalog-item|catalog item|provision|validation|reject|unknown|required|error|invalid" enhancements/OSAC-3538-catalog-items-v2/prd.md . --glob '*.md' --glob '!vendor/**' --glob '!node_modules/**' | head -n 200Repository: osac-project/enhancement-proposals
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# No external tools are needed for this review; stop here and wait for capability results.Repository: osac-project/enhancement-proposals
Length of output: 172
Define rejection behavior for invalid template parameters.
The PRD says template parameters are validated against the template definition, but it does not state how the system handles invalid scalar values, missing required parameters, unknown parameters, or changed parameter definitions.
Define the validation point — catalog-item creation, catalog-item update, provisioning, or all three — and document the user-visible failure outcome for each invalid case.
🤖 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-3538-catalog-items-v2/prd.md` at line 18, Update the
template-parameter section of the PRD to define when validation occurs across
catalog-item creation, update, and provisioning. Document the user-visible
rejection behavior for invalid scalar values, missing required parameters,
unknown parameters, and parameter definitions that changed after configuration.
| - Database-level referential integrity prevents deletion of resources (images, instance types) referenced by catalog items. [User] | ||
| - Applies to all three catalog item types: ComputeInstanceCatalogItem, ClusterCatalogItem, and BareMetalInstanceCatalogItem. [User] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate prd and baremetal readme files =="
git ls-files | rg 'enhancements/(OSAC-3538-catalog-items-v2/prd.md|OSAC-1118-baremetal-instance-api/README\.md)$' || true
echo
echo "== OSAC-3538 prd relevant sections =="
if [ -f enhancements/OSAC-3538-catalog-items-v2/prd.md ]; then
nl -ba enhancements/OSAC-3538-catalog-items-v2/prd.md | sed -n '1,90p'
fi
echo
echo "== OSAC-1118 BareMetalInstanceCatalogItem refs around lines 230-262 =="
if [ -f enhancements/OSAC-1118-baremetal-instance-api/README.md ]; then
nl -ba enhancements/OSAC-1118-baremetal-instance-api/README.md | sed -n '220,275p'
fi
echo
echo "== searches for catalog item delete/reference protection text =="
rg -n "referential|integrity|delet(e|ion)|template|ComputeInstanceCatalogItem|ClusterCatalogItem|BareMetalInstanceCatalogItem" enhancements -S || trueRepository: osac-project/enhancement-proposals
Length of output: 402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OSAC-3538 prd relevant sections =="
if [ -f enhancements/OSAC-3538-catalog-items-v2/prd.md ]; then
awk '{printf "%6d\t%s\n", NR, $0}' enhancements/OSAC-3538-catalog-items-v2/prd.md | sed -n '1,100p'
fi
echo
echo "== OSAC-1118 BareMetalInstanceCatalogItem refs around lines 230-262 =="
if [ -f enhancements/OSAC-1118-baremetal-instance-api/README.md ]; then
awk '{printf "%6d\t%s\n", NR, $0}' enhancements/OSAC-1118-baremetal-instance-api/README.md | sed -n '220,275p'
fi
echo
echo "== searches for catalog item delete/reference protection text =="
rg -n "referential|integrity|delet(e|ion)|template|ComputeInstanceCatalogItem|ClusterCatalogItem|BareMetalInstanceCatalogItem" enhancements -S || true
echo
echo "== exact text around OSAC-3538 lines 15-25 and 38-46 =="
awk 'NR>=15 && NR<=25 {printf "%6d\t%s\n", NR, $0} NR>=38 && NR<=46 {printf "%6d\t%s\n", NR, $0}' enhancements/OSAC-3538-catalog-items-v2/prd.mdRepository: osac-project/enhancement-proposals
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OSAC-3538 prd exact relevant blocks =="
awk 'NR>=15 && NR<=28 {printf "%6d\t%s\n", NR, $0} NR>=38 && NR<=48 {printf "%6d\t%s\n", NR, $0}' enhancements/OSAC-3538-catalog-items-v2/prd.md
echo
echo "== OSAC-3538 prd all delete/reference protection mentions =="
rg -n "referential|integrity|delet(e|ion)|image|instance type|instance type|template|published|unpublished|global|organization" enhancements/OSAC-3538-catalog-items-v2/prd.md -S
echo
echo "== osac-1118 baremetal README exact relevant blocks =="
awk 'NR>=220 && NR<=270 {printf "%6d\t%s\n", NR, $0}' enhancements/OSAC-1118-baremetal-instance-api/README.md
echo
echo "== osac-1118 baremetal delete/topic mentions =="
rg -n "delete|deletion|integrity|template|BareMetalInstanceCatalogItem|published|unpublished" enhancements/OSAC-1118-baremetal-instance-api/README.md -S
echo
echo "== OSAC-1002 catalog README delete/template protection exact blocks =="
awk 'NR>=436 && NR<=446 {printf "%6d\t%s\n", NR, $0}' enhancements/OSAC-1002-catalog-items/README.mdRepository: osac-project/enhancement-proposals
Length of output: 18063
Define deletion protection without mandating database implementation.
Lines 19 and 42 require a database-level referential-integrity mechanism. State the observable server behavior instead. Also cover templates: BareMetalInstanceCatalogItem references template, and OSAC-1002 says catalog-item-referenced templates must be rejected from deletion. Define whether the rule applies to published and unpublished global and tenant-scoped items, and whether protection drops after a catalog item is unpublished or deleted.
Also applies to: lines 25-25, 42-42
🤖 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-3538-catalog-items-v2/prd.md` around lines 19 - 20, Replace
the database-level referential-integrity requirement in the catalog-item
deletion rules with observable server behavior: reject deletion of any
referenced images, instance types, or BareMetalInstanceCatalogItem templates.
Explicitly define coverage for published and unpublished global and
tenant-scoped catalog items, and state when protection ends after unpublishing
or deleting a catalog item; apply the same clarification to the related
requirement at line 42.
Source: Learnings
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Meeting Alignment Review
The strongly-typed direction aligns well with what was discussed in the meeting. A few questions about gaps between the PRD and the meeting discussion.
| Category | Count |
|---|---|
| 🟡 Important | 3 |
| 💡 Suggestion | 2 |
|
|
||
| - As a Cloud Provider Admin, I want to create a catalog item by selecting which resource fields are locked vs. editable using a structured form that shows the actual resource fields — not freeform path inputs — so that I cannot accidentally reference invalid fields. | ||
|
|
||
| - As a Cloud Provider Admin, I want locked fields to have pre-defined values that are enforced when a tenant provisions from the catalog item, so that I can enforce guardrails (e.g., lock image to a specific version, restrict instance type to approved sizes). |
There was a problem hiding this comment.
🟡 Is image always locked, or is it the admin's choice?
In the meeting, the consensus was that disk images should be locked within the catalog item with a 1-to-1 relationship - a catalog item is created per image. This user story frames image locking as an example of what an admin CAN do ("lock image to a specific version"), but the meeting decision seemed to be that image is always locked by design - the tenant user selects a catalog item, not an image.
@avishayt was the intent that image is always locked, or that admins have the option to lock it?
There was a problem hiding this comment.
Image is always locked from the tenant's perspective during provisioning — the tenant selects a catalog item, not an image. But the catalog item owner (Cloud Provider Admin or Tenant Admin) can update the image on the catalog item itself (e.g., to bump versions for CVE fixes). Updated the PRD with two separate stories to make this clear.
There was a problem hiding this comment.
So this means enabling setting the image in CLI and backend doesn't make sense, correct? It will always be rejected because the catalogitem doesn't allow editing it
|
|
||
| ## In Scope | ||
|
|
||
| - Catalog items become an overlay on existing resource creation — fields not mentioned in the catalog item behave as if no catalog item exists. [User] |
There was a problem hiding this comment.
🟡 Should the PRD state that a catalog item reference is mandatory for resource creation?
In the meeting (00:18:49), the team confirmed that the current design requires a catalog item reference for resource deployment and agreed this should remain in place. The overlay model here describes what happens when a catalog item is used, but doesn't say whether referencing one is mandatory or optional.
@avishayt should this be captured as an In Scope item or an Assumption?
There was a problem hiding this comment.
This is existing behavior — a catalog item reference is already mandatory for resource creation, and we're not changing that. Added it as an Assumption to make it explicit.
|
|
||
| ### Tenant Admin | ||
|
|
||
| - As a Tenant Admin, I want to create organization-scoped catalog items using the same field governance model as global items, so that I can tailor offerings for my organization. |
There was a problem hiding this comment.
🟡 Tenant-provided images
In the meeting (00:14:21), the team agreed that tenants need to bring their own VM images (similar to uploading OVAs in VMware), and the system must support both provider-supplied and tenant-provided images. This story covers creating org-scoped catalog items but doesn't address the bring-your-own-image workflow.
@avishayt is this intentionally simplified for this iteration? If so, worth calling it out in Out of Scope or as a known gap so it doesn't get lost.
There was a problem hiding this comment.
Tenant-provided images are covered by a separate proposal. Added it to Out of Scope with a note that this PRD assumes tenants can reference both provider-supplied and tenant-provided images in catalog items.
| - Spec fields on each catalog item type are strongly-typed proto fields with a per-field behavior (locked or editable with a default). [User] | ||
| - Per-field type customization: fields can use richer types than the underlying resource spec (e.g., instance type as an enum with curated options, image as a mandatory field with a reference selector). [User] | ||
| - Template parameters are governed separately via a typed map validated against the referenced template's parameter definitions. [User] | ||
| - Database-level referential integrity prevents deletion of resources (images, instance types) referenced by catalog items. [User] |
There was a problem hiding this comment.
💡 Deprecation vs. deletion blocking
In the meeting (00:42:50), the discussion leaned toward a deprecation and obsolescence process rather than simply blocking deletion of referenced resources. The "prevent deletion" approach here seems like a reasonable starting point. Worth noting the connection to the Out of Scope lifecycle feature (draft/active/deprecated/retired) so readers can see these are related.
There was a problem hiding this comment.
Good point. Updated the In Scope deletion blocking bullet to note that deletion blocking is the immediate behavior, and a deprecation/obsolescence model may replace or complement it when the lifecycle feature is implemented.
| - Lifecycle management and versioning (draft/active/deprecated/retired states, version pinning) — separate feature; the design must be extensible to support this. | ||
| - Multi-resource composition (catalog items that bundle multiple resources with dependency ordering) — will likely use a different mechanism, not catalog items. | ||
| - Post-provisioning governance (restricting what a tenant can modify on a resource after provisioning) — separate feature. | ||
| - Cost metadata, metering/usage tracking, discoverability metadata (categories, tags) — separate features. |
There was a problem hiding this comment.
💡 Billing and field locking
In the meeting (00:19:37, 00:49:45), there was discussion that certain fields must be locked to ensure accurate billing calculations, and that locking decisions should be driven by billing requirements. Billing/cost metadata is correctly Out of Scope here, but should the PRD note that billing requirements may constrain which fields must be locked?
There was a problem hiding this comment.
Agree that billing may constrain which fields must be locked. Added a note to the cost metadata Out of Scope bullet: "billing requirements may constrain which fields must be locked; those constraints will be captured in the design when the billing feature is specified." The specific constraints are a design-level detail that doesn't belong in this PRD.
|
|
||
| ### Cloud Provider Admin | ||
|
|
||
| - As a Cloud Provider Admin, I want to create a catalog item by selecting which resource fields are locked vs. editable using a structured form that shows the actual resource fields — not freeform path inputs — so that I cannot accidentally reference invalid fields. |
There was a problem hiding this comment.
Worth mentioning that cloud provider admin should be able to select the tenant to assign the catalogitem to.
Also about publish/unpublish
There was a problem hiding this comment.
Added a user story: "As a Cloud Provider Admin, I want to assign a catalog item to a specific tenant and control its visibility via publish/unpublish, so that I can target offerings to the right audience."
- Image is mandatory and always locked during provisioning; owner can update it - Added tenant assignment and publish/unpublish story - Added pre-GA assumption (no migration needed) - Added catalog item reference mandatory assumption - Connected deletion blocking to lifecycle out-of-scope - Added billing constraint note to cost metadata out-of-scope - Added tenant-provided images out-of-scope with link to PR osac-project#145 - Cleaned up design leakage and removed [User] markers Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Avishay Traeger <atraeger@redhat.com>
AI EP Review: EP-195Score: 10/10 | Verdict: PASS
Verdict: A clean, well-structured PRD that clearly describes the user-facing need for structured catalog item governance, with concrete justification, no design leakage, focused scope, and fully testable requirements. Feedback: Strong PRD overall. Two minor improvements: (1) explicitly declare the affected services (BMaaS, CaaS, VMaaS) rather than leaving them implicit through the catalog item type names, and (2) remove the Provenance section before merging — it's AI workflow metadata outside the PRD template's sections. The Out of Scope section is particularly well done, naming concrete separate features rather than vague exclusions. Critical (0)None. Important (0)None. Suggestions (2)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
enhancements/OSAC-3538-catalog-items-v2/prd.md (1)
61-61:⚠️ Potential issue | 🟠 MajorRedact secret fields from the Tenant User view.
Line 61 requires the Tenant User to see the full resource configuration, including locked values. Resource configuration can include
pull_secret; displaying it as read-only would expose the secret. Require redaction or omission for secret fields and write-only handling for secret inputs.🤖 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-3538-catalog-items-v2/prd.md` at line 61, Update the catalog-item provisioning requirement for the Tenant User view to redact or omit secret fields such as pull_secret, while retaining read-only display for non-secret locked values. Define secret inputs as write-only and ensure their values are never exposed in the rendered resource configuration.
🤖 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-3538-catalog-items-v2/prd.md`:
- Line 20: Update the deletion-protection requirements around the catalog item
lifecycle statements to include templates alongside images and instance types.
Define rejection behavior for referenced templates, images, and instance types
across published and unpublished global and tenant-scoped catalog items, and
specify when protection ends after unpublishing or deleting a catalog item.
- Line 19: Expand the template-parameter requirements near the referenced
template-definition validation to specify when validation occurs during
catalog-item creation, update, and provisioning, and define rejection behavior
for invalid scalar values, missing required parameters, and unknown parameters.
Also specify how existing configurations behave when the referenced template’s
parameter definitions change, including whether they are revalidated or
rejected.
- Line 68: Update the PRD statement around the OSAC pre-GA compatibility policy
to separately specify whether clients using the old API can call the new API and
the failure they receive, and whether persisted catalog items containing
field_definitions remain readable or must be recreated before rollout. Replace
the combined ambiguity with explicit observable outcomes for both API
compatibility and persisted-data handling.
---
Duplicate comments:
In `@enhancements/OSAC-3538-catalog-items-v2/prd.md`:
- Line 61: Update the catalog-item provisioning requirement for the Tenant User
view to redact or omit secret fields such as pull_secret, while retaining
read-only display for non-secret locked values. Define secret inputs as
write-only and ensure their values are never exposed in the rendered resource
configuration.
🪄 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: 1d7ce2f2-163c-494c-9a7d-9ccf2d5704a2
📒 Files selected for processing (1)
enhancements/OSAC-3538-catalog-items-v2/prd.md
|
|
||
| - Catalog items become an overlay on existing resource creation — fields not mentioned in the catalog item behave as if no catalog item exists. | ||
| - Spec fields on each catalog item type are structured, typed fields with a per-field behavior (locked or editable with a default). | ||
| - Image is mandatory and always locked on a catalog item — tenants cannot change the image during provisioning. The catalog item owner can update the image (e.g., to bump versions for CVE fixes). |
There was a problem hiding this comment.
This is clear for ComputeInstance and BareMetalInstance, which both have a direct spec.image field. However, ClusterCatalogItem has no image field — the closest equivalent is spec.version_name, which replaces release_image IIRC.
- For ClusterCatalogItem, should version_name be the mandatory locked field (the cluster equivalent of "image")?
- If so, should the PRD say "image or version" instead of just "image" to cover all three types?
- Does the referential integrity requirement (prevent deletion of referenced resources) also apply to ClusterVersions referenced by catalog items?
There was a problem hiding this comment.
Good catch. ClusterCatalogItem doesn't have an image equivalent — updated the PRD to clarify that image is mandatory for ComputeInstanceCatalogItem and BareMetalInstanceCatalogItem only. Referential integrity still applies for whatever resources the cluster catalog item references (e.g., the CaaS equivalent of instance type). The design will enumerate the exact fields per catalog item type.
| - Spec fields on each catalog item type are structured, typed fields with a per-field behavior (locked or editable with a default). | ||
| - Image is mandatory and always locked on a catalog item — tenants cannot change the image during provisioning. The catalog item owner can update the image (e.g., to bump versions for CVE fixes). | ||
| - Per-field type customization: fields can use richer types than the underlying resource spec (e.g., instance type as an enum with curated options, image as a mandatory reference selector). | ||
| - Template parameters are governed with the same locked/editable behavior as spec fields, validated against the referenced template's parameter definitions. |
There was a problem hiding this comment.
One thing to consider: if a template removes or renames a parameter that a catalog item currently governs (e.g., a catalog item locks infra_env_name, then the template drops that parameter), the catalog item becomes inconsistent. If that inconsistency is visible to users (provisioning fails, admin sees a broken catalog item), it might be worth adding a user story to the PRD to cover that scenario. If it's purely an internal concern, then the design is the right place for it.
There was a problem hiding this comment.
This is a valid concern. If a template removes or renames a parameter that a catalog item governs, that's a non-backward-compatible template change — and provisioning will fail. This is analogous to any API-breaking change: the cloud provider broke the contract by changing the template. The design should address how to detect and surface this inconsistency, but it doesn't warrant a PRD-level user story since it's an error scenario, not a capability.
|
|
||
| ## In Scope | ||
|
|
||
| - Catalog items become an overlay on existing resource creation — fields not mentioned in the catalog item behave as if no catalog item exists. |
There was a problem hiding this comment.
This is a significant behavioral reversal from v1. This means that an admin can create a catalog-item with only the image/version_name locked, and then the user can manage all the remaining fields even if they're not listed in the catalog?
This also means that if an admin wants a tightly controlled offering, they must know every spec field and explicitly govern them one by one. If they miss a field, the tenant has full control over it. There's no way to say "lock everything by default, only allow these fields as editable." Is this the intended UX?
There was a problem hiding this comment.
Yes, this is the intended UX and one of the main drivers of this change, requested by the UI team. Every resource has tenant-specific parameters (e.g., networking configuration) that don't make sense to lock or even specify in the catalog item — the catalog item governs the offering-specific fields (image, instance type, etc.) while leaving tenant-specific choices to the tenant.
Only the subset of spec fields relevant to catalog governance are exposed as typed fields on the catalog item — fields like network attachments are not governable through catalog items. A "lock everything by default" mode could be added later if needed, but we don't see it as necessary given that the governed field set is explicitly defined per resource type.
| ## In Scope | ||
|
|
||
| - Catalog items become an overlay on existing resource creation — fields not mentioned in the catalog item behave as if no catalog item exists. | ||
| - Spec fields on each catalog item type are structured, typed fields with a per-field behavior (locked or editable with a default). |
There was a problem hiding this comment.
All the fields in the spec will be managed or just a subgroup?
There was a problem hiding this comment.
Only the subset of spec fields relevant to catalog governance will be exposed — not every field in the spec. Fields like network attachments, template reference, and catalog item reference are not governable. Template parameters are governed via a key-value map. The design will enumerate the exact fields per resource type. Updated the PRD to clarify this.
|
|
||
| ### Tenant User | ||
|
|
||
| - As a Tenant User, I want to see the full resource configuration when provisioning from a catalog item — locked values displayed as read-only, editable values pre-filled with defaults I can change, and ungoverned fields available as normal — so that I understand what I am getting. |
There was a problem hiding this comment.
This is clear but a single story. What about managing (update, unpublish, delete) org-scoped catalog items?
There was a problem hiding this comment.
Good point. Updated the story to: "As a Tenant Admin, I want to create, update, unpublish, and delete organization-scoped catalog items..." — these operations already exist today and are retained.
- Clarify image is mandatory for VM/BM only, not clusters - Specify only governance-relevant spec fields are exposed - Note template parameters governed via key-value map - Expand Tenant Admin story to cover update/unpublish/delete Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Avishay Traeger <atraeger@redhat.com>
There was a problem hiding this comment.
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-3538-catalog-items-v2/prd.md`:
- Line 17: Update the user story’s image requirement to apply only to
ComputeInstanceCatalogItem and BareMetalInstanceCatalogItem, or explicitly
qualify it as “where applicable,” while preserving the existing exclusion of
ClusterCatalogItem.
🪄 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: 72da7c3d-e624-4bc5-b724-dbf3c4afea30
📒 Files selected for processing (1)
enhancements/OSAC-3538-catalog-items-v2/prd.md
…tems Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Avishay Traeger <atraeger@redhat.com>
AI EP Review: EP-195Score: 10/10 | Verdict: PASS
Verdict: A strong, well-scoped PRD that clearly describes the catalog item field governance redesign from the user's perspective, with concrete pain points, complete persona coverage, and fully testable requirements. Feedback: This is a high-quality PRD ready for design. Two minor polish items: (1) The Out of Scope extensibility notes ('the behavior model must be extensible to support it') are design-directed constraints — consider softening to 'the design should accommodate future hidden-field behavior' or moving these to the design EP as requirements. (2) The In Scope reference to 'spec fields' could be rephrased as 'resource fields' for pure user-facing language, though in OSAC's Kubernetes-native context this is a marginal concern. Critical (0)None. Important (0)None. Suggestions (2)
Review costModel: claude-opus-4-6 |
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Re-Review: All Previous Findings Addressed
All five findings from my initial review have been addressed. The PRD now clearly defines the overlay model, scopes image locking correctly to VM and bare metal types, captures the catalog item reference mandate as an assumption, and draws clean boundaries between PRD-level requirements and design-level details.
| # | Finding | Status |
|---|---|---|
| 1 | 🟡 Image always locked vs. admin's choice | FIXED - Two separate user stories now distinguish provisioning-time locking from admin-side image updates. |
| 2 | 🟡 Catalog item reference mandatory | FIXED - Added as Assumption (line 67). |
| 3 | 🟡 Tenant-provided images | FIXED - Added to Out of Scope with cross-reference to PR #145. |
| 4 | 💡 Deprecation vs. deletion blocking | FIXED - In Scope bullet now connects to the Out of Scope lifecycle feature. |
| 5 | 💡 Billing and field locking | FIXED - Out of Scope cost metadata bullet now notes billing constraints. |
No new issues found.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avishayt, tzvatot 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 |
| ## In Scope | ||
|
|
||
| - Catalog items become an overlay on existing resource creation — fields not mentioned in the catalog item behave as if no catalog item exists. | ||
| - Spec fields on each catalog item type are structured, typed fields with a per-field behavior (locked or editable with a default). Only the subset of spec fields relevant to catalog governance are exposed — fields like network attachments are not governable through catalog items. The design will enumerate the exact fields per resource type. Template parameters are governed via a key-value map. |
There was a problem hiding this comment.
When a catalog item locks a field, the intent is clear for creation - the tenant can't set a value. But what about patch/update?
If locked means creation-only, tenants can work around the lock by patching the field after creation. If locked means always locked, how do we handle resource evolution?
For example, a ClusterCatalogItem that locks
release_image would prevent tenants from upgrading their control plane
There was a problem hiding this comment.
You're absolutely right. I put this as out-of-scope:
"Post-provisioning governance (allowed operations on created resources, field visibility after creation, update restrictions) — follow-up feature; the design must be extensible to support this"
I wanted to keep this feature slim, but we will definitely need that follow-up feature.
There was a problem hiding this comment.
I think this mixes two different things:
- Creation: the catalog item governs field values at provisioning time. "Locked" = tenant can't choose a different value.
- Update/upgrade: day-2 operation on an existing resource (e.g., osac edit cluster --version 4.21 or oc adm upgrade --to 4.21). This doesn't go through the catalog item it's a direct resource update.
So locking version at creation doesn't block upgrades, they're separate API calls.
That said, the question of what happens when a tenant updates a field that was locked at creation is real and needs an explicit answer, even if it lands in the post-provisioning governance follow-up.
There was a problem hiding this comment.
@alosadagrande @avishayt I agree these are separate API paths, but that's exactly the concern - if "locked" only governs creation, a tenant can create a resource and immediately patch the locked field to a different value. The lock becomes a suggestion, not enforcement.
Today, some fields are already immutable post-creation (catalog_item, template, template_parameters, instance_type on compute instances), but that's per-field hardcoding in the Update handler - not driven by the catalog item's governance. A field like image that an admin locked in the catalog item is not in that immutable list.
I'm not saying we need to solve full post-provisioning governance now. But we need to decide: when an admin locks a field in a catalog item, does the system enforce that lock on updates too? If yes, the Update path needs to check the catalog item's governance - and we should design for that now even if we implement it later. If no, we should document that lock is creation-only and accept the workaround.
The design uses oneof for field behavior, so LockedField can carry a typed scope rather than being a simple boolean:
oneof behavior {
LockedField locked = 1;
EditableField editable = 2;
}
message LockedField {
string value = 1; // the locked value
LockScope scope = 2; // when the lock applies
}
enum LockScope {
LOCK_SCOPE_UNSPECIFIED = 0; // rejected by validation (OSAC enum convention)
LOCK_SCOPE_CREATE = 1; // locked at creation, mutable on update
LOCK_SCOPE_ALWAYS = 2; // locked at creation and on update
}This lets admins express intent per field - image might be LOCK_SCOPE_ALWAYS (never changeable), while release_image on clusters might be LOCK_SCOPE_CREATE (locked at provisioning, but upgradeable day-2).
I'd rather we make a conscious decision on lock semantics now than discover the gap after someone patches around a lock.
There was a problem hiding this comment.
We're aligned on the need. I thought to reduce the scope of this feature, but post-provisioning governance isn't big enough for its own feature anyway. Added to this PRD: #201
|
Looks like it was auto-merged (by mistake) due to approval. |
5 participants
PRD: Catalog Items v2 — Field Governance Redesign
Jira: https://redhat.atlassian.net/browse/OSAC-3538
Summary
Redesign catalog item field governance to replace the generic field_definitions model with strongly-typed proto fields per catalog item type, each with a behavior enum (locked/editable). This enables richer UX (e.g., instance type as enum, image as dropdown), database-level referential integrity for referenced resources, and an overlay model where ungoverned fields behave normally.
Requesting Review On
How to Review
Summary by CodeRabbit