chore: Expose BMC IP Address Option for Expected Components#445
Merged
chet merged 1 commit intoNVIDIA:mainfrom May 4, 2026
Merged
chore: Expose BMC IP Address Option for Expected Components#445chet merged 1 commit intoNVIDIA:mainfrom
chet merged 1 commit intoNVIDIA:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an optional BmcIpAddress field and propagates it through API models/validation, handlers, DB models/DAO/migration, SDK models, OpenAPI, and workflow mapping; ExpectedPowerShelf persisted column renamed to bmc_ip_address. Tests updated to cover validation and propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Handler as API Handler
participant DB as Database
participant Temporal as Temporal Workflow
participant Reconciler
Client->>API_Handler: POST/PUT { ..., bmcIpAddress? }
API_Handler->>DB: Create/UpdateInput { ..., BmcIpAddress: ptr or nil }
DB-->>API_Handler: persisted record { ..., BmcIpAddress: ptr or nil }
API_Handler->>Temporal: StartWorkflow { Expected{ BmcIpAddress (if non-nil) } }
Temporal->>Reconciler: Reconcile(payload uses BmcIpAddress if set)
Reconciler->>DB: Persist reconciliation (may set/clear bmc_ip_address)
DB-->>Reconciler: ack
Temporal-->>API_Handler: workflow started/ack
API_Handler-->>Client: 201/200 { ..., bmcIpAddress?: value }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-04-30 07:22:34 UTC | Commit: 1fff94e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go (1)
187-191: ⚡ Quick winAdd explicit DB assertion for
BmcIpAddressin the update verification path.At Line 187, you introduce BMC IP update cases, but the verification loop only checks MAC + labels. Add a direct
updated.BmcIpAddressassertion so these scenarios cannot false-pass.Proposed test assertion addition
if ctrlEPS != nil { assert.Equal(t, ctrlEPS.BmcMacAddress, updated.BmcMacAddress, fmt.Sprintf("ExpectedPowerShelf %v should have been updated", eps.ID)) + if ctrlEPS.BmcIpAddress == "" { + assert.Nil(t, updated.BmcIpAddress, + fmt.Sprintf("ExpectedPowerShelf %v should have nil BmcIpAddress", eps.ID)) + } else { + assert.NotNil(t, updated.BmcIpAddress, + fmt.Sprintf("ExpectedPowerShelf %v should have BmcIpAddress set", eps.ID)) + assert.Equal(t, ctrlEPS.BmcIpAddress, *updated.BmcIpAddress, + fmt.Sprintf("ExpectedPowerShelf %v BmcIpAddress should match", eps.ID)) + } // Verify labels are updated correctly expectedLabels := getLabelsMapFromProto(ctrlEPS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go` around lines 187 - 191, The verification loop currently only asserts MAC and labels and can miss BMC IP updates; update the test's verification path (where you iterate updated entries and compare against ctrlExpectedPowerShelf/pagedExpectedPowerShelves) to include an explicit assertion that updated.BmcIpAddress equals the expected value (e.g., ctrlExpectedPowerShelf.BmcIpAddress for the i==2 case) so BmcIpAddress updates are validated alongside MAC and labels.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/handler/expectedswitch_test.go`:
- Around line 300-304: The BmcIpAddress assertions are currently nested inside
the labels-only response conditional, which skips BmcIpAddress checks when
labels are absent; update the test in expectedswitch_test.go to decouple the
BmcIpAddress check from the labels block by moving the
tt.requestBody.BmcIpAddress != nil / response.BmcIpAddress assertions out of the
labels-only conditional so they always run when a request BmcIpAddress is set
(use the existing tt.requestBody.BmcIpAddress and response.BmcIpAddress checks
and keep the assert.NotNil + assert.Equal logic).
In `@api/pkg/api/model/expectedpowershelf.go`:
- Around line 45-47: The model currently uses only BmcIpAddress (field
BmcIpAddress on ExpectedPowerShelf) which will break clients still sending
ipAddress; add backward-compatible JSON handling by implementing a custom
UnmarshalJSON on ExpectedPowerShelf that accepts either "bmcIpAddress" or legacy
"ipAddress" and sets BmcIpAddress accordingly, and implement a custom
MarshalJSON that writes both "bmcIpAddress" and a deprecated "ipAddress" alias
in responses; update any request-parsing helpers that reference BmcIpAddress
(the parsing functions around the earlier diff) to rely on the struct
unmarshaler so both keys work without changing call sites.
In `@openapi/spec.yaml`:
- Line 17439: Update the description for the bmcIpAddress OpenAPI field to
explicitly state both behaviors: that it is an optional IPv4/IPv6 BMC address
which, when used in internal networks, pre-allocates/reserves an IP for the BMC,
and that in external-network scenarios it may represent an externally-managed
BMC IP (i.e., the API accepts it but does not perform allocation). Apply this
clarified wording to every bmcIpAddress occurrence in the spec to keep the API
contract consistent.
- Around line 17435-17439: Update the bmcIpAddress schema in openapi/spec.yaml
so it enforces IPv4/IPv6 formats instead of plain string|null: replace the
simple type union with a oneOf that lists an object with type: string and
format: ipv4, another with type: string and format: ipv6, and allow null
(nullable or include "null" in the oneOf as appropriate to your OpenAPI
version); apply the same pattern to every other occurrence of the bmcIpAddress
property referenced in the review (the other blocks at the noted positions) so
generated clients/validators will enforce the IP format contract.
---
Nitpick comments:
In `@workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go`:
- Around line 187-191: The verification loop currently only asserts MAC and
labels and can miss BMC IP updates; update the test's verification path (where
you iterate updated entries and compare against
ctrlExpectedPowerShelf/pagedExpectedPowerShelves) to include an explicit
assertion that updated.BmcIpAddress equals the expected value (e.g.,
ctrlExpectedPowerShelf.BmcIpAddress for the i==2 case) so BmcIpAddress updates
are validated alongside MAC and labels.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 85a18eba-a4c5-403c-adf9-41e38e37feaa
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (32)
api/pkg/api/handler/expectedmachine.goapi/pkg/api/handler/expectedmachine_test.goapi/pkg/api/handler/expectedpowershelf.goapi/pkg/api/handler/expectedpowershelf_test.goapi/pkg/api/handler/expectedswitch.goapi/pkg/api/handler/expectedswitch_test.goapi/pkg/api/model/expectedmachine.goapi/pkg/api/model/expectedmachine_test.goapi/pkg/api/model/expectedpowershelf.goapi/pkg/api/model/expectedpowershelf_test.goapi/pkg/api/model/expectedswitch.goapi/pkg/api/model/expectedswitch_test.godb/pkg/db/model/expectedmachine.godb/pkg/db/model/expectedmachine_test.godb/pkg/db/model/expectedpowershelf.godb/pkg/db/model/expectedpowershelf_test.godb/pkg/db/model/expectedswitch.godb/pkg/db/model/expectedswitch_test.godb/pkg/migrations/20260430070805_expected_components_bmc_ip_address.godocs/index.htmlopenapi/spec.yamlsdk/standard/model_expected_machine.gosdk/standard/model_expected_machine_create_request.gosdk/standard/model_expected_machine_update_request.gosdk/standard/model_expected_power_shelf.gosdk/standard/model_expected_power_shelf_create_request.gosdk/standard/model_expected_power_shelf_update_request.gosdk/standard/model_expected_switch.gosdk/standard/model_expected_switch_create_request.gosdk/standard/model_expected_switch_update_request.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go
Comment on lines
+45
to
47
| // BmcIpAddress is the BMC IP address of the expected power shelf | ||
| BmcIpAddress *string `json:"bmcIpAddress"` | ||
| // RackID is the optional rack identifier |
Contributor
There was a problem hiding this comment.
Preserve backward compatibility for legacy ipAddress clients.
Line 45 and Line 127 move request parsing to bmcIpAddress only, and Line 223 does the same for responses. That can break existing clients still using ipAddress (request values get ignored; response key disappears). Please keep a deprecation window with alias support (or explicitly mark/version this as a breaking API change).
Suggested compatibility approach
type APIExpectedPowerShelfCreateRequest struct {
+ // Deprecated: kept for backward compatibility; prefer BmcIpAddress.
+ IpAddress *string `json:"ipAddress,omitempty"`
BmcIpAddress *string `json:"bmcIpAddress"`
}
func (epcr *APIExpectedPowerShelfCreateRequest) Validate() error {
+ if epcr.BmcIpAddress == nil && epcr.IpAddress != nil {
+ epcr.BmcIpAddress = epcr.IpAddress
+ }
+ if epcr.BmcIpAddress != nil && epcr.IpAddress != nil && *epcr.BmcIpAddress != *epcr.IpAddress {
+ return validation.Errors{"bmcIpAddress": errors.New("bmcIpAddress and ipAddress must match when both are set")}
+ }
// existing validation...
}Also applies to: 127-129, 223-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/pkg/api/model/expectedpowershelf.go` around lines 45 - 47, The model
currently uses only BmcIpAddress (field BmcIpAddress on ExpectedPowerShelf)
which will break clients still sending ipAddress; add backward-compatible JSON
handling by implementing a custom UnmarshalJSON on ExpectedPowerShelf that
accepts either "bmcIpAddress" or legacy "ipAddress" and sets BmcIpAddress
accordingly, and implement a custom MarshalJSON that writes both "bmcIpAddress"
and a deprecated "ipAddress" alias in responses; update any request-parsing
helpers that reference BmcIpAddress (the parsing functions around the earlier
diff) to rely on the struct unmarshaler so both keys work without changing call
sites.
Comment on lines
+17435
to
+17439
| bmcIpAddress: | ||
| type: | ||
| - string | ||
| - 'null' | ||
| description: Optional BMC IP address (IPv4 or IPv6). When set, pre-allocates a reserved IP for the BMC. |
Contributor
There was a problem hiding this comment.
Add machine-readable IP constraints for bmcIpAddress.
Right now bmcIpAddress is string|null, but the description promises IPv4/IPv6 semantics. Please model that in schema (e.g., oneOf with format: ipv4 / format: ipv6, plus nullable) so generated clients and validators enforce the contract.
Suggested OpenAPI shape
bmcIpAddress:
- type:
- - string
- - 'null'
- description: Optional BMC IP address (IPv4 or IPv6). When set, pre-allocates a reserved IP for the BMC.
+ oneOf:
+ - type: string
+ format: ipv4
+ - type: string
+ format: ipv6
+ - type: 'null'
+ description: Optional BMC IP address (IPv4 or IPv6). When set, pre-allocates a reserved IP for the BMC.As per coding guidelines: "openapi/spec.yaml: Review the OpenAPI specification, check for consistency and correctness, check for misspellings and grammatical errors."
Also applies to: 17562-17566, 17692-17696, 17781-17785, 17892-17896, 18001-18005, 18100-18104, 18223-18227, 18343-18347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openapi/spec.yaml` around lines 17435 - 17439, Update the bmcIpAddress schema
in openapi/spec.yaml so it enforces IPv4/IPv6 formats instead of plain
string|null: replace the simple type union with a oneOf that lists an object
with type: string and format: ipv4, another with type: string and format: ipv6,
and allow null (nullable or include "null" in the oneOf as appropriate to your
OpenAPI version); apply the same pattern to every other occurrence of the
bmcIpAddress property referenced in the review (the other blocks at the noted
positions) so generated clients/validators will enforce the IP format contract.
chet
force-pushed
the
expected-bmc-ip-address
branch
2 times, most recently
from
811c516 to
3db1a42
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openapi/spec.yaml (1)
17439-17439:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd external-network semantics to every
bmcIpAddressdescription.These descriptions currently only cover reserved/pre-allocation behavior. Please also document that in external networks this field may be an externally managed BMC IP (accepted without allocation), so the OpenAPI contract matches the intended behavior.
As per coding guidelines: "Ensure the BMC IP field is optional (
string|null) in both the main resource schemas and create/update request shapes, and that its description reflects the reserved/external allocation behavior..."Also applies to: 17566-17566, 17696-17696, 17785-17785, 17896-17896, 18005-18005, 18104-18104, 18227-18227, 18347-18347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openapi/spec.yaml` at line 17439, Update every bmcIpAddress property description and type so the field is optional (type string|null) in both main resource schemas and create/update request objects; change the description for each bmcIpAddress to state that when provided on internal networks it pre-allocates/reserves a BMC IP, and when provided on external networks it may be an externally managed BMC IP accepted without allocation (and will not trigger reservation), and include guidance about validation/format (IPv4 or IPv6). Locate each bmcIpAddress schema entry (the bmcIpAddress property in the resource schemas and the create/update request shapes) and modify its type to allow null and expand its description to cover both reserved/pre-allocation and external-network semantics; apply the same change to all other bmcIpAddress occurrences referenced in the review.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@openapi/spec.yaml`:
- Line 17439: Update every bmcIpAddress property description and type so the
field is optional (type string|null) in both main resource schemas and
create/update request objects; change the description for each bmcIpAddress to
state that when provided on internal networks it pre-allocates/reserves a BMC
IP, and when provided on external networks it may be an externally managed BMC
IP accepted without allocation (and will not trigger reservation), and include
guidance about validation/format (IPv4 or IPv6). Locate each bmcIpAddress schema
entry (the bmcIpAddress property in the resource schemas and the create/update
request shapes) and modify its type to allow null and expand its description to
cover both reserved/pre-allocation and external-network semantics; apply the
same change to all other bmcIpAddress occurrences referenced in the review.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1fb0564-ccf8-4f70-bd6b-efaabc00d808
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (32)
api/pkg/api/handler/expectedmachine.goapi/pkg/api/handler/expectedmachine_test.goapi/pkg/api/handler/expectedpowershelf.goapi/pkg/api/handler/expectedpowershelf_test.goapi/pkg/api/handler/expectedswitch.goapi/pkg/api/handler/expectedswitch_test.goapi/pkg/api/model/expectedmachine.goapi/pkg/api/model/expectedmachine_test.goapi/pkg/api/model/expectedpowershelf.goapi/pkg/api/model/expectedpowershelf_test.goapi/pkg/api/model/expectedswitch.goapi/pkg/api/model/expectedswitch_test.godb/pkg/db/model/expectedmachine.godb/pkg/db/model/expectedmachine_test.godb/pkg/db/model/expectedpowershelf.godb/pkg/db/model/expectedpowershelf_test.godb/pkg/db/model/expectedswitch.godb/pkg/db/model/expectedswitch_test.godb/pkg/migrations/20260430070805_expected_components_bmc_ip_address.godocs/index.htmlopenapi/spec.yamlsdk/standard/model_expected_machine.gosdk/standard/model_expected_machine_create_request.gosdk/standard/model_expected_machine_update_request.gosdk/standard/model_expected_power_shelf.gosdk/standard/model_expected_power_shelf_create_request.gosdk/standard/model_expected_power_shelf_update_request.gosdk/standard/model_expected_switch.gosdk/standard/model_expected_switch_create_request.gosdk/standard/model_expected_switch_update_request.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go
✅ Files skipped from review due to trivial changes (7)
- api/pkg/api/handler/expectedpowershelf_test.go
- api/pkg/api/handler/expectedswitch_test.go
- api/pkg/api/handler/expectedswitch.go
- db/pkg/db/model/expectedmachine.go
- api/pkg/api/handler/expectedmachine.go
- api/pkg/api/model/expectedmachine.go
- sdk/standard/model_expected_power_shelf.go
🚧 Files skipped from review as they are similar to previous changes (9)
- api/pkg/api/handler/expectedmachine_test.go
- workflow/pkg/activity/expectedpowershelf/expectedpowershelf.go
- db/pkg/migrations/20260430070805_expected_components_bmc_ip_address.go
- workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go
- api/pkg/api/model/expectedswitch_test.go
- db/pkg/db/model/expectedpowershelf_test.go
- db/pkg/db/model/expectedpowershelf.go
- api/pkg/api/model/expectedpowershelf.go
- api/pkg/api/model/expectedpowershelf_test.go
Comment on lines
+32
to
+33
| // Optional BMC IP address (IPv4 or IPv6). When set, pre-allocates a reserved IP for the BMC. | ||
| BmcIpAddress NullableString `json:"bmcIpAddress,omitempty"` |
Contributor
There was a problem hiding this comment.
Renaming ipAddress to bmcIpAddress is a breaking SDK/API contract change.
At Line 33 and in the renamed helpers/serialization blocks (Line 288 onward and Line 775 onward), the old field/key surface is removed. Existing clients using ipAddress / GetIpAddress* will break. Please add a transition path (deprecated alias support) or explicitly mark/version this as a breaking change in release notes.
Also applies to: 288-329, 775-777
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/pkg/api/model/expectedswitch.go (1)
259-268: ⚡ Quick winAdd a positive mapping assertion for non-nil
BmcIpAddressin unit tests.
NewAPIExpectedSwitchnow mapsdbModel.BmcIpAddress; adding an explicit non-nil assertion inapi/pkg/api/model/expectedswitch_test.gowould lock this path against regression.Suggested test update
diff --git a/api/pkg/api/model/expectedswitch_test.go b/api/pkg/api/model/expectedswitch_test.go @@ func TestNewAPIExpectedSwitch(t *testing.T) { + bmcIP := "192.168.1.10" dbES := &cdbm.ExpectedSwitch{ BmcMacAddress: "00:11:22:33:44:55", SwitchSerialNumber: "SWITCH123", + BmcIpAddress: &bmcIP, Labels: map[string]string{"env": "test", "zone": "us-west-1"}, Created: cdb.GetCurTime(), Updated: cdb.GetCurTime(), } @@ assert.Equal(t, tc.dbObj.BmcMacAddress, got.BmcMacAddress) assert.Equal(t, tc.dbObj.SwitchSerialNumber, got.SwitchSerialNumber) + assert.Equal(t, tc.dbObj.BmcIpAddress, got.BmcIpAddress) assert.Equal(t, tc.dbObj.Labels, got.Labels) assert.Equal(t, tc.dbObj.Created, got.Created) assert.Equal(t, tc.dbObj.Updated, got.Updated)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/model/expectedswitch.go` around lines 259 - 268, NewAPIExpectedSwitch now maps dbModel.BmcIpAddress into APIExpectedSwitch.BmcIpAddress but the unit tests lack a positive assertion to guard that mapping; update api/pkg/api/model/expectedswitch_test.go to create a cdbm.ExpectedSwitch with a non-nil BmcIpAddress, call NewAPIExpectedSwitch, and assert that the returned APIExpectedSwitch.BmcIpAddress equals the input value (use the test helper/assertion style used elsewhere), referencing NewAPIExpectedSwitch and APIExpectedSwitch.BmcIpAddress to locate where to add the assertion.workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go (1)
454-464: ⚡ Quick winAdd
BmcIpAddressassertions in creation verification path as well.
BmcIpAddressis asserted for updates, but the creation-verification loop currently checks labels only. Adding a parallel assertion there would close the regression gap for create flow behavior in this PR.Proposed test addition
@@ // Verify newly created power shelves have correct labels for _, ceps := range tt.args.expectedPowerShelfInventory.ExpectedPowerShelves { epsID, perr := uuid.Parse(ceps.ExpectedPowerShelfId.Value) assert.NoError(t, perr) created := powerShelvesByID[epsID] if created != nil { + if ceps.BmcIpAddress == "" { + assert.Nil(t, created.BmcIpAddress, + fmt.Sprintf("ExpectedPowerShelf %v should have nil BmcIpAddress on creation", epsID)) + } else if assert.NotNil(t, created.BmcIpAddress, + fmt.Sprintf("ExpectedPowerShelf %v should have BmcIpAddress set on creation", epsID)) { + assert.Equal(t, ceps.BmcIpAddress, *created.BmcIpAddress, + fmt.Sprintf("ExpectedPowerShelf %v BmcIpAddress should match on creation", epsID)) + } + expectedLabels := getLabelsMapFromProto(ceps) // Both nil and empty maps should be treated as equivalent (no labels) if len(expectedLabels) == 0 && len(created.Labels) == 0 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go` around lines 454 - 464, The creation-verification loop currently only checks labels and omits assertions for BmcIpAddress; add the same BmcIpAddress checks used in the update path (referencing ctrlEPS, updated and eps.ID) into the creation verification block so that if ctrlEPS.BmcIpAddress == "" you assert updated.BmcIpAddress is nil, otherwise assert updated.BmcIpAddress is non-nil and equals ctrlEPS.BmcIpAddress, mirroring the exact logic used in the update verification to cover create-flow regressions.db/pkg/db/model/expectedmachine_test.go (1)
114-195: ⚡ Quick winAdd focused update/clear regression tests for
BmcIpAddress.This change verifies create round-trip, but the PR also updates DAO update/clear behavior for the same field. Please add explicit tests for set/update and clear flows to prevent silent regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/pkg/db/model/expectedmachine_test.go` around lines 114 - 195, Add focused tests for BmcIpAddress update and clear flows: extend expectedmachine_test.go to include cases that (1) create an ExpectedMachine via emsd.Create with BmcIpAddress set, then call the service's update method (e.g., emsd.Update or UpdateExpectedMachine) to change BmcIpAddress and assert the returned/loaded record (via emsd.Get or GetByID) has the new value, and (2) create with BmcIpAddress set, then update to clear it (set nil or empty depending on semantics) and assert the persisted record has BmcIpAddress cleared. Reference the BmcIpAddress field and the emsd.Create/emsd.Update/emsd.Get functions to locate where to add the new test cases and assertions. Ensure tests cover both non-nil -> non-nil update and non-nil -> nil clear behavior and include appropriate expectError checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/pkg/db/model/expectedmachine.go`:
- Around line 535-538: In UpdateMultiple on ExpectedMachine, add a validation
that prevents mixed presence of BmcIpAddress across the batch: iterate the slice
of inputs before building columnsSet and track two flags (seenSet, seenUnset)
for input.BmcIpAddress; if both become true, return an error (e.g.,
ErrMixedFieldPresence) instead of proceeding. This ensures UpdateMultiple does
not union columns and unintentionally set bmc_ip_address to NULL for rows where
the field was omitted; reference the UpdateMultiple function, the
ExpectedMachine type, the input.BmcIpAddress field and the existing columnsSet
logic when implementing the guard.
---
Nitpick comments:
In `@api/pkg/api/model/expectedswitch.go`:
- Around line 259-268: NewAPIExpectedSwitch now maps dbModel.BmcIpAddress into
APIExpectedSwitch.BmcIpAddress but the unit tests lack a positive assertion to
guard that mapping; update api/pkg/api/model/expectedswitch_test.go to create a
cdbm.ExpectedSwitch with a non-nil BmcIpAddress, call NewAPIExpectedSwitch, and
assert that the returned APIExpectedSwitch.BmcIpAddress equals the input value
(use the test helper/assertion style used elsewhere), referencing
NewAPIExpectedSwitch and APIExpectedSwitch.BmcIpAddress to locate where to add
the assertion.
In `@db/pkg/db/model/expectedmachine_test.go`:
- Around line 114-195: Add focused tests for BmcIpAddress update and clear
flows: extend expectedmachine_test.go to include cases that (1) create an
ExpectedMachine via emsd.Create with BmcIpAddress set, then call the service's
update method (e.g., emsd.Update or UpdateExpectedMachine) to change
BmcIpAddress and assert the returned/loaded record (via emsd.Get or GetByID) has
the new value, and (2) create with BmcIpAddress set, then update to clear it
(set nil or empty depending on semantics) and assert the persisted record has
BmcIpAddress cleared. Reference the BmcIpAddress field and the
emsd.Create/emsd.Update/emsd.Get functions to locate where to add the new test
cases and assertions. Ensure tests cover both non-nil -> non-nil update and
non-nil -> nil clear behavior and include appropriate expectError checks.
In `@workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go`:
- Around line 454-464: The creation-verification loop currently only checks
labels and omits assertions for BmcIpAddress; add the same BmcIpAddress checks
used in the update path (referencing ctrlEPS, updated and eps.ID) into the
creation verification block so that if ctrlEPS.BmcIpAddress == "" you assert
updated.BmcIpAddress is nil, otherwise assert updated.BmcIpAddress is non-nil
and equals ctrlEPS.BmcIpAddress, mirroring the exact logic used in the update
verification to cover create-flow regressions.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ca86adfa-91e6-4421-9bbe-e35d5d94c239
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (32)
api/pkg/api/handler/expectedmachine.goapi/pkg/api/handler/expectedmachine_test.goapi/pkg/api/handler/expectedpowershelf.goapi/pkg/api/handler/expectedpowershelf_test.goapi/pkg/api/handler/expectedswitch.goapi/pkg/api/handler/expectedswitch_test.goapi/pkg/api/model/expectedmachine.goapi/pkg/api/model/expectedmachine_test.goapi/pkg/api/model/expectedpowershelf.goapi/pkg/api/model/expectedpowershelf_test.goapi/pkg/api/model/expectedswitch.goapi/pkg/api/model/expectedswitch_test.godb/pkg/db/model/expectedmachine.godb/pkg/db/model/expectedmachine_test.godb/pkg/db/model/expectedpowershelf.godb/pkg/db/model/expectedpowershelf_test.godb/pkg/db/model/expectedswitch.godb/pkg/db/model/expectedswitch_test.godb/pkg/migrations/20260430070805_expected_components_bmc_ip_address.godocs/index.htmlopenapi/spec.yamlsdk/standard/model_expected_machine.gosdk/standard/model_expected_machine_create_request.gosdk/standard/model_expected_machine_update_request.gosdk/standard/model_expected_power_shelf.gosdk/standard/model_expected_power_shelf_create_request.gosdk/standard/model_expected_power_shelf_update_request.gosdk/standard/model_expected_switch.gosdk/standard/model_expected_switch_create_request.gosdk/standard/model_expected_switch_update_request.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go
✅ Files skipped from review due to trivial changes (4)
- api/pkg/api/model/expectedmachine_test.go
- sdk/standard/model_expected_power_shelf.go
- sdk/standard/model_expected_power_shelf_update_request.go
- openapi/spec.yaml
🚧 Files skipped from review as they are similar to previous changes (12)
- db/pkg/db/model/expectedswitch_test.go
- api/pkg/api/handler/expectedpowershelf_test.go
- api/pkg/api/handler/expectedswitch.go
- api/pkg/api/handler/expectedmachine_test.go
- db/pkg/migrations/20260430070805_expected_components_bmc_ip_address.go
- api/pkg/api/handler/expectedswitch_test.go
- workflow/pkg/activity/expectedpowershelf/expectedpowershelf.go
- api/pkg/api/handler/expectedmachine.go
- api/pkg/api/handler/expectedpowershelf.go
- api/pkg/api/model/expectedpowershelf.go
- db/pkg/db/model/expectedpowershelf.go
- api/pkg/api/model/expectedpowershelf_test.go
Comment on lines
+535
to
+538
| if input.BmcIpAddress != nil { | ||
| em.BmcIpAddress = input.BmcIpAddress | ||
| columnsSet["bmc_ip_address"] = true | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent unintended bmc_ip_address nulling in heterogeneous batch updates.
Because UpdateMultiple uses a unioned column list across all rows, if any input sets BmcIpAddress, rows where it is omitted will be updated with NULL. This can silently clear data.
Suggested guard (reject mixed presence in one batch)
func (emsd ExpectedMachineSQLDAO) UpdateMultiple(ctx context.Context, tx *db.Tx, inputs []ExpectedMachineUpdateInput) ([]ExpectedMachine, error) {
...
if len(inputs) == 0 {
return []ExpectedMachine{}, nil
}
+
+ hasBmcIpAddress := inputs[0].BmcIpAddress != nil
+ for i := 1; i < len(inputs); i++ {
+ if (inputs[i].BmcIpAddress != nil) != hasBmcIpAddress {
+ return nil, fmt.Errorf("bmc_ip_address must be provided for all rows or none in a single batch update")
+ }
+ }
// Build expected machines and collect columns to update
expectedMachines := make([]*ExpectedMachine, 0, len(inputs))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/pkg/db/model/expectedmachine.go` around lines 535 - 538, In UpdateMultiple
on ExpectedMachine, add a validation that prevents mixed presence of
BmcIpAddress across the batch: iterate the slice of inputs before building
columnsSet and track two flags (seenSet, seenUnset) for input.BmcIpAddress; if
both become true, return an error (e.g., ErrMixedFieldPresence) instead of
proceeding. This ensures UpdateMultiple does not union columns and
unintentionally set bmc_ip_address to NULL for rows where the field was omitted;
reference the UpdateMultiple function, the ExpectedMachine type, the
input.BmcIpAddress field and the existing columnsSet logic when implementing the
guard.
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
force-pushed
the
expected-bmc-ip-address
branch
from
3db1a42 to
c59de37
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go (1)
187-192: ⚡ Quick winAdd one explicit “clear BmcIpAddress” update case.
Current changes validate set and preserve behavior well, but they do not explicitly cover clearing an existing non-nil DB
BmcIpAddresswhen controller sends"". Adding that case would lock in the nil-conversion path.Also applies to: 454-464
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go` around lines 187 - 192, Add an explicit test case that verifies clearing an existing non-nil BmcIpAddress when the controller sends an empty string: in the test block that mutates ctrlExpectedPowerShelf and appends to expectedPowerShelvesToUpdate, add a branch (e.g., when i == X) that sets ctrlExpectedPowerShelf.BmcIpAddress = "" to simulate the controller clearing the value and append that entry to expectedPowerShelvesToUpdate; ensure the assertion later verifies the DB value becomes nil/cleared. Repeat the same clear-case addition in the other similar test section referenced (the block around the other occurrence).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/pkg/migrations/20260430070805_expected_components_bmc_ip_address.go`:
- Around line 45-51: The ALTER RENAME block in the DO $$ transaction that
targets table expected_power_shelf should be made idempotent by checking that
ip_address exists AND bmc_ip_address does NOT exist, and by scoping the
information_schema lookup to the current schema; update the IF condition in the
DO $$ block used in the tx.Exec call so it queries information_schema.columns
WHERE table_name='expected_power_shelf' AND table_schema = current_schema() AND
column_name='ip_address' AND NOT EXISTS (SELECT 1 FROM
information_schema.columns WHERE table_name='expected_power_shelf' AND
table_schema = current_schema() AND column_name='bmc_ip_address'), then perform
ALTER TABLE expected_power_shelf RENAME COLUMN ip_address TO bmc_ip_address
inside that guarded branch.
---
Nitpick comments:
In `@workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go`:
- Around line 187-192: Add an explicit test case that verifies clearing an
existing non-nil BmcIpAddress when the controller sends an empty string: in the
test block that mutates ctrlExpectedPowerShelf and appends to
expectedPowerShelvesToUpdate, add a branch (e.g., when i == X) that sets
ctrlExpectedPowerShelf.BmcIpAddress = "" to simulate the controller clearing the
value and append that entry to expectedPowerShelvesToUpdate; ensure the
assertion later verifies the DB value becomes nil/cleared. Repeat the same
clear-case addition in the other similar test section referenced (the block
around the other occurrence).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c9f2fb92-d7a4-4595-84cd-3bc499beb3d0
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (32)
api/pkg/api/handler/expectedmachine.goapi/pkg/api/handler/expectedmachine_test.goapi/pkg/api/handler/expectedpowershelf.goapi/pkg/api/handler/expectedpowershelf_test.goapi/pkg/api/handler/expectedswitch.goapi/pkg/api/handler/expectedswitch_test.goapi/pkg/api/model/expectedmachine.goapi/pkg/api/model/expectedmachine_test.goapi/pkg/api/model/expectedpowershelf.goapi/pkg/api/model/expectedpowershelf_test.goapi/pkg/api/model/expectedswitch.goapi/pkg/api/model/expectedswitch_test.godb/pkg/db/model/expectedmachine.godb/pkg/db/model/expectedmachine_test.godb/pkg/db/model/expectedpowershelf.godb/pkg/db/model/expectedpowershelf_test.godb/pkg/db/model/expectedswitch.godb/pkg/db/model/expectedswitch_test.godb/pkg/migrations/20260430070805_expected_components_bmc_ip_address.godocs/index.htmlopenapi/spec.yamlsdk/standard/model_expected_machine.gosdk/standard/model_expected_machine_create_request.gosdk/standard/model_expected_machine_update_request.gosdk/standard/model_expected_power_shelf.gosdk/standard/model_expected_power_shelf_create_request.gosdk/standard/model_expected_power_shelf_update_request.gosdk/standard/model_expected_switch.gosdk/standard/model_expected_switch_create_request.gosdk/standard/model_expected_switch_update_request.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go
✅ Files skipped from review due to trivial changes (8)
- api/pkg/api/model/expectedmachine_test.go
- api/pkg/api/model/expectedswitch.go
- db/pkg/db/model/expectedmachine.go
- api/pkg/api/handler/expectedpowershelf.go
- db/pkg/db/model/expectedpowershelf.go
- api/pkg/api/model/expectedpowershelf_test.go
- api/pkg/api/model/expectedmachine.go
- sdk/standard/model_expected_power_shelf_create_request.go
🚧 Files skipped from review as they are similar to previous changes (11)
- db/pkg/db/model/expectedswitch_test.go
- api/pkg/api/handler/expectedpowershelf_test.go
- workflow/pkg/activity/expectedpowershelf/expectedpowershelf.go
- sdk/standard/model_expected_machine_update_request.go
- db/pkg/db/model/expectedswitch.go
- api/pkg/api/model/expectedpowershelf.go
- api/pkg/api/handler/expectedswitch.go
- sdk/standard/model_expected_power_shelf_update_request.go
- db/pkg/db/model/expectedmachine_test.go
- db/pkg/db/model/expectedpowershelf_test.go
- sdk/standard/model_expected_power_shelf.go
Contributor
|
@chet I assume we are ok with backwards incompatible |
chet
force-pushed
the
expected-bmc-ip-address
branch
from
c59de37 to
927e77d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
api/pkg/api/model/expectedpowershelf.go (1)
45-46:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve a deprecation path for
ipAddress.This still removes the legacy public JSON key from create, update, and response DTOs. Existing clients sending
ipAddresswill silently drop the value, and older consumers will stop seeing it in responses. Please keep an alias during a deprecation window or explicitly version this as a breaking change.Also applies to: 127-128, 223-224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/model/expectedpowershelf.go` around lines 45 - 46, The JSON key "ipAddress" was removed for the BmcIpAddress field causing backward-compatibility breaks; restore a deprecation alias in the DTOs by exposing the legacy "ipAddress" JSON name alongside the new BmcIpAddress field. Specifically, in the ExpectedPowerShelf struct and the corresponding create/update/response DTO structs (the symbols referencing BmcIpAddress at the shown locations), add a deprecated alias field or struct tag mapping so the object still marshals/unmarshals "ipAddress" (for example by adding an IpAddress *string json tag for "ipAddress" that proxies to BmcIpAddress or by using struct tags that include both keys via a small custom marshal/unmarshal helper), mark it deprecated in comments, and ensure create/update handlers copy the value into BmcIpAddress so both new and legacy clients continue to work during the deprecation window.
🧹 Nitpick comments (3)
api/pkg/api/handler/expectedmachine_test.go (1)
340-341: ⚡ Quick winAdd a workflow-level assertion for
BmcIpAddressin this test path.Current assertions prove DB/API round-trip only. Add capture/assertion on the mocked workflow request so this test fails if
bmcIpAddressis omitted from the workflow proto.Also applies to: 410-414
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/expectedmachine_test.go` around lines 340 - 341, Add an assertion that the mocked workflow request contains the bmcIpAddress field matching the test input BmcIpAddress ("192.168.1.10"); after the code that triggers the workflow (the test that constructs BmcIpAddress: cdb.GetStrPtr("192.168.1.10") and Labels...), capture the mocked workflow client call (the mocked Start/Send/Invoke method used in this test) and assert the proto request has bmcIpAddress set (non-nil) and equals "192.168.1.10" so the test fails if bmcIpAddress is omitted from the workflow proto.api/pkg/api/handler/expectedswitch_test.go (1)
180-180: ⚡ Quick winAdd PATCH coverage for
BmcIpAddress.The new assertions only exercise
POST.TestUpdateExpectedSwitchHandler_Handlestill never sendsBmcIpAddress, so a broken update path would pass this PR. Add one update case that sets the field and asserts it is returned or persisted.Also applies to: 294-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/expectedswitch_test.go` at line 180, TestUpdateExpectedSwitchHandler_Handle lacks an update (PATCH) test that sets BmcIpAddress; add a subcase that issues a PATCH to the update handler with BmcIpAddress set (e.g., "192.168.1.10") and then assert the response body and/or subsequent GET/DB read contains that value. Modify the test cases in TestUpdateExpectedSwitchHandler_Handle (and the similar block around lines referenced 294-306) to include a case that sends BmcIpAddress in the update payload, invokes the update handler, and verifies persistence/response for field BmcIpAddress using the same helper utils/assertions the test suite already uses.workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go (1)
187-190: ⚡ Quick winCover clearing an existing
BmcIpAddress.This table verifies nil→value, but not value→nil. The activity explicitly converts
""tonilbefore DB writes, so a regression in clearing the column would currently slip through. Add one case with a pre-existing DB IP and controllerBmcIpAddress: "", then assert the stored pointer becomesnil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go` around lines 187 - 190, Add a test case in expectedpowershelf_test.go that covers clearing an existing BmcIpAddress: create a pagedExpectedPowerShelves entry where the existing DB record (ctrlExpectedPowerShelf) has a non-empty BmcIpAddress value, then set ctrlExpectedPowerShelf.BmcIpAddress = "" (controller input) and append that entry to expectedPowerShelvesToUpdate; run the activity and assert that the persisted record's BmcIpAddress pointer is nil (verify the code path that converts ""→nil executes), using the same helper/assert pattern as other update cases to locate the updated record and confirm the pointer is nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/pkg/api/handler/expectedmachine.go`:
- Around line 298-299: The workflow payload builder expectedMachineToProto omits
copying em.BmcIpAddress, so single-item create/update workflows lose the BMC IP;
update expectedMachineToProto to set the returned proto's BmcIpAddress from
em.BmcIpAddress (and mirror this in any variant overloads that convert
expectedMachine -> proto used at the single-create/update paths), ensuring the
proto field is populated whenever em.BmcIpAddress is non-empty; reference the
expectedMachineToProto function and the em.BmcIpAddress/proto.BmcIpAddress
fields when making the change.
In `@api/pkg/api/handler/expectedswitch.go`:
- Around line 239-240: The workflow payload is missing the BMC IP because
expectedSwitchToProto never sets proto.BmcIpAddress; update the
expectedSwitchToProto converter to copy the BmcIpAddress from the source model
(the apiRequest/expectedSwitch struct) into proto.BmcIpAddress so the created
payload used when calling the workflow (places that call expectedSwitchToProto
for create/update) includes the BMC IP; ensure both create and update call sites
that currently use expectedSwitchToProto receive the populated
proto.BmcIpAddress.
---
Duplicate comments:
In `@api/pkg/api/model/expectedpowershelf.go`:
- Around line 45-46: The JSON key "ipAddress" was removed for the BmcIpAddress
field causing backward-compatibility breaks; restore a deprecation alias in the
DTOs by exposing the legacy "ipAddress" JSON name alongside the new BmcIpAddress
field. Specifically, in the ExpectedPowerShelf struct and the corresponding
create/update/response DTO structs (the symbols referencing BmcIpAddress at the
shown locations), add a deprecated alias field or struct tag mapping so the
object still marshals/unmarshals "ipAddress" (for example by adding an IpAddress
*string json tag for "ipAddress" that proxies to BmcIpAddress or by using struct
tags that include both keys via a small custom marshal/unmarshal helper), mark
it deprecated in comments, and ensure create/update handlers copy the value into
BmcIpAddress so both new and legacy clients continue to work during the
deprecation window.
---
Nitpick comments:
In `@api/pkg/api/handler/expectedmachine_test.go`:
- Around line 340-341: Add an assertion that the mocked workflow request
contains the bmcIpAddress field matching the test input BmcIpAddress
("192.168.1.10"); after the code that triggers the workflow (the test that
constructs BmcIpAddress: cdb.GetStrPtr("192.168.1.10") and Labels...), capture
the mocked workflow client call (the mocked Start/Send/Invoke method used in
this test) and assert the proto request has bmcIpAddress set (non-nil) and
equals "192.168.1.10" so the test fails if bmcIpAddress is omitted from the
workflow proto.
In `@api/pkg/api/handler/expectedswitch_test.go`:
- Line 180: TestUpdateExpectedSwitchHandler_Handle lacks an update (PATCH) test
that sets BmcIpAddress; add a subcase that issues a PATCH to the update handler
with BmcIpAddress set (e.g., "192.168.1.10") and then assert the response body
and/or subsequent GET/DB read contains that value. Modify the test cases in
TestUpdateExpectedSwitchHandler_Handle (and the similar block around lines
referenced 294-306) to include a case that sends BmcIpAddress in the update
payload, invokes the update handler, and verifies persistence/response for field
BmcIpAddress using the same helper utils/assertions the test suite already uses.
In `@workflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go`:
- Around line 187-190: Add a test case in expectedpowershelf_test.go that covers
clearing an existing BmcIpAddress: create a pagedExpectedPowerShelves entry
where the existing DB record (ctrlExpectedPowerShelf) has a non-empty
BmcIpAddress value, then set ctrlExpectedPowerShelf.BmcIpAddress = ""
(controller input) and append that entry to expectedPowerShelvesToUpdate; run
the activity and assert that the persisted record's BmcIpAddress pointer is nil
(verify the code path that converts ""→nil executes), using the same
helper/assert pattern as other update cases to locate the updated record and
confirm the pointer is nil.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5c7f3673-d5d7-4f3a-9dd1-ec9b869ffd4d
⛔ Files ignored due to path filters (2)
workflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (32)
api/pkg/api/handler/expectedmachine.goapi/pkg/api/handler/expectedmachine_test.goapi/pkg/api/handler/expectedpowershelf.goapi/pkg/api/handler/expectedpowershelf_test.goapi/pkg/api/handler/expectedswitch.goapi/pkg/api/handler/expectedswitch_test.goapi/pkg/api/model/expectedmachine.goapi/pkg/api/model/expectedmachine_test.goapi/pkg/api/model/expectedpowershelf.goapi/pkg/api/model/expectedpowershelf_test.goapi/pkg/api/model/expectedswitch.goapi/pkg/api/model/expectedswitch_test.godb/pkg/db/model/expectedmachine.godb/pkg/db/model/expectedmachine_test.godb/pkg/db/model/expectedpowershelf.godb/pkg/db/model/expectedpowershelf_test.godb/pkg/db/model/expectedswitch.godb/pkg/db/model/expectedswitch_test.godb/pkg/migrations/20260430070805_expected_components_bmc_ip_address.godocs/index.htmlopenapi/spec.yamlsdk/standard/model_expected_machine.gosdk/standard/model_expected_machine_create_request.gosdk/standard/model_expected_machine_update_request.gosdk/standard/model_expected_power_shelf.gosdk/standard/model_expected_power_shelf_create_request.gosdk/standard/model_expected_power_shelf_update_request.gosdk/standard/model_expected_switch.gosdk/standard/model_expected_switch_create_request.gosdk/standard/model_expected_switch_update_request.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.go
✅ Files skipped from review due to trivial changes (6)
- api/pkg/api/handler/expectedpowershelf_test.go
- api/pkg/api/model/expectedmachine_test.go
- sdk/standard/model_expected_machine_create_request.go
- api/pkg/api/model/expectedswitch.go
- db/pkg/db/model/expectedmachine.go
- db/pkg/db/model/expectedpowershelf.go
🚧 Files skipped from review as they are similar to previous changes (6)
- workflow/pkg/activity/expectedpowershelf/expectedpowershelf.go
- sdk/standard/model_expected_power_shelf_update_request.go
- db/pkg/migrations/20260430070805_expected_components_bmc_ip_address.go
- db/pkg/db/model/expectedswitch.go
- db/pkg/db/model/expectedpowershelf_test.go
- openapi/spec.yaml
chet
force-pushed
the
expected-bmc-ip-address
branch
2 times, most recently
from
af8fbc2 to
5a9dde0
Compare
Contributor
Author
@thossain-nv Yeah -- it's all new, so we should/can get it in sync now before LL and others start integrating with it more widely for production purposes. I'll update the PR description to mention it though -- it's a good call out. |
chet
force-pushed
the
expected-bmc-ip-address
branch
2 times, most recently
from
d7a96e8 to
66b8842
Compare
thossain-nv
approved these changes
May 4, 2026
Contributor
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
This seems ready to go, thank you @chet
All expected components in "Core" (machine, switch, and power shelf) now all support an optional `bmc_ip_address` that you can set, which allows a caller to specify a pre-defined BMC IP, whether it be asking for a static allocation within a managed network, or instructing Core that the IP is an externally-managed IP in an "external" network. This simply exposes the ability to set + plumb through that value to Core. Tests added, including IP validation and round trip tests. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
expected-bmc-ip-address
branch
from
66b8842 to
a77644f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This supports #434.
All expected components in "Core" (machine, switch, and power shelf) now all support an optional
bmc_ip_addressthat you can set, which allows a caller to specify a pre-defined BMC IP, whether it be asking for a static allocation within a managed network, or instructing Core that the IP is an externally-managed IP in an "external" network. This simply exposes the ability to set + plumb through that value to Core.This also renames the
PowerShelf.ip_addresstoPowerShelf.bmc_ip_addressto match the plumbing AND keep things consistent across all components. Sometimes we refer to the PowerShelf BMC as the "PMC", but when it comes down to expected-* components and such, they are all abmc_ip_address.Tests added, including IP validation and round trip tests.
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes
None