chore: Add FromProto receivers for ExpectedMachine, PowerShelf, Switch, Rack#500
Merged
chet merged 2 commits intoNVIDIA:mainfrom May 7, 2026
Merged
Conversation
Contributor
WalkthroughAdds a proto-conversion guidance section, centralizes metadata label extraction, implements FromProto on ExpectedMachine/ExpectedPowerShelf/ExpectedRack/ExpectedSwitch, and refactors workflow activities/tests to use those FromProto methods instead of manual field mapping. ChangesProto Conversion Standardization and Activity Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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-05-07 05:49:17 UTC | Commit: 5261ebe |
chet
changed the title
![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: 3
🧹 Nitpick comments (1)
db/pkg/db/model/common_test.go (1)
104-113: 💤 Low value
assert.Equalhandles typed-nil maps — the explicit branch is redundant.
assert.Equal(t, tc.want, got)handlesnilvsmap[string]string(nil)correctly; theif tc.want == nilguard can be removed.♻️ Simplification
- if tc.want == nil { - assert.Nil(t, got) - } else { - assert.Equal(t, tc.want, got) - } + assert.Equal(t, tc.want, got)🤖 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 `@db/pkg/db/model/common_test.go` around lines 104 - 113, The test's explicit nil-check branch is redundant because assert.Equal handles typed nil maps; simplify the loop in the test by removing the if tc.want == nil branch and always call assert.Equal(t, tc.want, got) inside the t.Run closure so LabelsFromProtoMetadata behavior is still verified without special-casing nil; update the table-driven test in common_test.go accordingly, referencing the test loop that calls LabelsFromProtoMetadata and uses assert.Equal.
🤖 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 `@db/pkg/db/model/expectedmachine.go`:
- Around line 185-188: The FromProto conversion in ExpectedMachine (method
ExpectedMachine.FromProto) fails to clear em.RackID when proto.RackId is nil,
leading to stale RackID values; change the logic so that if proto.RackId != nil
you set em.RackID = &proto.RackId.Id, otherwise explicitly set em.RackID = nil,
and add a unit test mirroring the fix in expectedswitch.go that verifies RackID
is cleared when proto.RackId is nil (use the same test pattern and assertions as
ExpectedSwitch.FromProto's test).
In `@db/pkg/db/model/expectedpowershelf.go`:
- Around line 176-179: The FromProto implementation for ExpectedPowerShelf
leaves eps.RackID unchanged when proto.RackId is nil, causing old values to
persist; update ExpectedPowerShelf.FromProto so that if proto.RackId is nil it
explicitly sets eps.RackID = nil, and if non-nil sets eps.RackID to a pointer to
proto.RackId.Id (mirroring the pattern used in ExpectedSwitch.FromProto and
ExpectedMachine.FromProto) to ensure the field is correctly cleared or assigned
based on the proto.
In `@db/pkg/db/model/expectedswitch.go`:
- Around line 183-186: The FromProto method for ExpectedSwitch currently only
assigns es.RackID when proto.RackId != nil, leaving old values intact; update
FromProto so that when proto.RackId is nil it explicitly sets es.RackID = nil
(mirror how other optional fields are handled), i.e., in the code handling
proto.RackId ensure an else branch that clears es.RackID; also add the suggested
test ("nil RackId clears a previously-set RackID") to verify a pre-populated
ExpectedSwitch with RackID gets cleared after FromProto is called with a nil
RackId.
---
Nitpick comments:
In `@db/pkg/db/model/common_test.go`:
- Around line 104-113: The test's explicit nil-check branch is redundant because
assert.Equal handles typed nil maps; simplify the loop in the test by removing
the if tc.want == nil branch and always call assert.Equal(t, tc.want, got)
inside the t.Run closure so LabelsFromProtoMetadata behavior is still verified
without special-casing nil; update the table-driven test in common_test.go
accordingly, referencing the test loop that calls LabelsFromProtoMetadata and
uses assert.Equal.
🪄 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: e360d92b-6bae-464f-bddb-0de91b8ba048
📒 Files selected for processing (15)
AGENTS.mddb/pkg/db/model/common.godb/pkg/db/model/common_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.goworkflow/pkg/activity/expectedmachine/expectedmachine.goworkflow/pkg/activity/expectedmachine/expectedmachine_test.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf.goworkflow/pkg/activity/expectedpowershelf/expectedpowershelf_test.goworkflow/pkg/activity/expectedswitch/expectedswitch.goworkflow/pkg/activity/expectedswitch/expectedswitch_test.go
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
chet
changed the title
…h, Rack Mirrors the existing `ToProto` receivers on the aforementioned DB models so that the inventory-reconciliation activities (across workflow/pkg/activity/expected*) no longer build proto-derived field maps inline. Each `FromProto`: - Treats a nil proto as a no-op. - Copies every persisted field that has a proto counterpart, so the receiver and `ToProto` stay symmetric. - Leaves the receiver's existing ID untouched on missing/invalid proto.Id (callers pre-validate the UUID before calling). - Accepts non-persisted side inputs as additional args (e.g. `linkedMachineID` for `ExpectedMachine`, since BMC-MAC to Machine resolution lives in the activity). The duplicated `getLabelsMapFromProto` helper that lived in three sites is replaced by a new `LabelsFromProtoMetadata` in `db/pkg/db/model`, used by `FromProto` (and in tests). In this function, empty/nil distinction is preserved: nil metadata or nil labels yield nil map; empty labels yield an empty map (not nil), so callers can still distinguish "no labels reported" from "labels explicitly cleared". I'm also dropping in an update to `AGENTS.md` that talks about "*Proto conversion methods*", so agent-assisted code changes going into the REST API adhere/support the modeling here: `ToProto`/`FromProto` receiver methods on DB and API models, plus the carve-outs for entities that map to multiple proto request types (e.g. `Tenant`) or produce reusable sub-messages (e.g. `RackFilter.ToTargetSpec` and `APIRackGetAllRequest.ToFilters`). Tests added! Signed-off-by: Chet Nichols III <chetn@nvidia.com>
chet
force-pushed
the
proto-expected-fromproto-receivers
branch
from
5261ebe to
f2e884f
Compare
chet
changed the title
21 tasks
thossain-nv
approved these changes
May 7, 2026
Contributor
thossain-nv
left a comment
thossain-nv
left a comment
There was a problem hiding this comment.
This looks good, thanks @chet
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
Mirrors the existing
ToProtoreceivers on the aforementioned DB models so that the inventory-reconciliation activities (across workflow/pkg/activity/expected*) no longer build proto-derived field maps inline. EachFromProto:ToProtostay symmetric.linkedMachineIDforExpectedMachine, since BMC-MAC to Machine resolution lives in the activity).The duplicated
getLabelsMapFromProtohelper that lived in three sites is replaced by a newLabelsFromProtoMetadataindb/pkg/db/model, used byFromProto(and in tests). In this function, empty/nil distinction is preserved: nil metadata or nil labels yield nil map; empty labels yield an empty map (not nil), so callers can still distinguish "no labels reported" from "labels explicitly cleared".I'm also dropping in an update to
AGENTS.mdthat talks about "Proto conversion methods", so agent-assisted code changes going into the REST API adhere/support the modeling here:ToProto/FromProtoreceiver methods on DB and API models, plus the carve-outs for entities that map to multiple proto request types (e.g.Tenant) or produce reusable sub-messages (e.g.RackFilter.ToTargetSpecandAPIRackGetAllRequest.ToFilters).Tests added!
Signed-off-by: Chet Nichols III chetn@nvidia.com
Type of Change
Services Affected
Related Issues (Optional)
Breaking Changes
Testing
Additional Notes