feat(cli): add the boot-interface command family -- show, candidates, set#3341
Draft
chet wants to merge 1 commit into
Draft
feat(cli): add the boot-interface command family -- show, candidates, set#3341chet wants to merge 1 commit into
chet wants to merge 1 commit into
Conversation
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕦ(ò_óˇ)ᕤ ✅ Action performedFull review finished. |
Contributor
Summary by CodeRabbit
WalkthroughBoot-interface management adds server-side default and predicted selection reporting, new ChangesBoot-interface management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant AdminCLI
participant ApiClient
participant MachineBootInterfaces
participant SetPrimaryInterface
Operator->>AdminCLI: boot-interface candidates MACHINE
AdminCLI->>ApiClient: get_machine_boot_interfaces
ApiClient->>MachineBootInterfaces: request report
MachineBootInterfaces-->>ApiClient: candidates and selection fields
ApiClient-->>AdminCLI: response
AdminCLI-->>Operator: rendered candidates and markers
Operator->>AdminCLI: boot-interface set MACHINE INTERFACE
AdminCLI->>ApiClient: resolve MAC or use interface UUID
AdminCLI->>SetPrimaryInterface: set_primary_interface(reboot)
SetPrimaryInterface-->>AdminCLI: update result
AdminCLI-->>Operator: command result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3341.docs.buildwithfern.com/infra-controller |
🔐 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-09 23:32:41 UTC | Commit: ef9c5a4 |
![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: 11
🧹 Nitpick comments (4)
crates/api-core/tests/integration/machine_boot_interfaces.rs (1)
237-253: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the default interface ID alongside its MAC.
This test verifies
default_boot_interface_macbut notdefault_boot_interface_id. A regression that populated the ID from the wrong row would still pass. Seed a deterministic default row with a known Redfish ID and assert both fields together.As per coding guidelines, API-contract changes should have focused tests that exercise the 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/tests/integration/machine_boot_interfaces.rs` around lines 237 - 253, Update the integration test around the default interface selection to seed a deterministic non-underlay row with a known Redfish interface ID, then assert that both default_boot_interface_mac and default_boot_interface_id match that same row. Extend the existing default_row lookup or add a focused assertion using the row’s identifying fields so a mismatched ID cannot pass.Source: Coding guidelines
crates/api-model/src/machine/mod.rs (1)
3473-3508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven test for the new selector cases.
These two
pick_default_boot_interfacescenarios should be consolidated withvalue_scenarios!orcheck_values, making future boundary cases easier to add and keeping selection coverage consistent.As per coding guidelines, prefer table-driven tests for functions that map inputs to outputs.
🤖 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-model/src/machine/mod.rs` around lines 3473 - 3508, Consolidate the two `pick_default_boot_interface` tests into a single table-driven test using the project’s `value_scenarios!` or `check_values` helper. Define each interface set and expected result as a scenario, including the primary-flag case and all-underlay case, while preserving the existing assertion that `pick_boot_interface` honors the primary flag.Source: Coding guidelines
crates/admin-cli/src/boot_interface/set/cmd.rs (1)
131-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate resolver tests into table-driven scenarios.
All four tests call the same fallible function with different inputs. Use
scenarios!with labeledYields/Failscases so duplicate, unknown, missing-ID, and unique-MAC behavior remains easy to extend.As per coding guidelines, prefer table-driven tests for fallible operations.
🤖 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/admin-cli/src/boot_interface/set/cmd.rs` around lines 131 - 192, Consolidate the four resolver tests into one table-driven test using the project’s scenarios! macro, with labeled Yields and Fails cases covering unique, unknown, duplicate, and missing-ID MAC scenarios. Preserve the existing success UUID assertion and failure-message checks, while keeping the shared response setup and resolve_mac_to_interface_id invocation easy to extend.Source: Coding guidelines
crates/admin-cli/src/boot_interface/candidates/cmd.rs (1)
347-392: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the UUID candidate handoff in tests.
Every fixture sets
interface_idtoNone, so the UUID column consumed byboot-interface setis never tested. Add a valid interface ID to an owned row and assert that it appears in the table and structured output.As per coding guidelines, focused tests should exercise the behavior changed by the contract.
🤖 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/admin-cli/src/boot_interface/candidates/cmd.rs` around lines 347 - 392, The sample_response fixture does not cover UUID candidate handoff because all machine interfaces have interface_id set to None. Update one owned interface in sample_response with a valid interface ID, then extend the focused boot-interface set tests to assert that this UUID appears in both the rendered table and structured output.Source: Coding guidelines
🤖 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/admin-cli/src/boot_interface/candidates/cmd.rs`:
- Around line 195-199: Do not route OutputFormat::Csv through
render_candidates(), since it produces a human-readable table rather than valid
CSV. Update the format match in the candidates command to either serialize the
report with a dedicated CSV renderer or explicitly reject CSV with a clear
error; keep render_candidates() limited to OutputFormat::AsciiTable.
- Around line 132-133: Replace the unresolved
model::network_segment::NetworkSegmentType reference in the eligible closure
setup with the RPC type path, using forgerpc::NetworkSegmentType::Underlay or
::rpc::NetworkSegmentType::Underlay, while preserving the existing string
comparison behavior.
In `@crates/admin-cli/src/boot_interface/set/args.rs`:
- Around line 25-40: The command help examples in the `after_long_help`
attribute lose literal shell continuation backslashes and use a non-concrete
`<MACHINE_ID>` placeholder. Update the help string using raw-string syntax or
escaped backslashes/newlines so wrapped commands render copy-pasteable `\`
continuations, and replace `<MACHINE_ID>` with the concrete UUID placeholder
shown in the other examples.
In
`@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-candidates.md`:
- Around line 33-35: Correct the grammar in the measured-boot description by
changing “This used by measured boot” to “This is used by measured boot.”
- Around line 12-13: Update the boot-interface candidates synopsis to show the
required argument for --sort-by, matching the OPTIONS documentation by rendering
it as --sort-by <SORT_BY>.
In `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-set.md`:
- Around line 34-36: Correct the grammar in the measured-boot description by
changing “This used by measured boot” to “This is used by measured boot.”
- Around line 12-14: Update the nico-admin-cli boot-interface set synopsis to
show the required argument for --sort-by, matching the documented <SORT_BY>
option format in the OPTIONS section.
In `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-show.md`:
- Around line 12-13: Update the synopsis for the boot-interface show command to
include the required <SORT_BY> argument after --sort-by, matching the OPTIONS
documentation and showing the complete invocation syntax.
- Around line 30-32: Correct the grammar in the measured-boot description by
changing “This used by measured boot” to “This is used by measured boot.”
In `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface.md`:
- Around line 23-25: Correct the grammar in the measured-boot description by
changing “This used by measured boot” to “This is used by measured boot.”
- Around line 11-12: Update the boot-interface command synopsis to show the
required value for --sort-by, matching the documented OPTIONS placeholder by
rendering it as --sort-by <SORT_BY> while preserving the existing options and
subcommands.
---
Nitpick comments:
In `@crates/admin-cli/src/boot_interface/candidates/cmd.rs`:
- Around line 347-392: The sample_response fixture does not cover UUID candidate
handoff because all machine interfaces have interface_id set to None. Update one
owned interface in sample_response with a valid interface ID, then extend the
focused boot-interface set tests to assert that this UUID appears in both the
rendered table and structured output.
In `@crates/admin-cli/src/boot_interface/set/cmd.rs`:
- Around line 131-192: Consolidate the four resolver tests into one table-driven
test using the project’s scenarios! macro, with labeled Yields and Fails cases
covering unique, unknown, duplicate, and missing-ID MAC scenarios. Preserve the
existing success UUID assertion and failure-message checks, while keeping the
shared response setup and resolve_mac_to_interface_id invocation easy to extend.
In `@crates/api-core/tests/integration/machine_boot_interfaces.rs`:
- Around line 237-253: Update the integration test around the default interface
selection to seed a deterministic non-underlay row with a known Redfish
interface ID, then assert that both default_boot_interface_mac and
default_boot_interface_id match that same row. Extend the existing default_row
lookup or add a focused assertion using the row’s identifying fields so a
mismatched ID cannot pass.
In `@crates/api-model/src/machine/mod.rs`:
- Around line 3473-3508: Consolidate the two `pick_default_boot_interface` tests
into a single table-driven test using the project’s `value_scenarios!` or
`check_values` helper. Define each interface set and expected result as a
scenario, including the primary-flag case and all-underlay case, while
preserving the existing assertion that `pick_boot_interface` honors the primary
flag.
🪄 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: 4a3116c8-709c-45a2-ae6d-c8b75ebd112b
⛔ 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 (24)
crates/admin-cli/cli_domains.yamlcrates/admin-cli/src/boot_interface/candidates/args.rscrates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/candidates/mod.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/set/mod.rscrates/admin-cli/src/boot_interface/show/args.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/boot_interface/show/mod.rscrates/admin-cli/src/cfg/cli_options.rscrates/admin-cli/src/machine/mod.rscrates/admin-cli/src/main.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-model/src/machine/mod.rscrates/rpc/proto/forge.protodocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-candidates.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-set.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-show.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface.mddocs/manuals/nico-admin-cli/hardware.mdrest-api/proto/core/src/v1/nico_nico.proto
Comment on lines
+195
to
+199
| // CSV is a poor fit for a table-plus-summary report; AsciiTable is | ||
| // the human-readable form. Both render the same output. | ||
| OutputFormat::AsciiTable | OutputFormat::Csv => { | ||
| print!("{}", render_candidates(&report)); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cmd.rs around target =="
nl -ba crates/admin-cli/src/boot_interface/candidates/cmd.rs | sed -n '140,240p'
echo
echo "== search OutputFormat and render_candidates =="
rg -n "enum OutputFormat|render_candidates\\(|OutputFormat::Csv|Csv\\s*\\|" crates/admin-cli/src/boot_interface -SRepository: NVIDIA/infra-controller
Length of output: 228
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== cmd.rs around target =="
sed -n '140,240p' crates/admin-cli/src/boot_interface/candidates/cmd.rs | cat -n
echo
echo "== search OutputFormat and render_candidates =="
rg -n "enum OutputFormat|render_candidates\\(|OutputFormat::Csv|Csv\\s*\\|" crates/admin-cli/src/boot_interface -SRepository: NVIDIA/infra-controller
Length of output: 5646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OutputFormat definition and parser =="
rg -n "enum OutputFormat|impl.*OutputFormat|ValueEnum|csv" crates/admin-cli/src -S
echo
echo "== nearby command using the same branch =="
sed -n '120,190p' crates/admin-cli/src/boot_interface/show/cmd.rs | cat -nRepository: NVIDIA/infra-controller
Length of output: 14781
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OutputFormat definition =="
rg -n "pub enum OutputFormat|enum OutputFormat|OutputFormat" crates/admin-cli/src -S
echo
echo "== candidates cmd top =="
sed -n '1,120p' crates/admin-cli/src/boot_interface/candidates/cmd.rs | cat -nRepository: NVIDIA/infra-controller
Length of output: 50379
Do not route --format csv through the prettytable renderer.
crates/admin-cli/src/boot_interface/candidates/cmd.rs:197-199 currently sends Csv to render_candidates(), which emits a human-readable table plus summary prose, not valid CSV. Either add a CSV serializer for this report or reject csv explicitly.
🤖 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/admin-cli/src/boot_interface/candidates/cmd.rs` around lines 195 -
199, Do not route OutputFormat::Csv through render_candidates(), since it
produces a human-readable table rather than valid CSV. Update the format match
in the candidates command to either serialize the report with a dedicated CSV
renderer or explicitly reject CSV with a clear error; keep render_candidates()
limited to OutputFormat::AsciiTable.
Contributor
Author
There was a problem hiding this comment.
Deliberate, and matches the established pattern -- boot-interface show (and its predecessor machine boot-interfaces) renders the same way for csv: a multi-section table-plus-summary report is a poor fit for CSV, so AsciiTable and Csv share the human-readable form (there's a comment at the match site). Happy to revisit CSV for the whole family separately. Thanks for checking!
Contributor
There was a problem hiding this comment.
@chet Understood — thanks for pointing to the established convention. Since boot-interface show and its predecessor already treat AsciiTable/Csv identically for this multi-section report, keeping candidates consistent makes sense, and the in-code comment documents the rationale. Withdrawing this one; happy to revisit CSV support for the whole family if it comes up later.
(๑˃̵ᴗ˂̵)و
✏️ Learnings added
Learnt from: chet
Repo: NVIDIA/infra-controller PR: 3341
File: crates/admin-cli/src/boot_interface/candidates/cmd.rs:195-199
Timestamp: 2026-07-10T00:59:27.582Z
Learning: In `crates/admin-cli/src/boot_interface/**` (e.g., `candidates/cmd.rs`, `show/cmd.rs`), `OutputFormat::AsciiTable` and `OutputFormat::Csv` are deliberately routed to the same human-readable table-plus-summary renderer, since multi-section reports are a poor fit for true CSV. This matches the pattern from the predecessor `machine boot-interfaces` command. This is a bit of established convention across the boot-interface CLI family, not a bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Comment on lines
+12
to
+13
| **nico-admin-cli boot-interface candidates** \[**--extended**\] | ||
| \[**--sort-by**\] \[**-h**\|**--help**\] \<*MACHINE*\> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show the required --sort-by value in the synopsis.
The OPTIONS section documents <SORT_BY>, but the synopsis renders only --sort-by, making the documented invocation incomplete.
🤖 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
`@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-candidates.md`
around lines 12 - 13, Update the boot-interface candidates synopsis to show the
required argument for --sort-by, matching the OPTIONS documentation by rendering
it as --sort-by <SORT_BY>.
Source: Path instructions
Comment on lines
+33
to
+35
| This used by measured boot, where basic output contains just what you | ||
| probably care about, and "extended" output also dumps out all the | ||
| internal UUIDs that are used to associate instances. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the measured-boot description grammar.
Change “This used by measured boot” to “This is used by measured boot.”
🤖 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
`@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-candidates.md`
around lines 33 - 35, Correct the grammar in the measured-boot description by
changing “This used by measured boot” to “This is used by measured boot.”
Source: Path instructions
Comment on lines
+34
to
+36
| This used by measured boot, where basic output contains just what you | ||
| probably care about, and "extended" output also dumps out all the | ||
| internal UUIDs that are used to associate instances. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the measured-boot description grammar.
Change “This used by measured boot” to “This is used by measured boot.”
🤖 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 `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-set.md`
around lines 34 - 36, Correct the grammar in the measured-boot description by
changing “This used by measured boot” to “This is used by measured boot.”
Source: Path instructions
Comment on lines
+12
to
+13
| **nico-admin-cli boot-interface show** \[**--extended**\] | ||
| \[**--sort-by**\] \[**-h**\|**--help**\] \<*MACHINE*\> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show the required --sort-by value in the synopsis.
The OPTIONS section documents <SORT_BY>, but the synopsis renders only --sort-by, making the documented invocation incomplete.
🤖 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 `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-show.md`
around lines 12 - 13, Update the synopsis for the boot-interface show command to
include the required <SORT_BY> argument after --sort-by, matching the OPTIONS
documentation and showing the complete invocation syntax.
Source: Path instructions
Comment on lines
+30
to
+32
| This used by measured boot, where basic output contains just what you | ||
| probably care about, and "extended" output also dumps out all the | ||
| internal UUIDs that are used to associate instances. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the measured-boot description grammar.
Change “This used by measured boot” to “This is used by measured boot.”
🤖 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 `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-show.md`
around lines 30 - 32, Correct the grammar in the measured-boot description by
changing “This used by measured boot” to “This is used by measured boot.”
Source: Path instructions
Comment on lines
+11
to
+12
| **nico-admin-cli boot-interface** \[**--extended**\] \[**--sort-by**\] | ||
| \[**-h**\|**--help**\] \<*subcommands*\> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show the required --sort-by value in the synopsis.
The OPTIONS section documents <SORT_BY>, but the synopsis renders only --sort-by, making the documented invocation incomplete.
🤖 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 `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface.md` around
lines 11 - 12, Update the boot-interface command synopsis to show the required
value for --sort-by, matching the documented OPTIONS placeholder by rendering it
as --sort-by <SORT_BY> while preserving the existing options and subcommands.
Source: Path instructions
Comment on lines
+23
to
+25
| This used by measured boot, where basic output contains just what you | ||
| probably care about, and "extended" output also dumps out all the | ||
| internal UUIDs that are used to associate instances. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the measured-boot description grammar.
Change “This used by measured boot” to “This is used by measured boot.”
🤖 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 `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface.md` around
lines 23 - 25, Correct the grammar in the measured-boot description by changing
“This used by measured boot” to “This is used by measured boot.”
Source: Path instructions
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
crates/api-core/tests/integration/machine_boot_interfaces.rs (2)
227-235: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the authoritative row ID, not only presence.
is_some()would still pass if the handler returned an incorrectMachineInterfaceId. Retain the seeded owned row ID and compare it directly with the corresponding response field, especially for the primary row used bySetPrimaryInterface.As per path instructions, endpoint tests should verify changed response identifiers and contracts.
🤖 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/tests/integration/machine_boot_interfaces.rs` around lines 227 - 235, Update the integration test to retain the seeded owned row’s authoritative MachineInterfaceId and assert each corresponding response field equals that exact ID, rather than only checking interface_id.is_some(). Ensure the primary machine interface response is explicitly compared against the retained ID used by SetPrimaryInterface.Source: Path instructions
237-265: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover
default_boot_interface_idin the integration test.The test verifies the selected default MAC but never verifies its paired interface ID, so a MAC/ID mismatch could regress unnoticed. Seed a captured ID on the selected default row and assert that the response ID belongs to that same row.
As per path instructions, endpoint tests should cover the complete changed response shape.
🤖 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/tests/integration/machine_boot_interfaces.rs` around lines 237 - 265, Extend the integration test setup to seed a captured interface ID on the row selected as the default, then assert report.default_boot_interface_id matches that row’s ID alongside the existing MAC assertion. Update the default-row lookup or fixtures around the machine interface setup, using the default_boot_interface_id response field to verify the MAC/ID pair belongs to the same interface.Source: Path instructions
crates/api-model/src/machine/mod.rs (1)
3473-3508: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a table-driven test for these selector cases.
These related pure input-to-output cases fit
value_scenarios!orcheck_values; a labeled table would make future boundary cases easier to add consistently.As per coding guidelines, Rust tests should prefer table-driven scenarios for mapping and validation logic.
🤖 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-model/src/machine/mod.rs` around lines 3473 - 3508, Convert the related selector tests, including pick_default_boot_interface_ignores_the_primary_flag and pick_default_boot_interface_excludes_underlay_rows, into a single labeled table-driven test using the repository’s value_scenarios! or check_values helper. Represent each interface configuration and expected selected MAC or None as a scenario, while preserving the assertion that pick_boot_interface honors the primary flag where applicable.Source: Coding guidelines
🤖 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/admin-cli/src/boot_interface/mod.rs`:
- Around line 18-23: Correct the module-level documentation for the
boot-interface command family by rewording the descriptions of the show and
candidates commands in the module doc comment. Replace the awkward phrases with
clear, grammatically correct wording while preserving the existing command
meanings and RPC references.
In `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-set.md`:
- Around line 12-14: The boot-interface set documentation advertises unsupported
--extended and --sort-by options. Update the command synopsis and associated
option descriptions in the boot-interface set documentation to list only
--reboot, -h/--help, and the required MACHINE and INTERFACE arguments, unless
the parser in set args intentionally gains those options.
---
Nitpick comments:
In `@crates/api-core/tests/integration/machine_boot_interfaces.rs`:
- Around line 227-235: Update the integration test to retain the seeded owned
row’s authoritative MachineInterfaceId and assert each corresponding response
field equals that exact ID, rather than only checking interface_id.is_some().
Ensure the primary machine interface response is explicitly compared against the
retained ID used by SetPrimaryInterface.
- Around line 237-265: Extend the integration test setup to seed a captured
interface ID on the row selected as the default, then assert
report.default_boot_interface_id matches that row’s ID alongside the existing
MAC assertion. Update the default-row lookup or fixtures around the machine
interface setup, using the default_boot_interface_id response field to verify
the MAC/ID pair belongs to the same interface.
In `@crates/api-model/src/machine/mod.rs`:
- Around line 3473-3508: Convert the related selector tests, including
pick_default_boot_interface_ignores_the_primary_flag and
pick_default_boot_interface_excludes_underlay_rows, into a single labeled
table-driven test using the repository’s value_scenarios! or check_values
helper. Represent each interface configuration and expected selected MAC or None
as a scenario, while preserving the assertion that pick_boot_interface honors
the primary flag where applicable.
🪄 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: 155a2860-b00d-4dda-a667-63bf7d30e627
⛔ 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 (24)
crates/admin-cli/cli_domains.yamlcrates/admin-cli/src/boot_interface/candidates/args.rscrates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/candidates/mod.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/set/mod.rscrates/admin-cli/src/boot_interface/show/args.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/boot_interface/show/mod.rscrates/admin-cli/src/cfg/cli_options.rscrates/admin-cli/src/machine/mod.rscrates/admin-cli/src/main.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-model/src/machine/mod.rscrates/rpc/proto/forge.protodocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-candidates.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-set.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-show.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface.mddocs/manuals/nico-admin-cli/hardware.mdrest-api/proto/core/src/v1/nico_nico.proto
chet
commented
Jul 10, 2026
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
Author
|
/ok to test ba146ac |
Contributor
Author
|
/ok to test 8079306 |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
(๑˃̵ᴗ˂̵)و ✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/admin-cli/src/boot_interface/set/args.rs (1)
75-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider table-driving the
FromStrcases.Three standalone tests exercise the same fallible
parse::<InterfaceSelector>()operation over different input strings — a clean fit forcarbide-test-support'sscenarios!/Outcome, per the repo's testing convention for fallible operations.♻️ Illustrative table-driven form
use carbide_test_support::Outcome::*; use carbide_test_support::scenarios; scenarios!(parse_interface_selector: "uuid" { "abcdef01-2345-6789-abcd-ef0123456789" => Yields(InterfaceSelector::Id("abcdef01-2345-6789-abcd-ef0123456789".parse().unwrap())), } "mac" { "00:11:22:33:44:55" => Yields(InterfaceSelector::Mac("00:11:22:33:44:55".parse().unwrap())), } "invalid" { "not-an-interface" => Fails, } );🤖 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/admin-cli/src/boot_interface/set/args.rs` around lines 75 - 99, Replace the three standalone tests in the tests module with a table-driven carbide-test-support scenarios! definition covering UUID, MAC address, and invalid selector inputs. Use Outcome variants to assert Yields with the expected InterfaceSelector values and Fails for invalid input, preserving the existing parsing coverage and importing the required scenarios and Outcome symbols.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.
Inline comments:
In `@crates/admin-cli/src/boot_interface/candidates/mod.rs`:
- Around line 27-31: Thread ctx.output_file from Run::run into
cmd::handle_candidates, update the handler signature and its callers
accordingly, and replace direct stdout writes with writes through the provided
output destination.
---
Nitpick comments:
In `@crates/admin-cli/src/boot_interface/set/args.rs`:
- Around line 75-99: Replace the three standalone tests in the tests module with
a table-driven carbide-test-support scenarios! definition covering UUID, MAC
address, and invalid selector inputs. Use Outcome variants to assert Yields with
the expected InterfaceSelector values and Fails for invalid input, preserving
the existing parsing coverage and importing the required scenarios and Outcome
symbols.
🪄 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: 714ad80b-4177-49ac-ba97-49ad07dbd43c
⛔ 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 (24)
crates/admin-cli/cli_domains.yamlcrates/admin-cli/src/boot_interface/candidates/args.rscrates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/candidates/mod.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/set/mod.rscrates/admin-cli/src/boot_interface/show/args.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/boot_interface/show/mod.rscrates/admin-cli/src/cfg/cli_options.rscrates/admin-cli/src/machine/mod.rscrates/admin-cli/src/main.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-model/src/machine/mod.rscrates/rpc/proto/forge.protodocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-candidates.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-set.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-show.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface.mddocs/manuals/nico-admin-cli/hardware.mdrest-api/proto/core/src/v1/nico_nico.proto
… set Gives operators one place to reason about a machine's boot interface. `boot-interface show` is the existing four-store view relocated from `machine boot-interfaces` (which stays parseable but hidden, running the same handler), `boot-interface candidates` is new -- one row per NIC across managed interface rows and pre-first-lease predictions, marked with the picks the system computes among them -- and `boot-interface set` fronts the same `SetPrimaryInterface` RPC as `managed-host set-primary-interface`, naming the interface by UUID or MAC. The candidates markers tell the story at a glance: `current` is what resolution targets right now, `default` is what the automatic selection would choose if no primary interface were set, and `explored` is site-explorer's stored default. Every pick is computed server-side -- `GetMachineBootInterfacesResponse` grows additive fields (`default_boot_interface_mac`/`_id`, `predicted_boot_interface_mac`/`_id`, plus the row id on each managed interface) and the CLI only matches rows against them, so selection logic lives in exactly one place. A taste of `boot-interface candidates` against a host whose operator moved the boot NIC off the automatic pick: ``` Boot-interface candidates for machine fm100ht038bg3qsho433vkg684heguv282qaggmrsh2ugn1qk096n2c6hcg +-------------------+--------------------------------------+----------------------+-------------+---------+---------+---------------+------------------+ | MAC Address | Interface UUID | Boot Interface Id | Segment | Source | Primary | Eligible | Markers | +===================+======================================+======================+=============+=========+=========+===============+==================+ | 00:11:22:33:44:11 | 0aa11b22-c333-4d44-9e55-f66667778888 | - | tor | managed | false | no (underlay) | - | +-------------------+--------------------------------------+----------------------+-------------+---------+---------+---------------+------------------+ | 00:11:22:33:44:55 | abcdef01-2345-6789-abcd-ef0123456789 | NIC.Slot.7-1-1 | host_inband | managed | false | yes | default | +-------------------+--------------------------------------+----------------------+-------------+---------+---------+---------------+------------------+ | 00:11:22:33:44:66 | bcdef012-3456-789a-bcde-f01234567890 | NIC.Integrated.1-1-1 | admin | managed | true | yes | current,explored | +-------------------+--------------------------------------+----------------------+-------------+---------+---------+---------------+------------------+ Current boot interface: 00:11:22:33:44:66 (NIC.Integrated.1-1-1) Default (auto) pick: 00:11:22:33:44:55 (NIC.Slot.7-1-1) Explored default(s): 00:11:22:33:44:66 (NIC.Integrated.1-1-1) Stores diverge on boot MAC: false ``` Primary callouts are: - `pick_boot_interface`'s fallback rung split out as `pick_default_boot_interface` in api-model, so the RPC handler reports the automatic pick with the same selection code the machine-controller acts on. - `boot-interface set <machine> <interface>` takes a machine-interface UUID or a MAC; a MAC must resolve to exactly one managed interface row -- a duplicate across segments is refused with the UUIDs listed rather than guessed at, and a machine still waiting on its first lease is pointed at the expected machine's `host_nics` `primary` declaration. - `machine boot-interfaces` is hidden and delegates to the new `show` (same `Args` type), so existing scripts keep working while the generated reference shows one canonical page. - Registration surfaces all updated: `cli_domains.yaml` (hardware), the generated CLI reference pages for the new commands, and the `rest-api` core-proto mirror (`nico_nico.proto` + Go bindings) regenerated for the additive fields. Tests added! This supports NVIDIA#3335 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
Contributor
Author
|
/ok to test 5eba717 |
Contributor
Author
|
@coderabbitai full_review, thanks! |
Contributor
|
ᕦ(ò_óˇ)ᕤ ✅ Action performedFull review finished. |
Contributor
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 `@docs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-set.md`:
- Around line 12-14: Update the boot-interface set command synopsis to render
the sort option with its required argument, changing `--sort-by` to `--sort-by
<SORT_BY>` while preserving the existing command structure and options.
🪄 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: 746288ec-0283-44c2-8681-eb3b473ea610
⛔ 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 (24)
crates/admin-cli/cli_domains.yamlcrates/admin-cli/src/boot_interface/candidates/args.rscrates/admin-cli/src/boot_interface/candidates/cmd.rscrates/admin-cli/src/boot_interface/candidates/mod.rscrates/admin-cli/src/boot_interface/mod.rscrates/admin-cli/src/boot_interface/set/args.rscrates/admin-cli/src/boot_interface/set/cmd.rscrates/admin-cli/src/boot_interface/set/mod.rscrates/admin-cli/src/boot_interface/show/args.rscrates/admin-cli/src/boot_interface/show/cmd.rscrates/admin-cli/src/boot_interface/show/mod.rscrates/admin-cli/src/cfg/cli_options.rscrates/admin-cli/src/machine/mod.rscrates/admin-cli/src/main.rscrates/api-core/src/handlers/machine_boot_interfaces.rscrates/api-core/tests/integration/machine_boot_interfaces.rscrates/api-model/src/machine/mod.rscrates/rpc/proto/forge.protodocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-candidates.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-set.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface-show.mddocs/manuals/nico-admin-cli/commands/boot-interface/boot-interface.mddocs/manuals/nico-admin-cli/hardware.mdrest-api/proto/core/src/v1/nico_nico.proto
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
1 participant
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.
Gives operators one place to reason about a machine's boot interface:
boot-interface show(the four-store view, relocated frommachine boot-interfaces, which stays as a hidden alias),boot-interface candidates(new: every NIC with the picks marked), andboot-interface set(fronts the sameSetPrimaryInterfaceRPC asmanaged-host set-primary-interface, by UUID or MAC).A taste of
boot-interface candidatesagainst a host whose operator moved the boot NIC off the automatic pick:current(what resolution targets now),default(what the automatic selection would choose if no primary interface were set), andexplored(site-explorer's stored default). All computed server-side via additiveGetMachineBootInterfacesResponsefields; the CLI never re-derives selection logic.pick_boot_interface's fallback rung split out aspick_default_boot_interfacein api-model, shared by the handler and the machine-controller.setby MAC refuses a duplicate across segments (lists the UUIDs) instead of guessing.cli_domains.yaml, the generated CLI reference pages, and therest-apicore-proto mirror + Go bindings regenerated.Tests added!
This supports #3335