feat(cli): add the boot-interface command family -- show, candidates, set#3341
feat(cli): add the boot-interface command family -- show, candidates, set#3341chet wants to merge 1 commit into
Conversation
|
@coderabbitai full_review, thanks! |
|
ᕦ(ò_óˇ)ᕤ ✅ Action performedFull review finished. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds default and predicted boot-interface reporting, managed interface identifiers, and new ChangesBoot-interface management
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant AdminCLI
participant ApiClient
participant MachineBootInterfaces
Operator->>AdminCLI: boot-interface candidates MACHINE
AdminCLI->>ApiClient: get_machine_boot_interfaces
ApiClient->>MachineBootInterfaces: request machine report
MachineBootInterfaces-->>ApiClient: candidates and selection fields
ApiClient-->>AdminCLI: response
AdminCLI-->>Operator: rendered candidates and markers
sequenceDiagram
participant Operator
participant AdminCLI
participant ApiClient
participant SetPrimaryInterface
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)
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)crates/rpc/build.rsast-grep timed out on this file 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}
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
| // 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)); | ||
| } |
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.
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!
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.
| **nico-admin-cli boot-interface candidates** \[**--extended**\] | ||
| \[**--sort-by**\] \[**-h**\|**--help**\] \<*MACHINE*\> |
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
| 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. |
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
| 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. |
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
| **nico-admin-cli boot-interface show** \[**--extended**\] | ||
| \[**--sort-by**\] \[**-h**\|**--help**\] \<*MACHINE*\> |
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
| 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. |
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
| **nico-admin-cli boot-interface** \[**--extended**\] \[**--sort-by**\] | ||
| \[**-h**\|**--help**\] \<*subcommands*\> |
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
| 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. |
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 |
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
|
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. |
|
/ok to test ba146ac |
|
/ok to test 8079306 |
|
@coderabbitai full_review, thanks! |
|
(๑˃̵ᴗ˂̵)و ✅ Action performedFull review finished. |
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
|
/ok to test 5eba717 |
|
@coderabbitai full_review, thanks! |
|
ᕦ(ò_óˇ)ᕤ ✅ Action performedFull review finished. |
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
|
/ok to test a23db92 |
|
@coderabbitai full_review, thanks! |
|
ᕦ(ò_óˇ)ᕤ ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crates/admin-cli/src/boot_interface/set/args.rs (1)
25-40: 🎯 Functional Correctness | 🟡 MinorPreserve literal shell continuations in rendered help.
The
\at the end of lines 32 and 36 is Rust’s escaped-newline syntax, so Clap receives no literal backslash and the commands are not copy-pasteable. The tip also still uses<MACHINE_ID>instead of a concrete UUID.Proposed fix
-#[command(after_long_help = "\ -EXAMPLES: +#[command(after_long_help = r#"EXAMPLES: ... -Tip: 'boot-interface candidates <MACHINE_ID>' lists the candidate NICs with their MACs and UUIDs. +Tip: 'boot-interface candidates 12345678-1234-5678-90ab-cdef01234567' lists the candidate NICs with their MACs and UUIDs. ... -")] +"#)]As per path instructions, CLI help examples must use concrete placeholders and preserve literal shell continuations.
🤖 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 25 - 40, Update the after_long_help text in the boot-interface command to escape the backslashes so Clap renders literal shell continuations in the multi-line examples. Replace the <MACHINE_ID> tip placeholder with the same concrete machine UUID format used by the examples, while preserving the existing command content and formatting.Source: Path instructions
🧹 Nitpick comments (2)
crates/admin-cli/src/boot_interface/set/args.rs (1)
75-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse table-driven coverage for selector parsing.
These tests exercise the same fallible parser with three input variants. Group them with
scenarios!orcheck_casesso valid UUID, valid MAC, and invalid input remain easy to extend consistently.As per coding guidelines, parser and conversion tests should prefer table-driven cases.
🤖 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 - 98, Consolidate the three separate selector parsing tests into a table-driven test using the repository’s established scenarios! or check_cases helper. Keep coverage for the valid UUID, valid MAC, and invalid input cases, including their current assertions, within the shared table-driven structure near InterfaceSelector parsing.Source: Coding guidelines
crates/admin-cli/src/boot_interface/candidates/cmd.rs (1)
385-431: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test coverage for a populated
interface_id.Every
MachineInterfaceBootInterfaceinsample_response()setsinterface_id: None, so no test exercises theSome(id) → id.to_string()path feeding the "Interface UUID" column (line 158, line 276). This is the exact field the PR introduces to letcandidatesoutput be piped intoboot-interface set, so it is worth a concrete assertion.✅ Suggested test addition
forgerpc::MachineInterfaceBootInterface { mac_address: "aa:bb:cc:00:00:02".to_string(), primary_interface: true, boot_interface_id: Some("NIC.Slot.2-1-1".to_string()), network_segment_type: Some("admin".to_string()), - interface_id: None, + interface_id: Some(machine_interface_id_for_test()), },Then assert
rendered.contains(&machine_interface_id_for_test().to_string())(or the equivalent JSON assertion oncandidates[0]["interface_id"]) in a new/existing test.Also applies to: 502-513
🤖 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 385 - 431, Add populated interface_id test data to sample_response(), using a known test UUID on at least one MachineInterfaceBootInterface. Update or add the relevant candidates output test to assert that this identifier appears in the rendered Interface UUID column or equivalent JSON interface_id field, covering the Some(id) conversion path.
🤖 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/args.rs`:
- Around line 22-32: Update the after_long_help string on the candidates command
to keep the EXAMPLES block limited to description/command pairs: move the Tip
text before EXAMPLES or outside the help string, and ensure exactly one trailing
blank line follows the final example command.
In `@crates/admin-cli/src/boot_interface/set/cmd.rs`:
- Around line 179-184: Update the duplicate-MAC error test around
resolve_mac_to_interface_id to assert that the error message also contains the
second matching UUID, 12345678-1234-5678-90ab-cdef01234567; alternatively,
compare the complete expected message while preserving the existing count and
first-UUID assertions.
In `@crates/admin-cli/src/boot_interface/show/args.rs`:
- Around line 28-30: Update the `boot-interface show` help text to remove the
claim that `--output` names a file to write to, while retaining the supported
global `--format` JSON/YAML description and examples.
---
Duplicate comments:
In `@crates/admin-cli/src/boot_interface/set/args.rs`:
- Around line 25-40: Update the after_long_help text in the boot-interface
command to escape the backslashes so Clap renders literal shell continuations in
the multi-line examples. Replace the <MACHINE_ID> tip placeholder with the same
concrete machine UUID format used by the examples, while preserving the existing
command content and formatting.
---
Nitpick comments:
In `@crates/admin-cli/src/boot_interface/candidates/cmd.rs`:
- Around line 385-431: Add populated interface_id test data to
sample_response(), using a known test UUID on at least one
MachineInterfaceBootInterface. Update or add the relevant candidates output test
to assert that this identifier appears in the rendered Interface UUID column or
equivalent JSON interface_id field, covering the Some(id) conversion path.
In `@crates/admin-cli/src/boot_interface/set/args.rs`:
- Around line 75-98: Consolidate the three separate selector parsing tests into
a table-driven test using the repository’s established scenarios! or check_cases
helper. Keep coverage for the valid UUID, valid MAC, and invalid input cases,
including their current assertions, within the shared table-driven structure
near InterfaceSelector parsing.
🪄 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: 851e3064-5f78-48f8-a5c8-0ccd2607c5a7
⛔ 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 (19)
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.protorest-api/proto/core/src/v1/nico_nico.proto
|
/ok to test 853b561 |
|
@coderabbitai full_review, thanks! |
|
ᕦ(ò_óˇ)ᕤ ✅ 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)
crates/rpc/proto/forge.proto (1)
9093-9116: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRename the row-id field for this boot-interface view
MachineBootInterface.interface_idalready names the RedfishEthernetInterface.Id, whileMachineInterfaceBootInterface.interface_idnow means thecommon.MachineInterfaceIdrow key. Use a distinct name here, such asmachine_interface_idorrow_id, to avoid ambiguity and keep the boot-interface messages consistent.🤖 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/rpc/proto/forge.proto` around lines 9093 - 9116, Rename the common.MachineInterfaceId field in MachineInterfaceBootInterface from interface_id to a distinct row-key name such as machine_interface_id, while leaving MachineBootInterface.interface_id unchanged as the Redfish EthernetInterface.Id. Update references to the renamed field consistently.Source: Path instructions
🧹 Nitpick comments (1)
crates/rpc/src/model/machine_boot_interface.rs (1)
47-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer table-driven tests for this fallible conversion.
These three
#[test]functions each exerciseMachineBootInterface::try_from(...)with a different wire-input shape (complete pair, missing/emptyinterface_id, unparseable MAC) — exactly the parser/conversion pattern the style guide calls out forscenarios!/Outcometable-driven tests. Consolidating them keeps future edge cases (e.g. another malformed MAC variant) a one-line addition instead of a new function.♻️ Suggested table-driven refactor
- #[test] - fn an_incomplete_wire_pair_is_refused() { - // A MAC-only wire value is valid to *send* (the id may not be captured - // yet) but never becomes the model type, which requires both halves. - let wire = rpc::forge::MachineBootInterface { - mac_address: "00:11:22:33:44:55".to_string(), - interface_id: None, - }; - assert!(matches!( - MachineBootInterface::try_from(wire), - Err(RpcDataConversionError::MissingArgument("interface_id")) - )); - - let empty_id = rpc::forge::MachineBootInterface { - mac_address: "00:11:22:33:44:55".to_string(), - interface_id: Some(String::new()), - }; - assert!(MachineBootInterface::try_from(empty_id).is_err()); - } - - #[test] - fn an_unparseable_mac_is_refused() { - let wire = rpc::forge::MachineBootInterface { - mac_address: "not-a-mac".to_string(), - interface_id: Some("NIC.Slot.7-1-1".to_string()), - }; - assert!(matches!( - MachineBootInterface::try_from(wire), - Err(RpcDataConversionError::InvalidArgument(_)) - )); - } + #[test] + fn try_from_wire_pair() { + use carbide_test_support::Outcome::*; + use carbide_test_support::scenarios; + + scenarios!(MachineBootInterface::try_from: + "missing interface_id" { + rpc::forge::MachineBootInterface { + mac_address: "00:11:22:33:44:55".to_string(), + interface_id: None, + } => FailsWith(RpcDataConversionError::MissingArgument("interface_id")), + } + "empty interface_id" { + rpc::forge::MachineBootInterface { + mac_address: "00:11:22:33:44:55".to_string(), + interface_id: Some(String::new()), + } => Fails, + } + "unparseable mac" { + rpc::forge::MachineBootInterface { + mac_address: "not-a-mac".to_string(), + interface_id: Some("NIC.Slot.7-1-1".to_string()), + } => Fails, + } + ); + }As per coding guidelines: "prefer table-driven tests using the
carbide-test-supportcrate withscenarios!for fallible operations" and "When writing tests, prefer table-driven tests... for parsers, validators, conversions, and similar 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/rpc/src/model/machine_boot_interface.rs` around lines 47 - 97, Consolidate the three conversion tests into one table-driven test using the carbide-test-support scenarios!/Outcome pattern. Cover the complete pair success case, missing or empty interface_id failures, and unparseable MAC failure, preserving the existing assertions for returned values and error variants while making additional malformed-input cases easy to add as scenarios.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.
Outside diff comments:
In `@crates/rpc/proto/forge.proto`:
- Around line 9093-9116: Rename the common.MachineInterfaceId field in
MachineInterfaceBootInterface from interface_id to a distinct row-key name such
as machine_interface_id, while leaving MachineBootInterface.interface_id
unchanged as the Redfish EthernetInterface.Id. Update references to the renamed
field consistently.
---
Nitpick comments:
In `@crates/rpc/src/model/machine_boot_interface.rs`:
- Around line 47-97: Consolidate the three conversion tests into one
table-driven test using the carbide-test-support scenarios!/Outcome pattern.
Cover the complete pair success case, missing or empty interface_id failures,
and unparseable MAC failure, preserving the existing assertions for returned
values and error variants while making additional malformed-input cases easy to
add as scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 12e38e76-c8ed-47f1-97da-077e98b0d699
⛔ 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 (22)
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/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/machine_boot_interface.rscrates/rpc/src/model/mod.rsrest-api/proto/core/src/v1/nico_nico.proto
✅ Files skipped from review due to trivial changes (1)
- crates/admin-cli/src/boot_interface/show/args.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- crates/admin-cli/src/boot_interface/candidates/mod.rs
- crates/admin-cli/src/boot_interface/mod.rs
- crates/admin-cli/src/boot_interface/set/args.rs
- crates/admin-cli/src/machine/mod.rs
- crates/admin-cli/cli_domains.yaml
- crates/api-core/src/handlers/machine_boot_interfaces.rs
- crates/admin-cli/src/boot_interface/set/mod.rs
- crates/admin-cli/src/boot_interface/show/cmd.rs
- crates/admin-cli/src/boot_interface/candidates/args.rs
- crates/admin-cli/src/cfg/cli_options.rs
- crates/api-core/tests/integration/machine_boot_interfaces.rs
- crates/admin-cli/src/boot_interface/show/mod.rs
- crates/admin-cli/src/main.rs
- crates/admin-cli/src/boot_interface/set/cmd.rs
- crates/api-model/src/machine/mod.rs
- crates/admin-cli/src/boot_interface/candidates/cmd.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/tests/integration/machine_boot_interfaces.rs (1)
256-271: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the MAC-only selection response.
These assertions seed a captured ID and exercise only the full-pair response. Add a case where the selected owned or predicted row lacks
boot_interface_id, asserting that its MAC is retained whileinterface_idis unset.As per coding guidelines, “Add or update focused tests for ... API contracts,” and path instructions, “Prefer findings about behavior, concurrency, resource lifetimes, and missing tests over style-only comments.”
🤖 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 256 - 271, Extend the test around predicted_boot_interface to cover a selected owned or predicted row without boot_interface_id. Assert that the response retains the selected MAC address while predicted_boot_interface.interface_id is unset, preserving the existing full-pair assertions for rows with a captured ID.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/tests/integration/machine_boot_interfaces.rs`:
- Around line 256-271: Extend the test around predicted_boot_interface to cover
a selected owned or predicted row without boot_interface_id. Assert that the
response retains the selected MAC address while
predicted_boot_interface.interface_id is unset, preserving the existing
full-pair assertions for rows with a captured ID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 28f7618f-028c-4ce9-9627-ed0b5792dd7a
⛔ 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 (22)
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/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/machine_boot_interface.rscrates/rpc/src/model/mod.rsrest-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 (a `MachineBootInterface` message for the default and predicted picks, mirroring the api-model pair with `From`/`TryFrom` conversions, 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 automatic fallback split out as `pick_default_boot_interface` in api-model, so the RPC handler reports it 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 updated: `cli_domains.yaml` (hardware) and the `rest-api` core-proto mirror (`nico_nico.proto` + Go bindings) regenerated for the additive fields; the generated CLI reference pages land in a follow-on docs-only PR. Tests added! This supports NVIDIA#3335 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
/ok to test ed8f49e |
|
@coderabbitai full_review, thanks! |
|
ᕦ(ò_óˇ)ᕤ ✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/admin-cli/src/boot_interface/set/args.rs (1)
75-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the parser tests table-driven.
These tests all exercise
InterfaceSelector::from_strwith different outcomes. A singlescenarios!table would keep the valid UUID, valid MAC, and invalid-input contract together.As per coding guidelines, fallible parsing tests should prefer
carbide-test-supporttable-driven cases.🤖 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 - 98, Consolidate the three `InterfaceSelector` parsing tests into one `scenarios!` table-driven test covering the valid UUID, valid MAC, and invalid-input cases. Use `carbide-test-support`’s table-driven parsing pattern, preserving the existing expected values and error-message assertion.Source: Coding guidelines
crates/api-model/src/machine/mod.rs (1)
3471-3507: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a table-driven selector test.
Both new tests exercise the same total function with different inputs. Consolidate them with
value_scenarios!orcheck_valuesso the selection contract remains labeled and easy to extend.As per coding guidelines, tests mapping inputs to outputs should prefer
carbide-test-supporttable-driven cases.🤖 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 3471 - 3507, The two separate tests for pick_default_boot_interface should be consolidated into a table-driven selector test using carbide-test-support’s value_scenarios! or check_values. Preserve labeled cases covering primary-flag masking and exclusion of underlay-only interfaces, including each expected selected MAC or None result, while keeping the effective pick assertion only where needed to establish the primary-flag contrast.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.
Nitpick comments:
In `@crates/admin-cli/src/boot_interface/set/args.rs`:
- Around line 75-98: Consolidate the three `InterfaceSelector` parsing tests
into one `scenarios!` table-driven test covering the valid UUID, valid MAC, and
invalid-input cases. Use `carbide-test-support`’s table-driven parsing pattern,
preserving the existing expected values and error-message assertion.
In `@crates/api-model/src/machine/mod.rs`:
- Around line 3471-3507: The two separate tests for pick_default_boot_interface
should be consolidated into a table-driven selector test using
carbide-test-support’s value_scenarios! or check_values. Preserve labeled cases
covering primary-flag masking and exclusion of underlay-only interfaces,
including each expected selected MAC or None result, while keeping the effective
pick assertion only where needed to establish the primary-flag contrast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4a0b5292-e175-490c-ae29-d933fda46885
⛔ 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 (22)
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/build.rscrates/rpc/proto/forge.protocrates/rpc/src/model/machine_boot_interface.rscrates/rpc/src/model/mod.rsrest-api/proto/core/src/v1/nico_nico.proto
1 participant
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 -- the default and predicted picks travel as aMachineBootInterfaceproto message mirroring the api-model pair (From/TryFrom); the CLI never re-derives selection logic.pick_boot_interface's automatic fallback 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.yamland therest-apicore-proto mirror + Go bindings regenerated; the generated CLI reference pages land separately in docs(cli): add the boot-interface command reference pages #3410.Tests added!
This supports #3335