feat(admin-cli): add --host_nics to expected-machine patch#3360
feat(admin-cli): add --host_nics to expected-machine patch#3360lauragra-y wants to merge 1 commit into
Conversation
|
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. |
WalkthroughThe expected-machine patch command adds a ChangesExpected-machine host NIC patching
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
lauragra-y
force-pushed
the
expected-machine-patch-host-nics
branch
4 times, most recently
from
bafc611 to
fabc496
Compare
`expected-machine patch` could not update a machine's host NICs -- the merge path preserved the existing list (per an old TODO). Expose `--host_nics` on patch, mirroring `expected-machine add`: a JSON array of ExpectedHostNic that full-replaces the machine's host NIC list. Omitting the flag preserves the existing list. Also preserve sku_id on partial patches: the merge passed the raw `--sku-id` param straight through, and the DB update binds sku_id directly (not COALESCE), so any patch without `--sku-id` (now including a host-NIC-only patch) silently cleared an existing SKU. Fall back to the stored value with `.or(expected_machine.sku_id)`, matching the other preserved fields. File-based `expected-machine update` keeps its prior behavior (host_nics untouched); its JSON cannot distinguish an omitted list from a cleared one, so wiring that through is left as a separate change.
lauragra-y
force-pushed
the
expected-machine-patch-host-nics
branch
from
fabc496 to
5f38986
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
crates/admin-cli/src/rpc.rs (3)
810-812: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment doesn't call out
host_nics's full-replace semantics.Unlike labels/metadata (merged field-by-field),
host_nicsfully replaces the stored list when provided (including clearing it with[]). Worth a line in the doc comment so future readers don't assume merge semantics.🤖 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/rpc.rs` around lines 810 - 812, Update the doc comment for the partial expected-machine update function near `update_expected_machine` to explicitly state that a provided `host_nics` list fully replaces the stored list, including that `[]` clears it, unlike field-by-field merged labels and metadata.
813-834: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftParameter list keeps growing on an already-
#[allow(clippy::too_many_arguments)]'d function.
patch_expected_machinenow takes 19 parameters and still needs the clippy allow it already carries. Per repo style guide, code with "a large struct with lots of required fields and lots of non-required fields" should be split into a required-fields type plus aConfig/Paramstype for the rest — the same principle applies to this parameter list. Consolidating into aPatchExpectedMachineRequest { .. }struct literal would let this allow be removed and make future additions (likehost_nicshere) additive without further bloating the signature.🤖 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/rpc.rs` around lines 813 - 834, Refactor patch_expected_machine to accept a PatchExpectedMachineRequest struct containing the current parameters, separating required fields from optional configuration fields as appropriate. Update all callers to construct a PatchExpectedMachineRequest literal, preserve existing behavior and validation, and remove the #[allow(clippy::too_many_arguments)] attribute from patch_expected_machine.Source: Coding guidelines
906-909: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winJSON parse error lacks field context, and the parser itself is untested.
.transpose()?propagates a bareserde_json::Errorwith no indication it came from--host_nics, contrary to this crate's guidance to write error chains that read as a coherent "while attempting to …" story. Additionally, this JSON→Vec<ExpectedHostNic>conversion is a pure parser with zero test coverage; per the style guide, parsers/conversions should get table-drivenscenarios!/value_scenarios!coverage, which is easiest if extracted into a small named function.♻️ Suggested tweak for actionable error context
- host_nics: host_nics - .map(|s| serde_json::from_str::<Vec<rpc::ExpectedHostNic>>(&s)) - .transpose()? - .unwrap_or(expected_machine.host_nics), + host_nics: host_nics + .map(|s| { + serde_json::from_str::<Vec<rpc::ExpectedHostNic>>(&s).map_err(|e| { + CarbideCliError::GenericError(format!( + "invalid --host_nics JSON while patching expected machine: {e}" + )) + }) + }) + .transpose()? + .unwrap_or(expected_machine.host_nics),#!/bin/bash # Confirm CarbideCliError's conversion from serde_json::Error (used implicitly via `?` here and in update/mod.rs) rg -n "From<serde_json::Error>|serde_json::Error" crates/admin-cli/src -A3 -B3🤖 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/rpc.rs` around lines 906 - 909, Extract the `--host_nics` JSON conversion from the surrounding RPC construction into a named parser function, such as `parse_host_nics`, returning `Result<Vec<rpc::ExpectedHostNic>, _>`. Add field-specific context to its deserialization error so the chain clearly identifies `--host_nics`, then use the helper in the `host_nics` assignment while preserving the default fallback. Add table-driven `scenarios!` or `value_scenarios!` tests covering valid JSON, invalid JSON, and relevant empty/default cases.Source: Coding guidelines
crates/admin-cli/src/expected_machines/patch/args.rs (2)
221-260: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
host_nicsJSON eagerly instead of deferring topatch_expected_machine.
validate()only checks thathost_nicsis present, not that it's well-formed JSON. Malformed input is only caught deep insidepatch_expected_machine(crates/admin-cli/src/rpc.rs), after an unnecessaryget_expected_machineround-trip. Parsing (and surfacing the error) here would fail fast and keep the RPC parsing logic simpler.♻️ Proposed eager-validation sketch
if self .fallback_dpu_serial_numbers .as_ref() .is_some_and(has_duplicates) { return Err(CarbideCliError::GenericError( "Duplicate dpu serial numbers found".to_string(), )); } + if let Some(json) = &self.host_nics { + serde_json::from_str::<Vec<::rpc::forge::ExpectedHostNic>>(json).map_err(|e| { + CarbideCliError::GenericError(format!("invalid --host_nics JSON: {e}")) + })?; + } Ok(())🤖 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/expected_machines/patch/args.rs` around lines 221 - 260, Update Args::validate to eagerly parse self.host_nics when present, returning a CarbideCliError containing the JSON parsing failure; leave it absent as valid. Then simplify patch_expected_machine’s host_nics handling by using the already-validated representation or retaining only the necessary conversion without deferring malformed-JSON errors until after get_expected_machine.
205-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
--host_nicsexample to theEXAMPLES:block. The help text already covers the command, but it still does not show the JSON array payload for--host_nics; add one copy-pasteable invocation so operators can see the expected format.🤖 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/expected_machines/patch/args.rs` around lines 205 - 212, Add a copy-pasteable `--host_nics` invocation to the `EXAMPLES:` block associated with the expected-machine patch arguments, using a JSON array containing the documented fields and valid shell quoting. Keep the example consistent with the `host_nics` option defined in the `host_nics` argument declaration.Sources: Coding guidelines, Path instructions
crates/admin-cli/src/expected_machines/update/mod.rs (1)
78-80: 🗄️ Data Integrity & Integration | 🔵 TrivialTODO acknowledged — file-based
updateintentionally doesn't touchhost_nicsyet.Passing
Nonecorrectly preserves the stored list given rpc.rs'sunwrap_or(expected_machine.host_nics)fallback, and this matches the PR's stated scope (file-basedupdatebehavior unchanged). Once ready, wiringexpected_machine.host_nicsin here (serialized to JSON, matching the--host_nicscontract) would close the gap with thepatchcommand. Happy to draft that follow-up if useful.🤖 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/expected_machines/update/mod.rs` around lines 78 - 80, Leave the file-based update behavior unchanged: retain None at the host_nics argument in the update flow so rpc.rs preserves the stored list via its fallback. Treat wiring expected_machine.host_nics as a separate follow-up, serialized to JSON consistently with the --host_nics contract and patch command.
🤖 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/expected_machines/patch/args.rs`:
- Around line 221-260: Update Args::validate to eagerly parse self.host_nics
when present, returning a CarbideCliError containing the JSON parsing failure;
leave it absent as valid. Then simplify patch_expected_machine’s host_nics
handling by using the already-validated representation or retaining only the
necessary conversion without deferring malformed-JSON errors until after
get_expected_machine.
- Around line 205-212: Add a copy-pasteable `--host_nics` invocation to the
`EXAMPLES:` block associated with the expected-machine patch arguments, using a
JSON array containing the documented fields and valid shell quoting. Keep the
example consistent with the `host_nics` option defined in the `host_nics`
argument declaration.
In `@crates/admin-cli/src/expected_machines/update/mod.rs`:
- Around line 78-80: Leave the file-based update behavior unchanged: retain None
at the host_nics argument in the update flow so rpc.rs preserves the stored list
via its fallback. Treat wiring expected_machine.host_nics as a separate
follow-up, serialized to JSON consistently with the --host_nics contract and
patch command.
In `@crates/admin-cli/src/rpc.rs`:
- Around line 810-812: Update the doc comment for the partial expected-machine
update function near `update_expected_machine` to explicitly state that a
provided `host_nics` list fully replaces the stored list, including that `[]`
clears it, unlike field-by-field merged labels and metadata.
- Around line 813-834: Refactor patch_expected_machine to accept a
PatchExpectedMachineRequest struct containing the current parameters, separating
required fields from optional configuration fields as appropriate. Update all
callers to construct a PatchExpectedMachineRequest literal, preserve existing
behavior and validation, and remove the #[allow(clippy::too_many_arguments)]
attribute from patch_expected_machine.
- Around line 906-909: Extract the `--host_nics` JSON conversion from the
surrounding RPC construction into a named parser function, such as
`parse_host_nics`, returning `Result<Vec<rpc::ExpectedHostNic>, _>`. Add
field-specific context to its deserialization error so the chain clearly
identifies `--host_nics`, then use the helper in the `host_nics` assignment
while preserving the default fallback. Add table-driven `scenarios!` or
`value_scenarios!` tests covering valid JSON, invalid JSON, and relevant
empty/default cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b8094938-4625-4f3e-8939-977236d9aea1
📒 Files selected for processing (5)
crates/admin-cli/src/expected_machines/patch/args.rscrates/admin-cli/src/expected_machines/patch/mod.rscrates/admin-cli/src/expected_machines/tests.rscrates/admin-cli/src/expected_machines/update/mod.rscrates/admin-cli/src/rpc.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
3 participants
Description
nico-admin-cli expected-machine patchcould not update a machine's host NICs: the merge path inpatch_expected_machinepreserved the existing list behind an oldTODO(chet), so the field was never patchable.This exposes
--host_nicsonpatch, mirroringexpected-machine add- a JSON array ofExpectedHostNicthat full-replaces the machine's host NIC list. Omitting the flag preserves the existing list;--host_nics '[]'clears it.Also fixes a pre-existing bug this path newly exposes: partial patches passed the raw
--sku-idstraight through, and the DB update bindssku_iddirectly (notCOALESCE), so any patch without--sku-id(now including a host-NIC-only patch) silently cleared an existing SKU. It now falls back to the stored value via.or(expected_machine.sku_id), matching the other preserved fields.Type of Change
Related Issues (Optional)
Closes #3376
Breaking Changes
Testing
Additional Notes