fix(rest-api): allow clearing an ExpectedMachine BMC IP address by chet · Pull Request #3678 · NVIDIA/infra-controller

chet · 2026-07-17T18:17:00Z

ExpectedMachine PATCH requests can now clear bmcIpAddress with an explicit null; omitting it still preserves the configured address. This works for individual and batch updates and forwards the cleared state through the existing Core workflow.

The request model now keeps presence separate from value, and mixed batch updates are grouped by exact field set so one row cannot clear fields another row omitted. The OpenAPI contract and generated artifacts document the set/omit/clear behavior.

Related issues

This supports #3630

Type of Change

Add - New feature or capability
Change - Changes in existing functionality
Fix - Bug fixes
Remove - Removed features or deprecated functionality
Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

This PR contains breaking changes

Testing

Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)

Additional Notes

cargo make format-nightly
cargo make clippy
cargo make carbide-lints
go test -p 1 ./api/...
go test -p 1 ./db/...
Changed-line golangci-lint
OpenAPI lint and generated SDK tests

chet · 2026-07-17T18:17:42Z

@coderabbitai full_review, thanks!

coderabbitai · 2026-07-17T18:18:07Z

@chet Understood — I’ll perform a fresh, comprehensive review of #3678, including the request-presence semantics, batch grouping behavior, Core workflow propagation, OpenAPI/generated artifacts, and test coverage.

🐇🔍

✅ Action performed

Full review finished.

github-actions · 2026-07-17T18:18:28Z

🌿 Preview your docs: https://nvidia-preview-pull-request-3678.docs.buildwithfern.com/infra-controller

coderabbitai · 2026-07-17T18:19:41Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 121d28db-1583-4658-a63b-9f7774198cc1

📥 Commits

Reviewing files that changed from the base of the PR and between 0cd8acb and cc61718.

⛔ Files ignored due to path filters (1)

rest-api/sdk/standard/model_expected_machine_update_request.go is excluded by !rest-api/sdk/standard/model_*.go

📒 Files selected for processing (8)

rest-api/api/pkg/api/handler/expectedmachine.go
rest-api/api/pkg/api/handler/expectedmachine_test.go
rest-api/api/pkg/api/model/expectedmachine.go
rest-api/api/pkg/api/model/expectedmachine_test.go
rest-api/db/pkg/db/model/expectedmachine.go
rest-api/db/pkg/db/model/expectedmachine_test.go
rest-api/docs/index.html
rest-api/openapi/spec.yaml

Summary by CodeRabbit

New Features
- Expected Machine updates now support explicitly clearing the BMC IP address by setting it to null.
- Omitting the BMC IP preserves its current value.
- Batch updates support mixed BMC IP actions per machine.
Bug Fixes
- Prevented bulk updates from unintentionally clearing BMC IP addresses on unaffected machines.
Documentation
- Clarified BMC IP update behavior, including valid values, omission, null, and invalid empty strings.

Walkthrough

ExpectedMachine PATCH handling now distinguishes omitted, non-empty, empty, and explicit null BMC IP values. Explicit nulls clear persisted addresses in single and batch updates, while grouped DAO updates preserve fields omitted from mixed batches. Tests and OpenAPI documentation cover the behavior.

Changes

ExpectedMachine BMC IP semantics

Layer / File(s)	Summary
Track BMC IP field presence `rest-api/api/pkg/api/model/expectedmachine.go`, `rest-api/api/pkg/api/model/expectedmachine_test.go`, `rest-api/openapi/spec.yaml`	Custom JSON decoding records whether `bmcIpAddress` was supplied, supports explicit `null`, validates non-empty strings, and documents omitted, null, and invalid empty-string behavior.
Preserve fields in mixed batch updates `rest-api/db/pkg/db/model/expectedmachine.go`, `rest-api/db/pkg/db/model/expectedmachine_test.go`	`UpdateMultiple` groups inputs by their provided columns and executes deterministic bulk updates, preserving omitted BMC IP values and retaining host lifecycle profile handling.
Clear explicit null values `rest-api/api/pkg/api/handler/expectedmachine.go`, `rest-api/api/pkg/api/handler/expectedmachine_test.go`	Single and batch handlers clear explicitly null BMC IP values through the DAO before workflow processing, with coverage for responses and persisted state.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant APIClient
  participant UpdateExpectedMachineHandler
  participant ExpectedMachineSQLDAO
  participant UpdateExpectedMachineWorkflow
  APIClient->>UpdateExpectedMachineHandler: PATCH bmcIpAddress null
  UpdateExpectedMachineHandler->>ExpectedMachineSQLDAO: Clear BmcIpAddress
  ExpectedMachineSQLDAO-->>UpdateExpectedMachineHandler: Return cleared record
  UpdateExpectedMachineHandler->>UpdateExpectedMachineWorkflow: Execute update workflow
  UpdateExpectedMachineWorkflow-->>APIClient: Return updated ExpectedMachine

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately captures the main change: allowing BMC IP clearing on ExpectedMachine updates.
Description check	✅ Passed	The description is directly related to the patch semantics, batch handling, workflow propagation, and updated contract/tests.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.html

ast-grep timed out on this file

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-07-17T18:20:21Z

🔐 TruffleHog Secret Scan

✅ No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

_{🕐 Last updated: 2026-07-17 18:20:20 UTC | Commit: 59680ce}

`ExpectedMachine` PATCH requests can now clear `bmcIpAddress` with an explicit `null`, while omission still leaves the existing address alone. Individual and batch updates both send the resulting value through the existing Core workflow. Primary callouts are: - `APIExpectedMachineUpdateRequest` now preserves omitted versus explicit `null`, and empty strings remain invalid. - `ExpectedMachineDAO.UpdateMultiple` groups sparse batch updates by field set so one row can't clear fields another row omitted. - 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>

chet · 2026-07-17T18:22:29Z

@coderabbitai full_review, thanks!

coderabbitai · 2026-07-17T18:22:53Z

@chet Understood. I’ll perform a fresh, comprehensive review of #3678, with particular attention to PATCH presence semantics, mixed batch-update grouping, Core propagation, generated OpenAPI artifacts, and coverage of set/omit/clear behavior.

🐇🔍

✅ Action performed

Full review finished.

coderabbitai

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

📥 Commits

Reviewing files that changed from the base of the PR and between 0cd8acb and 59680ce.

⛔ Files ignored due to path filters (1)

rest-api/sdk/standard/model_expected_machine_update_request.go is excluded by !rest-api/sdk/standard/model_*.go

📒 Files selected for processing (8)

rest-api/api/pkg/api/handler/expectedmachine.go
rest-api/api/pkg/api/handler/expectedmachine_test.go
rest-api/api/pkg/api/model/expectedmachine.go
rest-api/api/pkg/api/model/expectedmachine_test.go
rest-api/db/pkg/db/model/expectedmachine.go
rest-api/db/pkg/db/model/expectedmachine_test.go
rest-api/docs/index.html
rest-api/openapi/spec.yaml

chet requested a review from a team as a code owner July 17, 2026 18:17

chet force-pushed the gh-issue-3630 branch from 59680ce to cc61718 Compare July 17, 2026 18:22

coderabbitai Bot reviewed Jul 17, 2026

View reviewed changes

Comment thread rest-api/openapi/spec.yaml Outdated

coderabbitai Bot mentioned this pull request Jul 17, 2026

fix(rest-api): keep ExpectedMachine BMC IPs synchronized #3701

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(rest-api): allow clearing an ExpectedMachine BMC IP address#3678

fix(rest-api): allow clearing an ExpectedMachine BMC IP address#3678
chet wants to merge 1 commit into
NVIDIA:mainfrom
chet:gh-issue-3630

chet commented Jul 17, 2026

Uh oh!

chet commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

chet commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

chet commented Jul 17, 2026

Related issues

Type of Change

Breaking Changes

Testing

Additional Notes

Uh oh!

chet commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Jul 17, 2026

🔐 TruffleHog Secret Scan

Uh oh!

chet commented Jul 17, 2026

Uh oh!

coderabbitai Bot commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading

coderabbitai Bot commented Jul 17, 2026 •

edited

Loading