docs: note the full-pair requirement in fetch_host_primary_interface_mac#2722
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
Walkthrough
ChangesDeclared Primary MAC Override
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/site-explorer/tests/site_explorer.rs (1)
2255-2303: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse a table-driven test and include the unresolved-interface-id fallback case.
This block tests one operation across multiple input variants; please convert it to
value_scenarios!(orcheck_values) and add a case where the declared MAC is present but does not resolve to a non-empty interface id, asserting fallback to the automatic pick.As per coding guidelines, "Reach for a table whenever two or more tests call the same operation with different inputs" and "Use
carbide_test_support::value_scenarios!macro for total 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/site-explorer/tests/site_explorer.rs` around lines 2255 - 2303, The test code contains multiple calls to fetch_host_primary_interface_mac with different input variants that should be refactored using table-driven testing. Convert this test block to use the value_scenarios! macro from carbide_test_support to parametrize the different test cases (the automatic pick case, declared DPU PF case, declared integrated NIC case, and absent MAC case). Additionally, add a new test case scenario where a declared MAC is present in the host report but does not resolve to a non-empty interface id, and verify that it falls back to the automatic pick behavior, as this edge case is currently missing from the test coverage.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/site-explorer/tests/site_explorer.rs`:
- Around line 2255-2303: The test code contains multiple calls to
fetch_host_primary_interface_mac with different input variants that should be
refactored using table-driven testing. Convert this test block to use the
value_scenarios! macro from carbide_test_support to parametrize the different
test cases (the automatic pick case, declared DPU PF case, declared integrated
NIC case, and absent MAC case). Additionally, add a new test case scenario where
a declared MAC is present in the host report but does not resolve to a non-empty
interface id, and verify that it falls back to the automatic pick behavior, as
this edge case is currently missing from the test coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1fba53a3-04e4-4f36-9e74-1ef7747e8b95
📒 Files selected for processing (3)
crates/api-model/src/site_explorer/mod.rscrates/site-explorer/src/lib.rscrates/site-explorer/tests/site_explorer.rs
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
The doc comment on fetch_host_primary_interface_mac (added with the declared-primary support) said a declared primary wins "when this report has that NIC", but the actual gate -- find_interface_id_for_mac -- needs a full pair: the MAC present on a system ethernet interface AND a non-empty Redfish interface id. A declared NIC whose id this report has not resolved yet falls back to the automatic DPU-PF pick, alongside the no-declaration case. Spell that out so a reader of the doc alone does not expect any in-report MAC to win. Behavior is unchanged. Part of NVIDIA#2662 (epic NVIDIA#2660). Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai review |
chet
force-pushed
the
explored-default-doc-clarity
branch
from
c645b1f to
a098673
Compare
✅ Action performedReview finished.
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2722.docs.buildwithfern.com/infra-controller |
2 participants
What
Doc-clarity follow-up to #2721 (CodeRabbit's one optional chat note there). No behavior change -- a doc comment only.
#2721's doc comment on
fetch_host_primary_interface_macsaid a declared primary wins "when this report has that NIC." The actual gate (find_interface_id_for_mac) requires a full pair -- the MAC present on a system ethernet interface and a non-empty Redfish interface id. A declared NIC whose id this report hasn't resolved yet falls back to the automatic DPU-PF pick, same as the no-declaration case. This spells that out in the public doc so a reader of the doc alone doesn't expect any in-report MAC to win.Notes
explored-default-declared-primary) -- the doc comment this tightens only exists there, so this branches off feat: honor a declared primary NIC in the explored boot default #2721's commit. Until feat: honor a declared primary NIC in the explored boot default #2721 merges the diff below includes feat: honor a declared primary NIC in the explored boot default #2721's change; once feat: honor a declared primary NIC in the explored boot default #2721 lands I'll rebase this ontomainso it's just the doc edit.Part of #2662 (epic #2660).