Make a first-class DeviceId type - #4381
Conversation
BmcCredentialRotationRequest has a use case where we pass one of a few different Device ID types that are parseable (MachineId, SwitchId, PowerShelfId)... make this a first-class carbide_uuid type instead of a type embedded in that message. It allows cleaning up some of the admin-cli code, using a DeviceId directly instead of explicit --machine-id/--switch-id flags. The PR NVIDIA#4344 actually broke the RPC wire format for BmcCredentialRotationRequest, so this also puts that back: leave MachineId its own field and mark it deprecated, and put the field numbering back too. 4344 was also the one to add the --switch-id flag to the admin-cli, so removing it shouldn't be a problem (it was merged hours ago.)
|
@spydaNVIDIA (This was originally PR feedback on #4344 but it merged before I could request changes.) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe PR introduces shared ChangesBMC device targeting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdminCLI
participant BmcCredentialRotationHandler
participant TargetResolver
participant MachineOrSwitchLookup
AdminCLI->>BmcCredentialRotationHandler: Submit DeviceId and/or bmc_mac
BmcCredentialRotationHandler->>TargetResolver: Resolve and validate identifiers
TargetResolver->>MachineOrSwitchLookup: Resolve BMC MAC owner
MachineOrSwitchLookup-->>TargetResolver: Machine or switch DeviceId
TargetResolver-->>BmcCredentialRotationHandler: Validated target
BmcCredentialRotationHandler-->>AdminCLI: Record rotation request
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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-07-30 17:18:43 UTC | Commit: 4373b10 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/api-core/src/handlers/bmc_credential_rotation.rs (1)
43-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated "power shelf ... not yet supported" message into a shared helper/constant.
The exact string
"power shelf BMC credential rotation is not yet supported"is duplicated four times across this file (reject_unsupported_device_id,resolve_target, and bothMode::Set/Mode::Cleararms intrigger_bmc_credential_rotation). A future wording tweak risks silently drifting between call sites. Consider a singlefn power_shelf_unsupported_error() -> CarbideError(or aconstmessage) reused everywhere. The.expect("a mac target implies a parsed mac")literal inresolve_targetis similarly duplicated four times and could share the same treatment.Note also that, because
reject_unsupported_device_idalready filters outDeviceId::PowerShelfbeforeresolve_targetis ever invoked (andresolve_mac_owneronly ever returnsMachine/Switch), theDeviceId::PowerShelf(_)arms insidetrigger_bmc_credential_rotation'sMode::Set/Mode::Clearmatches are currently unreachable in practice — they only exist to satisfy exhaustiveness. Worth a comment noting this invariant so a future refactor doesn't assume they're live paths.♻️ Suggested extraction
+fn power_shelf_unsupported() -> CarbideError { + CarbideError::InvalidArgument( + "power shelf BMC credential rotation is not yet supported".to_string(), + ) +} + fn reject_unsupported_device_id( device_id: Option<DeviceId>, ) -> Result<Option<DeviceId>, CarbideError> { match device_id { - Some(DeviceId::PowerShelf(_)) => Err(CarbideError::InvalidArgument( - "power shelf BMC credential rotation is not yet supported".to_string(), - )), + Some(DeviceId::PowerShelf(_)) => Err(power_shelf_unsupported()), device_id => Ok(device_id), } }Also applies to: 113-122, 152-199
🤖 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 `@crates/api-core/src/handlers/bmc_credential_rotation.rs` around lines 43 - 78, Extract the duplicated power-shelf unsupported error text and the repeated parsed-MAC expectation message into shared helpers or constants, then reuse them in reject_unsupported_device_id, resolve_target, and both Mode::Set/Mode::Clear branches of trigger_bmc_credential_rotation. Add a comment to the PowerShelf arms in trigger_bmc_credential_rotation noting they are unreachable because reject_unsupported_device_id filters that variant and resolve_mac_owner returns only Machine or Switch.
🤖 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 `@crates/uuid/src/device/mod.rs`:
- Around line 266-275: Update the error messages in the DeviceIdParseError enum
to use lowercase phrases, changing “Invalid” and “Unable” in the Machine,
Switch, PowerShelf, and UnknownId variants while preserving their existing
wording and interpolation.
- Around line 149-154: Update the DeviceId conversion/parsing logic around the
Default implementation so a present but empty device_id message is rejected with
an error instead of falling back to MachineId::default(). Preserve valid device
ID variants, and add a regression test covering device_id {}.
---
Nitpick comments:
In `@crates/api-core/src/handlers/bmc_credential_rotation.rs`:
- Around line 43-78: Extract the duplicated power-shelf unsupported error text
and the repeated parsed-MAC expectation message into shared helpers or
constants, then reuse them in reject_unsupported_device_id, resolve_target, and
both Mode::Set/Mode::Clear branches of trigger_bmc_credential_rotation. Add a
comment to the PowerShelf arms in trigger_bmc_credential_rotation noting they
are unreachable because reject_unsupported_device_id filters that variant and
resolve_mac_owner returns only Machine or Switch.
🪄 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: 4608b157-bb10-4417-af4a-8e5f260fce0d
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/common_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (14)
crates/admin-cli/src/credential/force_bmc/args.rscrates/admin-cli/src/credential/force_bmc/cmd.rscrates/api-core/src/handlers/bmc_credential_rotation.rscrates/rpc/build.rscrates/rpc/proto/common.protocrates/rpc/proto/forge.protocrates/rpc/src/lib.rscrates/uuid/src/device/mod.rscrates/uuid/src/lib.rscrates/uuid/src/machine/mod.rscrates/uuid/src/power_shelf/mod.rscrates/uuid/src/switch/mod.rsrest-api/proto/core/src/v1/common_nico.protorest-api/proto/core/src/v1/nico_nico.proto
Since BmcCredentialRotationRequest has not landed in a release yet (it won't be a part of 2.0, and 2.1 hasn't branched yet) we don't need to consider it "supported", so we should be ok breaking it.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/handlers/bmc_credential_rotation.rs (1)
208-235: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the target-resolution matrix.
The remaining tests cover only
reject_unsupported_device_id; they do not exerciseresolve_targetorresolve_mac_owner. Add table-driven cases for device-only, MAC-only, matching and mismatched selectors, unknown MACs, missing selectors, and legacy-selector translation.As per coding guidelines, Rust input/output and error variants should use table-driven cases; as per path instructions, prioritize missing tests for changed behavior.
🤖 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 `@crates/api-core/src/handlers/bmc_credential_rotation.rs` around lines 208 - 235, Extend the test module with table-driven coverage for resolve_target and resolve_mac_owner, covering device-only, MAC-only, matching and mismatched selectors, unknown MACs, missing selectors, and legacy-selector translation. Reuse the existing MachineId, SwitchId, PowerShelfId, and scenario-style test setup where applicable, and assert each expected resolved owner or specific error variant.Sources: Coding guidelines, 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.
Nitpick comments:
In `@crates/api-core/src/handlers/bmc_credential_rotation.rs`:
- Around line 208-235: Extend the test module with table-driven coverage for
resolve_target and resolve_mac_owner, covering device-only, MAC-only, matching
and mismatched selectors, unknown MACs, missing selectors, and legacy-selector
translation. Reuse the existing MachineId, SwitchId, PowerShelfId, and
scenario-style test setup where applicable, and assert each expected resolved
owner or specific error variant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 573c2a24-2fdb-42ae-8916-49d3e34f4875
⛔ Files ignored due to path filters (1)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go
📒 Files selected for processing (5)
crates/admin-cli/src/credential/force_bmc/args.rscrates/api-core/src/handlers/bmc_credential_rotation.rscrates/rpc/proto/forge.protocrates/rpc/src/lib.rsrest-api/proto/core/src/v1/nico_nico.proto
💤 Files with no reviewable changes (2)
- crates/admin-cli/src/credential/force_bmc/args.rs
- crates/rpc/src/lib.rs
BmcCredentialRotationRequest has a use case where we pass one of a few different Device ID types that are parseable (MachineId, SwitchId, PowerShelfId)... make this a first-class carbide_uuid type instead of a type embedded in that message. It allows cleaning up some of the admin-cli code, using a DeviceId directly instead of explicit --machine-id/--switch-id flags.
The PR #4344 actually broke the RPC wire format for BmcCredentialRotationRequest, but since this message is not part of a released branch yet, I've opted not to un-break it and instead have broken it a bit further by changing the field type of DeviceId.
Related issues
Type of Change
Breaking Changes
This breaks the BmcCredentialRotationRequest message format, but BmcCredentialRotationRequest is not part of the 2.0 release, and 2.1 hasn't branched yet, so we don't yet consider that message "supported". Such breakages should be considered acceptable.
Testing
Additional Notes