fix(rest-api): allow clearing an ExpectedMachine BMC IP address#3678
Conversation
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3678.docs.buildwithfern.com/infra-controller |
Summary by CodeRabbit
WalkthroughExplicit ChangesExpectedMachine BMC IP semantics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant APIClient
participant UpdateExpectedMachineHandler
participant ExpectedMachineDAO
participant TemporalWorkflow
APIClient->>UpdateExpectedMachineHandler: PATCH bmcIpAddress null
UpdateExpectedMachineHandler->>ExpectedMachineDAO: Clear BmcIpAddress
ExpectedMachineDAO-->>UpdateExpectedMachineHandler: Return cleared ExpectedMachine
UpdateExpectedMachineHandler->>TemporalWorkflow: Execute update workflow
TemporalWorkflow-->>APIClient: Return updated ExpectedMachine
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)rest-api/docs/index.htmlast-grep timed out on this file 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-07-17 18:20:20 UTC | Commit: 59680ce |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@rest-api/openapi/spec.yaml`:
- Around line 21510-21512: Add an allowlist entry to
oasdiff-breaking-changes-ignore.txt for the
ExpectedMachineUpdateRequest.bmcIpAddress minLength change, covering both PATCH
endpoint usages. Keep the existing OpenAPI validation unchanged so empty strings
remain rejected.
🪄 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: c1392df9-4ecf-4001-bcb6-eef3d0216e13
⛔ Files ignored due to path filters (1)
rest-api/sdk/standard/model_expected_machine_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (8)
rest-api/api/pkg/api/handler/expectedmachine.gorest-api/api/pkg/api/handler/expectedmachine_test.gorest-api/api/pkg/api/model/expectedmachine.gorest-api/api/pkg/api/model/expectedmachine_test.gorest-api/db/pkg/db/model/expectedmachine.gorest-api/db/pkg/db/model/expectedmachine_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yaml
`ExpectedMachine` PATCH requests can now distinguish an explicit `bmcIpAddress: null` from an omitted field. Explicit null clears the address, while omission continues through the existing update behavior for individual and batch requests. Primary callouts are: - `APIExpectedMachineUpdateRequest` now preserves omitted versus explicit `null`, and empty strings remain invalid. - The REST update handlers route explicit null through the existing `ExpectedMachineDAO.Clear` path; non-null values still use the established update path. - The OpenAPI contract and generated SDK/docs document the set/omit/clear behavior. Tests added! This supports NVIDIA#3630 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/api/pkg/api/handler/expectedmachine.go (1)
1543-1558: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject batches that mix BMC-IP omission with explicit presence.
Line 1556 records clears per item but never verifies that every item has the same
bmcIpAddresspresence. Consequently, a batch combiningnullwith omission succeeds, violating the published batch contract. ValidateHasBmcIpAddress()consistently across all items before any writes and add a regression test for the mixed-presence payload.Proposed validation
bmcIPClearIDs := make([]uuid.UUID, 0) - for _, machineReq := range apiRequests { + hasBmcIPAddress := apiRequests[0].HasBmcIpAddress() + for i, machineReq := range apiRequests { + if machineReq.HasBmcIpAddress() != hasBmcIPAddress { + return cutil.NewAPIErrorResponse( + c, + http.StatusBadRequest, + fmt.Sprintf("bmcIpAddress field presence differs at batch index %d", i), + nil, + ) + }As per path instructions, “every item in a batch update must provide the same set of fields.” <path_instructions>
🤖 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 `@rest-api/api/pkg/api/handler/expectedmachine.go` around lines 1543 - 1558, Update the batch validation around the Expected Machine update loop to require consistent HasBmcIpAddress() presence across every request, rejecting any batch that mixes explicit null with omission before modifying credsByID or recording clears. Preserve the existing nil-ID validation and add a regression test covering a mixed-presence bmcIpAddress payload.Source: Path instructions
🤖 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.
Outside diff comments:
In `@rest-api/api/pkg/api/handler/expectedmachine.go`:
- Around line 1543-1558: Update the batch validation around the Expected Machine
update loop to require consistent HasBmcIpAddress() presence across every
request, rejecting any batch that mixes explicit null with omission before
modifying credsByID or recording clears. Preserve the existing nil-ID validation
and add a regression test covering a mixed-presence bmcIpAddress payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7fd36417-8e7d-4570-9926-7e063c7f9f6f
⛔ Files ignored due to path filters (2)
rest-api/sdk/standard/api_expected_machine.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/model_expected_machine_update_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (7)
rest-api/api/pkg/api/handler/expectedmachine.gorest-api/api/pkg/api/handler/expectedmachine_test.gorest-api/api/pkg/api/model/expectedmachine.gorest-api/api/pkg/api/model/expectedmachine_test.gorest-api/db/pkg/db/model/expectedmachine_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yaml
2 participants
ExpectedMachinePATCH requests can now distinguish an explicitbmcIpAddress: nullfrom an omitted field. Explicit null clears the address, while omission preserves it for an individual update. Batch requests keep the established contract that every item provides the same field set.The request model now tracks presence separately from value, and both REST update handlers route explicit null through the existing
ExpectedMachineDAO.Clearpath. Non-null values and genericUpdateMultiplebehavior are unchanged. The OpenAPI contract and generated SDK/docs describe the set, omit, and clear behavior.Related issues
This supports #3630
Type of Change
Breaking Changes
Testing
Additional Notes
cargo make format-nightlycargo make clippycargo make carbide-lintsgo test -p 1 ./api/...go test -p 1 ./db/...golangci-lintv2.12.2go veton changed API/DB packages