OSAC-2870: rename 5 more EPs - #149
openshift-merge-bot[bot] merged 3 commits into
Conversation
|
@tchughesiv: This pull request references OSAC-2870 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 sub-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. |
WalkthroughAdds PRDs for storage control-plane and metering initiatives, defines the OSAC-1421 cluster and VM provisioning wizard, updates a related Bare Metal design reference, and replaces outdated enhancement tracking links. ChangesCluster and VM provisioning wizard
Storage control plane PRD
Metering and usage tracking PRD
Enhancement tracking metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 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-149Score: 7/8 | Verdict: PASS
Verdict: Well-executed housekeeping PR that renames 3 enhancement-proposal directories to follow OSAC naming conventions — the design review rubric is a mismatch for this PR type, but the changes are correct, thorough, and clearly scoped. Feedback: This PR is a naming-convention cleanup, not a design document, so the design review rubric doesn't meaningfully apply — no action needed on the low testability score. The PR itself is exemplary housekeeping: clear rationale for inclusions and exclusions, impact analysis on open PRs, and verification that no stale references remain. Consider adding a comment on #131 (as noted) to prompt the author to retarget their new file to the renamed directory path. Critical (0)None. Important (0)None. Suggestions (2)
Review costModel: claude-opus-4-6 |
tchughesiv
changed the title
… main pass These 3 directories were missed by the original OSAC-2870 naming cleanup (osac-project#139/osac-project#144) specifically because they had active, unmerged PRs against them at the time the plan was drafted, so their Jira keys and directory names were still moving targets: - storage-control-plane-osac-2872 -> OSAC-2872-storage-control-plane (Jira key existed, but was still on an open PR (osac-project#134) that hadn't merged to main yet when osac-project#139 was planned/built) - cluster-and-vm-provisioning-wizard -> OSAC-1421-cluster-and-vm-provisioning-wizard (key OSAC-1421 has been in the doc's tracking-link since June; PR osac-project#108 was open against it at audit time) - metering-and-usage-tracking -> OSAC-985-metering-and-usage-tracking (key OSAC-985 has been in the doc's tracking-link for weeks; PRs osac-project#131 and osac-project#143 were open against it at audit time) Updated the one cross-reference found repo-wide pointing at the old cluster-and-vm-provisioning-wizard path (in OSAC-1319-bare-metal-instance-ui/design.md). No cross-references found for the other two. Note: PR osac-project#131 (open, adds a new metering-and-usage-tracking/design.md) will need to retarget to the new path when it rebases, since it adds a file git has no rename history for -- flagging this on that PR separately. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
…itle - carbide-integration -> OSAC-1191-carbide-integration (MGMT-23384 in PR osac-project#27 title -> OSAC-102 Story -> OSAC-31 Epic -> OSAC-1191 Feature; confirmed by author match: doc author Trey West == OSAC-102 assignee) - computeinstance-phase-condition-expansion -> OSAC-1027-computeinstance-phase-condition-expansion (MGMT-22638 in PR osac-project#24 title -> OSAC-395 Task -> OSAC-53 Epic -> OSAC-1027 Feature; confirmed by author match: doc author Akshay Nadkarni == OSAC-395 assignee) Updated tracking-link frontmatter for both and the one cross-reference in OSAC-55-vm-snapshots/README.md. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
61c103b to
0cb5ef6
Compare
tchughesiv
changed the title
tchughesiv
changed the title
AI Design Review: EP-149Score: 6/8 | Verdict: PASS
Verdict: This PR is a well-executed housekeeping change that standardizes enhancement directory naming to the - convention and backfills tracking links — it is not a design document, so design-specific criteria (architecture depth, test strategy) are not meaningfully applicable. Feedback: The renames and reference updates are clean and complete. Verify that no other files in the repo reference the old directory paths (e.g., other design documents' see-also fields, any CI scripts, or documentation links) — a grep for the old slugs (e.g., 'carbide-integration', 'metering-and-usage-tracking', 'storage-control-plane-osac-2872') across the full repo would catch any missed cross-references. Consider adding a brief PR description noting the naming convention being enforced so reviewers understand the motivation at a glance. Critical (0)None. Important (1)
Suggestions (2)
Review costModel: claude-opus-4-6 |
…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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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-1421-cluster-and-vm-provisioning-wizard/design.md`:
- Around line 31-32: Update the Next-button validation wording in the design
document to say “fields that have not yet been blurred” or “fields that have not
been blurred,” replacing the current “fields that have not blurred” phrasing.
- Line 107: Resolve the cluster create adapter’s contract for empty node_sets,
then update the Configuration, create-payload, and Review requirements to
specify exactly one representation: either omit spec.node_sets or send an empty
map. Ensure all affected wording consistently documents the selected wire
format.
- Around line 146-165: Correct the inconsistent OsacForm path casing in the
design document: align the `OsacForm` reference with the surrounding shared-form
directory casing, using the repository’s actual casing consistently across the
documented component paths.
In `@enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md`:
- Around line 195-198: Update prd.md lines 195-198 to exempt sensitive fields
from literal Review value parity, specifically pull_secret and potentially
sensitive user_data, requiring redacted values or presence indicators without
logging or persisting contents. Update design.md lines 69-75 to define masked
Review rendering, and lines 181-184 to apply the masking requirement beyond
General to all relevant sensitive fields.
- Around line 224-253: Synchronize the v1 decisions across
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md lines 224-253
and update its §2.1.1 and acceptance criteria to explicitly define ssh key
requiredness, empty Cluster template node_sets behavior, and whether additional
disks are supported. In
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md line 175,
retain only decisions that are also documented in the PRD; remove or revise any
unresolved or inconsistent decision.
In `@enhancements/OSAC-2872-storage-control-plane/prd.md`:
- Line 21: Update the volume lifecycle definition in the PRD to include explicit
failure or unknown states for interrupted create and delete operations. Specify
the resulting user-visible behavior, including how inventory reflects uncertain
status and how cleanup or retry prevents volumes from remaining stuck or leaked,
while preserving the existing normal lifecycle states.
- Around line 17-19: Update the Storage control plane services and vendor plugin
deployment model to require a trusted control-plane proxy or brokered identity
for backend access, ensuring tenant-cluster compromise cannot expose vendor
credentials or directly access storage backends. If plugins must retain direct
backend access, revise the credential-isolation guarantee throughout the PRD to
explicitly define the weaker security boundary before implementation.
- Around line 47-61: Clarify the storage capability statement and the “view my
persistent volumes and claims” requirement so authorization scopes are explicit:
Tenant Admins may list/get all volumes owned by their organization across
clusters, while Tenant Users may list/get only volumes they created or own. Keep
creation, deletion, and unconfigured StorageClass behavior aligned with the
existing role permissions.
In `@enhancements/OSAC-985-metering-and-usage-tracking/prd.md`:
- Line 203: Update the instance-type-seconds definition in the metering table so
its usage formula returns 3,600 instance-type-seconds for one hour, without
currency. Represent the charge calculation separately as usage multiplied by the
applicable price-per-second rate.
- Around line 191-193: Update the VMaaS metering description around
instance-type-seconds to remove the claim that instance types bundle a fixed
boot disk. Keep instance-type-seconds limited to instance uptime and state that
boot-disk/storage usage will be handled separately when storage metering enters
scope, consistent with the existing instance-type contract.
- Line 87: Update the MaaS metering requirements around CAP-13 and the related
sections at the referenced acceptance criteria and formulas to define an
inference request-count meter, including its unit, rate, and acceptance
criterion; ensure the formulas and downstream usage definitions consistently
include this meter, or remove the per-inference-request billing language if
request-based charging is not supported.
- Around line 218-224: Clarify the token aggregation rules in the metering
description and table: cached tokens must be accounted for separately and
excluded from billable input-token totals, so the same cached input is not
counted twice. State how total input usage is derived when cached and non-cached
prompt tokens are both present.
- Around line 46-47: Resolve the scope inconsistency between the deferred
storage/networking metering statements and the stopped/paused VM acceptance
criteria around the VM metering requirements. Either remove or defer the
criteria requiring storage and public-IP/networking resources to be metered for
stopped or paused VMs, or explicitly add the necessary resource meters to this
PRD’s scope and update the relevant deferred-scope entries consistently.
🪄 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: 8bf0370d-69c3-43e9-a793-93edea4f0762
📒 Files selected for processing (8)
enhancements/OSAC-1027-computeinstance-phase-condition-expansion/README.mdenhancements/OSAC-1191-carbide-integration/README.mdenhancements/OSAC-1319-bare-metal-instance-ui/design.mdenhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.mdenhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.mdenhancements/OSAC-2872-storage-control-plane/prd.mdenhancements/OSAC-55-vm-snapshots/README.mdenhancements/OSAC-985-metering-and-usage-tracking/prd.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 13
🤖 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-1421-cluster-and-vm-provisioning-wizard/design.md`:
- Around line 31-32: Update the Next-button validation wording in the design
document to say “fields that have not yet been blurred” or “fields that have not
been blurred,” replacing the current “fields that have not blurred” phrasing.
- Line 107: Resolve the cluster create adapter’s contract for empty node_sets,
then update the Configuration, create-payload, and Review requirements to
specify exactly one representation: either omit spec.node_sets or send an empty
map. Ensure all affected wording consistently documents the selected wire
format.
- Around line 146-165: Correct the inconsistent OsacForm path casing in the
design document: align the `OsacForm` reference with the surrounding shared-form
directory casing, using the repository’s actual casing consistently across the
documented component paths.
In `@enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md`:
- Around line 195-198: Update prd.md lines 195-198 to exempt sensitive fields
from literal Review value parity, specifically pull_secret and potentially
sensitive user_data, requiring redacted values or presence indicators without
logging or persisting contents. Update design.md lines 69-75 to define masked
Review rendering, and lines 181-184 to apply the masking requirement beyond
General to all relevant sensitive fields.
- Around line 224-253: Synchronize the v1 decisions across
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md lines 224-253
and update its §2.1.1 and acceptance criteria to explicitly define ssh key
requiredness, empty Cluster template node_sets behavior, and whether additional
disks are supported. In
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md line 175,
retain only decisions that are also documented in the PRD; remove or revise any
unresolved or inconsistent decision.
In `@enhancements/OSAC-2872-storage-control-plane/prd.md`:
- Line 21: Update the volume lifecycle definition in the PRD to include explicit
failure or unknown states for interrupted create and delete operations. Specify
the resulting user-visible behavior, including how inventory reflects uncertain
status and how cleanup or retry prevents volumes from remaining stuck or leaked,
while preserving the existing normal lifecycle states.
- Around line 17-19: Update the Storage control plane services and vendor plugin
deployment model to require a trusted control-plane proxy or brokered identity
for backend access, ensuring tenant-cluster compromise cannot expose vendor
credentials or directly access storage backends. If plugins must retain direct
backend access, revise the credential-isolation guarantee throughout the PRD to
explicitly define the weaker security boundary before implementation.
- Around line 47-61: Clarify the storage capability statement and the “view my
persistent volumes and claims” requirement so authorization scopes are explicit:
Tenant Admins may list/get all volumes owned by their organization across
clusters, while Tenant Users may list/get only volumes they created or own. Keep
creation, deletion, and unconfigured StorageClass behavior aligned with the
existing role permissions.
In `@enhancements/OSAC-985-metering-and-usage-tracking/prd.md`:
- Line 203: Update the instance-type-seconds definition in the metering table so
its usage formula returns 3,600 instance-type-seconds for one hour, without
currency. Represent the charge calculation separately as usage multiplied by the
applicable price-per-second rate.
- Around line 191-193: Update the VMaaS metering description around
instance-type-seconds to remove the claim that instance types bundle a fixed
boot disk. Keep instance-type-seconds limited to instance uptime and state that
boot-disk/storage usage will be handled separately when storage metering enters
scope, consistent with the existing instance-type contract.
- Line 87: Update the MaaS metering requirements around CAP-13 and the related
sections at the referenced acceptance criteria and formulas to define an
inference request-count meter, including its unit, rate, and acceptance
criterion; ensure the formulas and downstream usage definitions consistently
include this meter, or remove the per-inference-request billing language if
request-based charging is not supported.
- Around line 218-224: Clarify the token aggregation rules in the metering
description and table: cached tokens must be accounted for separately and
excluded from billable input-token totals, so the same cached input is not
counted twice. State how total input usage is derived when cached and non-cached
prompt tokens are both present.
- Around line 46-47: Resolve the scope inconsistency between the deferred
storage/networking metering statements and the stopped/paused VM acceptance
criteria around the VM metering requirements. Either remove or defer the
criteria requiring storage and public-IP/networking resources to be metered for
stopped or paused VMs, or explicitly add the necessary resource meters to this
PRD’s scope and update the relevant deferred-scope entries consistently.
🪄 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: 8bf0370d-69c3-43e9-a793-93edea4f0762
📒 Files selected for processing (8)
enhancements/OSAC-1027-computeinstance-phase-condition-expansion/README.mdenhancements/OSAC-1191-carbide-integration/README.mdenhancements/OSAC-1319-bare-metal-instance-ui/design.mdenhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.mdenhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.mdenhancements/OSAC-2872-storage-control-plane/prd.mdenhancements/OSAC-55-vm-snapshots/README.mdenhancements/OSAC-985-metering-and-usage-tracking/prd.md
🛑 Comments failed to post (13)
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md (3)
31-32: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the wording around untouched fields.
Use “fields that have not yet been blurred” or “fields that have not been blurred.”
🧰 Tools
🪛 LanguageTool
[grammar] ~32-~32: Ensure spelling is correct
Context: ... that have not blurred. - On successful create, navigate to the VM or cluster Details ...(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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-1421-cluster-and-vm-provisioning-wizard/design.md` around lines 31 - 32, Update the Next-button validation wording in the design document to say “fields that have not yet been blurred” or “fields that have not been blurred,” replacing the current “fields that have not blurred” phrasing.Source: Linters/SAST tools
107-107: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate and inspect the referenced design excerpt plus nearby contract context. if [ -f "enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md" ]; then echo "== referenced excerpt ==" sed -n '90,120p' enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md | cat -n -v echo echo "== adjacent API/schema mentions ==" rg -n "node_sets|spec\.node_sets|nodeSets|Create payload|Create API|payload|schema|template" enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md else echo "referenced file not found" echo "matching files:" fd -a 'design\.md$' enhancements 2>/dev/null | sed -n '1,50p' fi echo echo "== workspace search for create payload semantics ==" rg -n "create payload|Create payload|omit.*node_sets|node_sets|spec\.node_sets|nodeSets" . -g '!node_modules' -g '!dist' -g '!build' | sed -n '1,160p'Repository: osac-project/enhancement-proposals
Length of output: 23618
Choose one empty-
node_setscreate payload representation.The current wording allows either omitting
spec.node_setsor sending an empty map, which maps to two incompatible wire formats for the cluster create adapter. Resolve the create API contract first, then document one consistent payload for the PRD/design.🤖 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-1421-cluster-and-vm-provisioning-wizard/design.md` at line 107, Resolve the cluster create adapter’s contract for empty node_sets, then update the Configuration, create-payload, and Review requirements to specify exactly one representation: either omit spec.node_sets or send an empty map. Ensure all affected wording consistently documents the selected wire format.
146-165: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Tracked OsacForm/form directories/files:" git ls-files | sed -n '/libs\/ui-components\/src\/components\/form/i; /libs\/ui-components\/src\/components\/Form/i; /libs\/ui-components\/src\/components\/form/i; /libs\/ui-components\/src\/components\/Form/i;' | grep -Ei 'libs/ui-components/src/components/(form|Form)/|OsacForm' || true echo echo "Find OsacForm exact/insensitive:" fd -i 'OsacForm' . echo echo "Find references to OsacForm/Form/form casing in design/code:" rg -n --hidden 'OsacForm|components/form|components/Form|from ["'\''][^"\']*form[^"\']*["'\'']|from ["'\''][^"\']*Form[^"\']*["'\'']' -g '!node_modules' -g '!dist' -g '!build' . || trueRepository: osac-project/enhancement-proposals
Length of output: 575
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Top-level files matching Form/form component names:" git ls-files | grep -Ei 'osacform|form' || true echo echo "Directory containing component code, if present:" fd -i ^form$ . | sed 's#^\./##' || true echo echo "Design references around wrapper layout:" rg -n 'libs/ui-components/src/components/(form|Form)/(Form|OsacForm|.*Field|Wizard|Fields|Picker|PickerSelect)|`@osac/ui-components`|OsacForm|FormGroup' enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md || trueRepository: osac-project/enhancement-proposals
Length of output: 4757
Correct the
OsacFormpath casing.The document lists shared components under
libs/ui-components/src/components/form/, but theOsacFormlocation usescomponents/Form/OsacForm.tsx. Either update the wrapper path tocomponents/form/OsacForm.tsxif that is the real casing, or adjust the shared-form directory casing in the surrounding text so this design does not point at two different paths.🤖 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-1421-cluster-and-vm-provisioning-wizard/design.md` around lines 146 - 165, Correct the inconsistent OsacForm path casing in the design document: align the `OsacForm` reference with the surrounding shared-form directory casing, using the repository’s actual casing consistently across the documented component paths.enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md (2)
195-198: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Prevent sensitive data disclosure on Review.
The “same values” requirement conflicts with masking Secret-backed fields. Review must display redacted values or presence indicators for
pull_secretand potentially sensitiveuser_data, without logging or persisting their contents.
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md#L195-L198: exempt sensitive fields from literal value parity.enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md#L69-L75: define masked Review rendering.enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md#L181-L184: extend the masking requirement beyond General.📍 Affects 2 files
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md#L195-L198(this comment)enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md#L69-L75enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md#L181-L184🤖 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-1421-cluster-and-vm-provisioning-wizard/prd.md` around lines 195 - 198, Update prd.md lines 195-198 to exempt sensitive fields from literal Review value parity, specifically pull_secret and potentially sensitive user_data, requiring redacted values or presence indicators without logging or persisting contents. Update design.md lines 69-75 to define masked Review rendering, and lines 181-184 to apply the masking requirement beyond General to all relevant sensitive fields.
224-253: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep the PRD and design decisions synchronized.
The PRD leaves requiredness, empty
node_sets, and additional disks open, while the design resolves them for v1. These decisions affect validation and payload construction.
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md#L224-L253: record the selected v1 decisions and update acceptance criteria.enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md#L175-L175: retain only decisions that are reflected in the PRD.📍 Affects 2 files
enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md#L224-L253(this comment)enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md#L175-L175🤖 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-1421-cluster-and-vm-provisioning-wizard/prd.md` around lines 224 - 253, Synchronize the v1 decisions across enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/prd.md lines 224-253 and update its §2.1.1 and acceptance criteria to explicitly define ssh key requiredness, empty Cluster template node_sets behavior, and whether additional disks are supported. In enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard/design.md line 175, retain only decisions that are also documented in the PRD; remove or revise any unresolved or inconsistent decision.enhancements/OSAC-2872-storage-control-plane/prd.md (3)
17-19: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Align the credential-isolation guarantee with the deployment model.
The PRD deploys vendor plugins on tenant clusters, but also promises that a compromised tenant cluster cannot access storage backends. If backend credentials or direct backend access are available to the plugin in that cluster, compromise can expose or use them. Require a trusted control-plane proxy/brokered identity, or explicitly weaken and redefine this guarantee before implementation.
Also applies to: 27-27, 69-69
🤖 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-2872-storage-control-plane/prd.md` around lines 17 - 19, Update the Storage control plane services and vendor plugin deployment model to require a trusted control-plane proxy or brokered identity for backend access, ensuring tenant-cluster compromise cannot expose vendor credentials or directly access storage backends. If plugins must retain direct backend access, revise the credential-isolation guarantee throughout the PRD to explicitly define the weaker security boundary before implementation.
21-21: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Define failure states for volume inventory and deletion.
The lifecycle omits failed or interrupted create/delete operations, yet the user story promises that deleting a PVC cleans up the underlying volume. Add explicit failure/unknown semantics and the resulting user-visible behavior; otherwise failed operations can remain stuck in
creatingordeleting, causing inaccurate inventory and leaked storage.Also applies to: 51-51
🤖 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-2872-storage-control-plane/prd.md` at line 21, Update the volume lifecycle definition in the PRD to include explicit failure or unknown states for interrupted create and delete operations. Specify the resulting user-visible behavior, including how inventory reflects uncertain status and how cleanup or retry prevents volumes from remaining stuck or leaked, while preserving the existing normal lifecycle states.
47-61: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Resolve the Tenant Admin/User authorization ambiguity.
Line 47 says both roles have the same capabilities, while Line 61 gives Tenant Admin organization-wide volume visibility. Define the list/get scope explicitly—such as organization-wide for Tenant Admin and self-owned volumes for Tenant User—to prevent inconsistent or over-broad authorization implementations.
🧰 Tools
🪛 LanguageTool
[style] ~51-~51: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...addresses. - As a Tenant Admin/User, I want to delete a PVC, so that the underlying vo...(REP_WANT_TO_VB)
[style] ~53-~53: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...aned up and the storage is released. - As a Tenant Admin/User, I want to view my ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~53-~53: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... released. - As a Tenant Admin/User, I want to view my persistent volumes and claims u...(REP_WANT_TO_VB)
[style] ~55-~55: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...monitor storage usage on my cluster. - As a Tenant Admin/User, I want a PVC refer...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~57-~57: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...d consistent with native Kubernetes. - As a Tenant Admin/User, I want storage to ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~59-~59: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...VCs without requesting manual setup. - As a Tenant User, I want every volume I cr...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~61-~61: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...storage usage is attributable to me. - As a Tenant Admin, I want to see all volum...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 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-2872-storage-control-plane/prd.md` around lines 47 - 61, Clarify the storage capability statement and the “view my persistent volumes and claims” requirement so authorization scopes are explicit: Tenant Admins may list/get all volumes owned by their organization across clusters, while Tenant Users may list/get only volumes they created or own. Keep creation, deletion, and unconfigured StorageClass behavior aligned with the existing role permissions.enhancements/OSAC-985-metering-and-usage-tracking/prd.md (5)
46-47: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the deferred-scope conflict for VM-attached resources.
Line [46] defers storage metering and Line [47] defers public-IP/networking metering, but Lines [85] and [105] require those resources to be metered while a VM is stopped or paused. Either defer these acceptance criteria or explicitly bring the required resource meters into scope.
Also applies to: 85-85, 103-105
🧰 Tools
🪛 LanguageTool
[grammar] ~47-~47: Ensure spelling is correct
Context: ...ce metering — VirtualNetworks, Subnets, PublicIPs, NAT Gateways (deferred to a future PRD...(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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-985-metering-and-usage-tracking/prd.md` around lines 46 - 47, Resolve the scope inconsistency between the deferred storage/networking metering statements and the stopped/paused VM acceptance criteria around the VM metering requirements. Either remove or defer the criteria requiring storage and public-IP/networking resources to be metered for stopped or paused VMs, or explicitly add the necessary resource meters to this PRD’s scope and update the relevant deferred-scope entries consistently.
87-87: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Define the MaaS per-request meter.
Line [87] promises charges per inference request, but the acceptance criteria and formulas define only token meters. Add a request-count meter, unit, rate, and acceptance criterion, or remove the per-request billing requirement.
Also applies to: 118-121, 220-224
🤖 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-985-metering-and-usage-tracking/prd.md` at line 87, Update the MaaS metering requirements around CAP-13 and the related sections at the referenced acceptance criteria and formulas to define an inference request-count meter, including its unit, rate, and acceptance criterion; ensure the formulas and downstream usage definitions consistently include this meter, or remove the per-inference-request billing language if request-based charging is not supported.
191-193: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align boot-disk treatment with the existing instance-type contract.
Line [193] says instance types bundle a fixed boot disk, but
enhancements/OSAC-46-vm-instance-types/README.md:44-70keeps storage specifications and boot disks separate. Do not include boot storage ininstance-type-seconds; specify separate storage handling when that meter enters scope.🤖 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-985-metering-and-usage-tracking/prd.md` around lines 191 - 193, Update the VMaaS metering description around instance-type-seconds to remove the claim that instance types bundle a fixed boot disk. Keep instance-type-seconds limited to instance uptime and state that boot-disk/storage usage will be handled separately when storage metering enters scope, consistent with the existing instance-type contract.
203-203: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Separate meter units from currency calculations.
The
instance-type-secondsformula returns$3.60, which is a charge rather than a meter quantity. Define usage as 3,600 instance-type-seconds and calculate charges separately as usage multiplied by the rate.🤖 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-985-metering-and-usage-tracking/prd.md` at line 203, Update the instance-type-seconds definition in the metering table so its usage formula returns 3,600 instance-type-seconds for one hour, without currency. Represent the charge calculation separately as usage multiplied by the applicable price-per-second rate.
218-224: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make cached-token accounting mutually exclusive.
The document defines input tokens as the full prompt and cached tokens as a separate meter, without stating whether cached tokens are included in input or total tokens. Clarify the aggregation rule to prevent cached input from being double-counted.
🤖 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-985-metering-and-usage-tracking/prd.md` around lines 218 - 224, Clarify the token aggregation rules in the metering description and table: cached tokens must be accounted for separately and excluded from billable input-token totals, so the same cached input is not counted twice. State how total input usage is derived when cached and non-cached prompt tokens are both present.
|
@coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
ᕱ⑅ᕱ ✏️ Learnings added
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eranco74, tchughesiv 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 |
The per-judge claude-sonnet-4-6 override belongs in OSAC-2264 (the PR that introduces the qualitative_finding_quality judge entry and is where the override was originally dropped during a rebase), not OSAC-2266 as previously stated. Already restored directly in OSAC-2264 PR osac-project#149 (commit 7e5346c8). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
Full-repo sweep across all 41 retired directory names from the entire OSAC-2870 effort (not just this PR's 5) turned up two more stale references that earlier passes missed: - OSAC-1330-type-safe-resource-references/design.md still linked to '/enhancements/networking' (renamed to OSAC-356-networking in osac-project#144). This directory was self-renamed by osac-project#121's own branch, so it never went through our cross-reference sweep. - OSAC-985-metering-and-usage-tracking/design.md still linked to '/enhancements/vm-instance-types' (renamed to OSAC-46-vm-instance-types in osac-project#144). metering-and-usage-tracking was one of the directories deferred at that time due to an open PR, so it was excluded from that pass's cross-reference sweep and the reference went stale once the deferred rename landed in osac-project#149. No open PRs conflict with either file (re-verified). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
The per-judge claude-sonnet-4-6 override belongs in OSAC-2264 (the PR that introduces the qualitative_finding_quality judge entry and is where the override was originally dropped during a rebase), not OSAC-2266 as previously stated. Already restored directly in OSAC-2264 PR osac-project#149 (commit 7e5346c8). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
Full-repo sweep across all 41 retired directory names from the entire OSAC-2870 effort (not just this PR's 5) turned up two more stale references that earlier passes missed: - OSAC-1330-type-safe-resource-references/design.md still linked to '/enhancements/networking' (renamed to OSAC-356-networking in osac-project#144). This directory was self-renamed by osac-project#121's own branch, so it never went through our cross-reference sweep. - OSAC-985-metering-and-usage-tracking/design.md still linked to '/enhancements/vm-instance-types' (renamed to OSAC-46-vm-instance-types in osac-project#144). metering-and-usage-tracking was one of the directories deferred at that time due to an open PR, so it was excluded from that pass's cross-reference sweep and the reference went stale once the deferred rename landed in osac-project#149. No open PRs conflict with either file (re-verified). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
3 participants
Summary
Third pass of the OSAC-2870 enhancement-proposals naming cleanup (following #139 and #144). Renames 5 more directories:
MGMT-23384: Add Carbide integration enhancement proposal (#27)) — confirmed by cross-checking that the doc's listed author matches the resolved Jira issue's assignee.Revisiting now: the first 3's blocking PRs (#108, #131, #134, #143) are either merged or still open with unrelated
CHANGES_REQUESTEDfeedback outstanding, so a rename today doesn't disrupt an otherwise-ready merge for any of them — see the per-PR rebase impact below. The 2 found via hidden PR-title keys have no open PRs against them at all.Changes
storage-control-plane-osac-2872OSAC-2872-storage-control-planemainyet — it landed via #134, which was still open when #139 was planned/built, and merged only ~1 hour before #139 did.cluster-and-vm-provisioning-wizardOSAC-1421-cluster-and-vm-provisioning-wizardOSAC-1421) has been in the doc'stracking-linksince June; #108 was open against this directory at audit time.metering-and-usage-trackingOSAC-985-metering-and-usage-trackingOSAC-985) has been in the doc'stracking-linkfor weeks; #131 and #143 were both open against this directory at audit time.carbide-integrationOSAC-1191-carbide-integrationtracking-link: None). Key found inMGMT-23384in PR #27's title → resolves toOSAC-102(Story) →OSAC-31(Epic) →OSAC-1191(Feature).computeinstance-phase-condition-expansionOSAC-1027-computeinstance-phase-condition-expansiontracking-link: TBD. Key found inMGMT-22638in PR #24's title → resolves toOSAC-395(Task) →OSAC-53(Epic) →OSAC-1027(Feature).All 5 renames used
git mvto preserve file history (visible as 100%-similarity renames in the diff).Cross-references updated
Searched the full repo (all file types, not just
.md) for references to all 5 old directory names, plus re-swept every retired directory name from the entire OSAC-2870 effort (25 renames across all prior PRs) to catch anything introduced by unrelated merges since our last audit. Found and updated three:OSAC-1319-bare-metal-instance-ui/design.mdlinked to/enhancements/cluster-and-vm-provisioning-wizard, now points to/enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard.OSAC-55-vm-snapshots/README.mdlinked to/enhancements/computeinstance-phase-condition-expansion, now points to/enhancements/OSAC-1027-computeinstance-phase-condition-expansion.catalog-items/ui-design.md(merged via #128 after our earlier renaming passes) linked to/enhancements/cluster-and-vm-provisioning-wizard, now points to/enhancements/OSAC-1421-cluster-and-vm-provisioning-wizard. This one wasn't stale when OSAC-2870: Standardize enhancement-proposals directory/file naming convention #139/OSAC-2870: rename 8 more README-only EPs with resolvable Jira keys #144 were built — it didn't exist yet — so it's a good example of why a periodic re-sweep is worth doing.The remaining hits from the full re-sweep were all in
.github/scripts/test_check_ep_naming.py— unit test fixtures using old directory names as arbitrary/illustrative path strings (e.g. reproducing the #121 false-positive scenario), not live references. Left those alone.Notes for reviewers
OSAC-2872-storage-control-plane— @akshaynadkarni, @rgolanghOSAC-1421-cluster-and-vm-provisioning-wizard— @batzionbOSAC-985-metering-and-usage-tracking— @masayagOSAC-1191-carbide-integration— @trewestOSAC-1027-computeinstance-phase-condition-expansion— @akshaynadkarniCHANGES_REQUESTEDfromAlonaKaplan; Design: OSAC Metering and Usage Tracking #131/OSAC-985: narrow CAP-17 wording to traceability #143 likewise), so this isn't racing anything toward merge.metering-and-usage-tracking/design.md) is the one exception — since it's adding a file git has no rename history for, it won't automatically follow the directory rename. Flagged this directly on the PR so its author can retarget toenhancements/OSAC-985-metering-and-usage-tracking/design.mdon their next rebase (which they'll need to do anyway to address open review feedback).type-safe-resource-references(OSAC-1330) — #121 has already renamed it toOSAC-1330-type-safe-resource-referencesin its own branch (alongside addingdesign.md), so there's nothing left for this PR to do there; duplicating that rename here would only create unnecessary rebase friction for OSAC-2766: Design - Type-Safe Resource References #121 once it lands.simplified-resource-creationor theunified-networking/unified-networking-prdpair — both flagged as genuinely ambiguous (a stale/mismatched Jira key in one case, a legacy split-PRD-and-design-across-two-directories structure in the other) rather than a simple rename; left for a separate decision.carbide-integration/computeinstance-phase-condition-expansiondiscovery method: checked every remaining non-conforming directory's origin commit and originating PR's title/body/comments for a Jira key not present in the doc itself. Two hits, bothMGMT-keys translated toOSAC-and traced up to a Feature via the parent chain. Cross-checked each against the doc's ownauthors:field matching the resolved issue's assignee, to guard against picking up an unrelated key that happened to appear in the same PR/commit title. All other remaining non-conforming directories (bare-metal-fulfillment,catalog-items,dns-api,organizations,repository-consolidation,tenant-specific-storageclasses/vm-api-fields,vmaas) came up empty on this same title/body/comment search — genuinely keyless.Testing
pre-commit run --all-fileslocally — all hooks pass, includingcheck-ep-naming.grepthat no stray references to any of the 5 old directory names remain anywhere in the repo.