OSAC-2644: PRD — BCM Backend Integration for BMaaS - #126
Conversation
|
@mennyaboush: This pull request references OSAC-2644 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. |
|
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:
WalkthroughThe PRD defines OSAC integration with NVIDIA BCM as a pluggable BMaaS inventory backend, covering configuration, secure connectivity, host discovery and assignment, lifecycle handling, retries, testing, acceptance criteria, and operational constraints. ChangesBCM Backend Integration
Estimated code review effort: 1 (Trivial) | ~3 minutes 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 EP Review: EP-126Score: 6/10 | Verdict: PASS
Verdict: The PRD has a clear problem statement and well-scoped focus, but scores zero on user-facing focus due to pervasive design leakage — requirements describe controllers, sentinel errors, internal function calls, CR annotations, and finalizer behavior instead of user-observable outcomes. Feedback: Rewrite FR-1 through FR-10 as user-observable outcomes rather than implementation steps. For example, instead of 'FindFreeHost() queries BCM for an unassigned LiteNode', write 'The system selects an available host matching the requested type from BCM inventory.' Move all internal details (ErrHostNotReady, BMH CRs, exponential backoff parameters, finalizers, function signatures) to the design document. Strengthen the WHY by explaining what business impact the BCM gap has — e.g., how many nodes are affected, what deployments are blocked, or what strategic goal (NVIDIA partnership, sovereign AI) this enables. Critical (2)
Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI EP Review: EP-126Score: 8/10 | Verdict: PASS
Verdict: A well-structured PRD with strong justification and tight scope, held back from a higher score by missing formal persona-grouped user stories and moderate design leakage in NFRs and dependencies. Feedback: Add a User Stories section with 'As a ...' stories grouped under persona headings (Cloud Infrastructure Admin, Cloud Provider Admin) — the FRs already contain the content, it just needs restructuring. Remove design leakage: drop internal BCM feature names from NFR-1 (just state the minimum version), replace 'networkClass: cudn_net' in NFR-3 with a user-facing description, and move FR-10 (BCM simulator) to a test strategy note rather than a functional requirement. The Dependencies section should reference user-facing capabilities rather than internal components (Metal3/Ironic, AAP jobs). 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}
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: Strong PRD with clear user need, concrete justification, focused scope, and verifiable acceptance criteria; moderate design leakage in functional requirements and dependencies is the main area for improvement. Feedback: The PRD is well-structured and covers personas, dimensions, and scope effectively. To strengthen it, rewrite FR-8 to describe the user-observable outcome of deprovisioning ('the host is released and available for new assignments') without naming internal steps like 'OS wipe, network detach, power off.' Similarly, trim NFR-1 to state only the minimum version requirement without explaining internal BCM API features, and move Metal3/Ironic/AAP from the Dependencies section into the design document where implementation dependencies belong. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: The PR contains five well-written, individually strong enhancement proposals covering BCM backend, DiskImage, bare metal UI, secret management, and catalog item improvements, but bundles them into a single oversized PR that should be split for independent review and merge. Feedback: Split this PR into separate PRs per enhancement — each feature deserves independent review and merge decisions. The disk-image PRD (OSAC-2540) is missing a formal 'Acceptance Criteria' section with checkable items, unlike the bcm-backend PRD which has 10 concrete criteria; add one mirroring the bcm-backend pattern. The bcm-backend PRD has minor design leakage in FR-5 ('records an assignment identifier in BCM'), FR-6 ('the system retries automatically'), and FR-10 ('BCM simulator') — reframe these as user-observable outcomes rather than system behaviors. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
1764241 to
dfb185a
Compare
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: Strong PRD with clear user need, concrete business justification, and well-scoped capabilities, held back slightly by design leakage in a few functional requirements that describe internal system behavior rather than user-observable outcomes. Feedback: Rewrite FR-5, FR-8, and FR-10 to describe user-observable outcomes rather than internal system behavior. FR-5 should focus on the tenant isolation guarantee (no tenant data in BCM), not the mechanism; FR-8 should state 'the host is fully cleaned and returned to the available pool' without listing internal steps (OS wipe, network detach); FR-10 should either be moved to a testing section or reframed as a deployment confidence requirement. Consider adding milestone scoping per osac-dimensions.md guidance. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
AI Design Review: EP-126Score: 3/8 | Verdict: FAIL
Verdict: The PR submits a PRD (prd.md) where a design document is expected — it lacks all architectural detail (proto schemas, controller patterns, tenant annotations, cross-repo impacts) required by the design review rubric, resulting in an automatic-fail zero on Architecture. Feedback: This document is a well-structured PRD with strong scope boundaries and specific out-of-scope items, but it cannot pass a design review because it is not a design document. To proceed, write a companion design.md that covers: (1) proto schemas for BCM backend configuration and any new/modified resources, (2) controller reconciliation flow with finalizer and status condition details, (3) BCM client implementation with mTLS setup and retry logic, (4) cross-repo change list (osac-operator controller, fulfillment-service backend interface, osac-installer config). Also add an Alternatives section to the PRD comparing BCM integration approaches (e.g., direct BCM API vs. adapter pattern vs. Metal3-only with BCM as inventory source). Critical (4)
Important (5)
Suggestions (3)
Review costModel: claude-opus-4-6 |
| ### 3.2 Non-Goals | ||
|
|
||
| - **Sysinfo-based hardware auto-classification.** Host type matching uses admin-assigned labels. Auto-detection from BCM sysinfo is out of scope. | ||
| - **CaaS/BMaaS race condition resolution.** Both CaaS and BMaaS consume BCM nodes; coordination mechanisms are deferred to the design phase. |
There was a problem hiding this comment.
this is an implementation detail, it can be removed from the prd
dfb185a to
f560da7
Compare
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: Strong PRD with clear personas, concrete business justification, and well-focused scope; held back from a perfect score by design leakage (Metal3/BMH, pluggable backend interface, internal assignment tracking) that should be removed or rewritten as user-observable outcomes. Feedback: Remove references to internal components: replace 'Metal3/BMH' with 'existing host power management,' rewrite 'pluggable backend interface (OSAC-1032)' as 'OSAC inventory backend system,' and remove 'combined BCM + Metal3 state' in favor of describing the user-visible lifecycle states directly. FR-5's internal assignment tracking and FR-10's simulator implementation details should describe what users observe, not how the system works internally. These are straightforward rewrites that would bring the PRD to a clean user-facing focus. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
| - Cloud Infrastructure Admins pre-register LiteNodes in BCM before OSAC operates against them (Day-0 prerequisite). | ||
| - Each deployment uses a single inventory backend; BCM and OpenStack do not run simultaneously. | ||
| - The pluggable backend interface (OSAC-1032) can accommodate a new inventory backend with host preparation delays. | ||
|
|
There was a problem hiding this comment.
I would add a bullet about being able to write back the host assignment into BCM extra_values
There was a problem hiding this comment.
This still needs to be handled. I unresolved the comment. I would just say that we need to write "metadata for host assignment" the exact key/value can wait for design.
f560da7 to
6a45b7d
Compare
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: Solid PRD with clear personas, concrete justification, and well-scoped capability, held back slightly by design leakage in functional requirements and acceptance criteria that reference BCM internals. Feedback: Remove the acceptance criterion referencing BCM's 'extra_values' field — it's an internal implementation detail that belongs in the design document. Rewrite FR-5 and FR-6 to describe user-observable outcomes (e.g., 'the host is reserved for the instance' instead of 'records an assignment identifier in BCM'). For NFR-2, focus on the tenant-facing guarantee ('BCM is invisible to tenants') and move data-handling specifics to the design. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
tchughesiv
left a comment
tchughesiv
left a comment
There was a problem hiding this comment.
Thanks for this proposal! Per the naming convention in CONTRIBUTING.md, new enhancement directories must follow OSAC-NNNN-feature-slug/ (with lowercase prd.md/design.md), where OSAC-NNNN is the Jira Feature-level key.
This PR adds enhancements/bcm-backend/, which doesn't match that pattern yet, and will fail the check-ep-naming CI check once this branch is rebased onto latest main (the check merged in #133, after this PR was opened).
Could you rename enhancements/bcm-backend/ → enhancements/OSAC-1339-bcm-backend/ (and update the tracking-link in the frontmatter if needed)? Happy to help if you have questions about the convention.
6a45b7d to
6547640
Compare
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: A well-structured PRD with clear user-facing capabilities, strong business justification, and focused scope, held back slightly by design leakage in acceptance criteria and functional requirements that prescribe internal behavior (BCM extra_values field, retry/backoff logic, Metal3/BMH references). Feedback: Remove implementation details from acceptance criteria and requirements: replace 'writes the assignment identifier to the host's extra_values field in BCM' with a user-observable equivalent like 'BCM reflects the host assignment so Cloud Infrastructure Admins can verify it via BCM tools.' Rewrite FR-6 to describe the user-observable timeout behavior without prescribing retry strategy. Replace the 'All Users' persona heading with the canonical OSAC personas (Tenant User, Tenant Admin) to match osac-dimensions.md conventions. 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/OSAC-1339-bcm-backend/prd.md`:
- Around line 6-7: Update the Jira metadata for OSAC-1339 to use target version
5.0.0 before merging. If PRDs are expected to mirror Jira release metadata, add
the corresponding target-version field near the existing Jira and Date entries
in the PRD.
- Around line 62-66: Update the FR-4 and FR-5 requirements to state that each
host may have at most one active assignment. Define assignment as an atomic or
otherwise conflict-safe claim, and require concurrent conflicting claims to be
rejected or retried rather than allowing the same host to be assigned twice.
- Around line 74-75: Update the FR-8 deprovisioning lifecycle requirement to
explicitly clear the host’s previous BCM assignment identifier from extra_values
before releasing it to the available pool or allowing reuse. Apply the same
clarification to the corresponding requirement at the additionally referenced
section, preserving the existing cleanup 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: Pro Plus
Run ID: 4afd0109-f398-41b1-aa24-77e007e8d604
📒 Files selected for processing (1)
enhancements/OSAC-1339-bcm-backend/prd.md
c5f2d23 to
989c760
Compare
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: A well-structured PRD with clear personas, strong justification, and focused scope, held back slightly by design leakage where internal BCM details and implementation choices bleed into requirements. Feedback: Move implementation-level details to the design document: remove references to BCM's 'extra_values' field from FR-8 and acceptance criteria, replace with user-observable language (e.g., 'the host is released back to BCM's available pool'). Drop the 'type: bcm' configuration key from acceptance criteria — describe the outcome ('admin can select BCM as the inventory backend') rather than the specific config syntax. FR-6's 'retries automatically with backoff' is implementation; rewrite as 'if preparation does not complete within a configurable timeout, the status shows a clear error' without prescribing retry strategy. Critical (0)None. Important (3)
Suggestions (2)
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/OSAC-1339-bcm-backend/prd.md`:
- Around line 68-69: Update FR-6 to define a deterministic cleanup policy for
preparation timeouts: specify whether a failed BareMetalInstance releases and
cleans up its assigned host or retains ownership until deletion. Align the
behavior with FR-4 and FR-8, and add corresponding acceptance criteria covering
the chosen policy.
- Around line 85-86: Update NFR-1 to remove the unsupported generic
monitoring-capabilities rationale and either identify the specific OSAC
monitoring requirement requiring BCM 10.25.03+ or narrow the stated dependency
to the required metadata and API support. Keep the BCM version floor unchanged
unless the documented OSAC requirements demonstrate a different minimum.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eb9b6d12-91a4-4690-bc6f-194728b0201a
📒 Files selected for processing (1)
enhancements/OSAC-1339-bcm-backend/prd.md
989c760 to
2568929
Compare
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: A well-structured PRD with clear user need, strong business justification, and focused scope. Design leakage through repeated references to BCM's internal 'extra_values' field in requirements and acceptance criteria is the primary weakness. Feedback: Remove references to BCM's 'extra_values' from functional requirements and acceptance criteria — rewrite these as user-observable outcomes (e.g., 'the host is released back to BCM's available pool' without naming the internal field). Move retry/backoff details from FR-6 to the design document; the PRD should state the user-observable behavior (clear status during preparation, error on timeout) without prescribing the mechanism. Consider replacing the 'All Users' persona heading with the standard OSAC persona names (Tenant Admin, Tenant User) for consistency with osac-dimensions.md. Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
enhancements/OSAC-1339-bcm-backend/prd.md (1)
57-63: 🎯 Functional Correctness | 🟠 MajorMake LiteNode-only discovery and label-based matching explicit.
The generic “node type” filter conflicts with the LiteNode-only scope and leaves host-type matching ambiguous. Require discovery to return available LiteNodes only, while requested host types are matched using administrator-assigned labels; keep exact BCM field mapping in the design document.
Based on learnings, PRDs should state behavioral requirements without prescribing implementation-specific field definitions.
🤖 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-1339-bcm-backend/prd.md` around lines 57 - 63, Update FR-4 to explicitly require discovery of available LiteNodes only, excluding hosts assigned to other instances. Specify that requested host types are matched against administrator-assigned labels, and defer the exact BCM field mapping to the design document rather than defining implementation-specific fields in the PRD.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-1339-bcm-backend/prd.md`:
- Around line 94-95: Update the NFR-4 acceptance checklist to explicitly verify
node-registration documentation, including references to the existing CaaS setup
scripts, or remove the node-registration requirement from NFR-4 so the
requirement and acceptance coverage remain consistent.
- Line 99: Update the acceptance criterion to require the operator configuration
to reference a Kubernetes Secret containing the BCM mTLS credentials, rather
than allowing certificate or private-key material to be provided inline; retain
the BCM endpoint and successful connection requirements.
- Line 57: The PRD still contains unresolved [Clarify: ...] markers across the
BCM requirements. Update each marker in the affected requirements to include the
finalized clarification text or a stable reference to the tracked decision,
preserving the surrounding requirement content and removing all opaque
placeholders.
---
Duplicate comments:
In `@enhancements/OSAC-1339-bcm-backend/prd.md`:
- Around line 57-63: Update FR-4 to explicitly require discovery of available
LiteNodes only, excluding hosts assigned to other instances. Specify that
requested host types are matched against administrator-assigned labels, and
defer the exact BCM field mapping to the design document rather than defining
implementation-specific fields in the PRD.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 15f1acbf-703b-4879-8b90-b7e25cb03d3c
📒 Files selected for processing (1)
enhancements/OSAC-1339-bcm-backend/prd.md
2568929 to
d61e80d
Compare
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: A well-structured PRD with clear persona coverage, concrete business justification, and focused scope, held back from a perfect score only by design leakage through references to BCM's internal Feedback: Remove all references to BCM's Critical (0)None. Important (2)
Suggestions (3)
Review costModel: claude-opus-4-6 |
carbonin
left a comment
carbonin
left a comment
There was a problem hiding this comment.
I think generally we could phrase the functional requirements a bit better. Right now they read more just like topics rather than actions.
| ### Cloud Infrastructure Admin | ||
|
|
||
| - As a Cloud Infrastructure Admin, I want to configure BCM as the inventory backend so that OSAC can discover and provision bare metal hosts from my BCM-managed infrastructure. | ||
| - As a Cloud Infrastructure Admin, I want to register bare metal machines as LiteNodes in BCM using BCM's own tools so that OSAC can discover and match them to tenant requests. |
There was a problem hiding this comment.
Are LiteNodes specifically relevant? What are the alternatives? Does it matter to us how they manage/register nodes in BCM?
|
|
||
| - **Sysinfo-based hardware auto-classification.** Host type matching uses admin-assigned labels. Auto-detection from BCM sysinfo is out of scope. | ||
| - **CaaS/BMaaS coordination.** CaaS will consume BCM nodes through the BMaaS API in the near future, so the BCM pool will be handled by BMaaS only. | ||
| - **BCM power control via BCM API.** Power control uses Metal3/BMH exclusively. |
There was a problem hiding this comment.
I think this is more of an implementation detail, right?
Maybe we should have "power management" as a requirement here, but what component handles that would be a design decision.
| - **Sysinfo-based hardware auto-classification.** Host type matching uses admin-assigned labels. Auto-detection from BCM sysinfo is out of scope. | ||
| - **CaaS/BMaaS coordination.** CaaS will consume BCM nodes through the BMaaS API in the near future, so the BCM pool will be handled by BMaaS only. | ||
| - **BCM power control via BCM API.** Power control uses Metal3/BMH exclusively. | ||
| - **PhysicalNode support.** Only LiteNode is supported; OSAC manages the OS. |
There was a problem hiding this comment.
Same here. Unless this somehow impacts the requirements I'm not sure it's relevant to this doc.
| - **CaaS/BMaaS coordination.** CaaS will consume BCM nodes through the BMaaS API in the near future, so the BCM pool will be handled by BMaaS only. | ||
| - **BCM power control via BCM API.** Power control uses Metal3/BMH exclusively. | ||
| - **PhysicalNode support.** Only LiteNode is supported; OSAC manages the OS. | ||
| - **Automated node registration.** Cloud Infrastructure Admins pre-register LiteNodes in BCM as a Day-0 prerequisite. |
There was a problem hiding this comment.
I think this is at most a prereq or docs issue, right?
| Networking is independent of the inventory backend. The existing OSAC networking stack handles all network operations. BCM has no networking role. | ||
|
|
||
| **NFR-4: Documentation.** | ||
| An operator configuration guide documents how to set up the BCM backend. Node registration documentation describes how to pre-register LiteNodes in BCM. |
There was a problem hiding this comment.
Not sure we need to mention LiteNodes specifically here, but yeah we should document whatever the configuration is that we need in BCM to make this work.
| - [ ] An operator configuration guide documents how to set up the BCM backend. | ||
| - [ ] Node registration documentation describes how to pre-register LiteNodes in BCM. |
There was a problem hiding this comment.
These can probably be the same bullet. Something like "documentation describes how to configure OSAC to use the BCM backend"
|
|
||
| ## Assumptions | ||
|
|
||
| - Cloud Infrastructure Admins pre-register LiteNodes in BCM before OSAC operates against them (Day-0 prerequisite). |
There was a problem hiding this comment.
Don't need to mention LiteNodes specifically
| - Cloud Infrastructure Admins pre-register LiteNodes in BCM before OSAC operates against them (Day-0 prerequisite). | ||
| - Each deployment uses a single inventory backend; BCM and OpenStack do not run simultaneously. | ||
| - The pluggable backend interface (OSAC-1032) can accommodate a new inventory backend with host preparation delays. | ||
|
|
There was a problem hiding this comment.
This still needs to be handled. I unresolved the comment. I would just say that we need to write "metadata for host assignment" the exact key/value can wait for design.
| ## Dependencies | ||
|
|
||
| - **Pluggable backend interface (OSAC-1032)** — the BCM backend registers against the existing inventory interface. | ||
| - **Bare metal provisioning stack** — existing provisioning and power management services must be operational on the management cluster. |
There was a problem hiding this comment.
Maybe? Might wait for design for this one too.
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
d61e80d to
ed365f8
Compare
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-1339-bcm-backend/prd.md`:
- Around line 68-69: Add a blank line immediately before the “### Non-Functional
Requirements” and “## Acceptance Criteria” headings in the PRD, preserving all
surrounding content and heading levels.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 06adbf9a-01f2-4bd0-a8b5-72ce39414655
📒 Files selected for processing (1)
enhancements/OSAC-1339-bcm-backend/prd.md
| **FR-10:** E2E tests validate the full BareMetalInstance lifecycle in CI without requiring a real BCM instance. | ||
| ### Non-Functional Requirements |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines before the headings.
markdownlint MD022 reports missing blank lines before ### Non-Functional Requirements and ## Acceptance Criteria. Add one blank line before each heading to keep the document lint-clean.
Proposed fix
**FR-10:** E2E tests validate the full BareMetalInstance lifecycle in CI without requiring a real BCM instance.
+
### Non-Functional Requirements
@@
Documentation describes how to configure OSAC to use the BCM backend, including any required BCM prerequisites.
+
## Acceptance CriteriaAlso applies to: 81-82
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 69-69: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
🤖 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-1339-bcm-backend/prd.md` around lines 68 - 69, Add a blank
line immediately before the “### Non-Functional Requirements” and “## Acceptance
Criteria” headings in the PRD, preserving all surrounding content and heading
levels.
Source: Linters/SAST tools
AI EP Review: EP-126Score: 9/10 | Verdict: PASS
Verdict: Strong PRD with clear user need, concrete business justification, focused scope, and testable requirements; held back from a perfect score by design leakage in credential management, internal data handling rules, and architecture constraints that belong in the design document. Feedback: Rewrite FR-3 to describe the user-observable outcome ('A Cloud Infrastructure Admin provides connection credentials during configuration; the system authenticates securely to BCM') and move the mTLS/Kubernetes Secrets prescription to the design document. Rewrite FR-5, NFR-2, and NFR-3 as user-observable properties — e.g., 'BCM admins cannot identify which tenant owns a given host' instead of describing what data is stored. The acceptance criterion 'No tenant-identifying data appears in BCM' should be reframed as a verifiable user scenario rather than a data-level assertion. Critical (0)None. Important (3)
Suggestions (3)
Review costModel: claude-opus-4-6 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, mennyaboush 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 |
8 participants
PRD: BCM Backend Integration for BMaaS
Jira: https://redhat.atlassian.net/browse/OSAC-1339
Summary
This PRD defines requirements for adding NVIDIA Base Command Manager (BCM) as a bare metal provisioning backend for OSAC. The hybrid architecture queries BCM directly for host discovery and delegates power control and OS management to Metal3. BCM is transparent to tenants — only Cloud Infrastructure Admin and Cloud Provider Admin personas are affected.
Key Decisions
extra_valuesfieldRequesting Review On
How to Review
Summary by CodeRabbit