fix(api): Address misclassification of NVOS data interface IPs as BMC IPs - #4395
Conversation
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
|
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
WalkthroughFinder address classification now uses interface type and switch metadata from database lookups. Static BMC results require a BMC interface. Machine and switch addresses receive distinct messages. Tests validate classifications and message content. ChangesFinder classification
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Finder
participant MachineInterfaceSearch
participant IpAddressMatch
Finder->>MachineInterfaceSearch: find_by_address
MachineInterfaceSearch-->>Finder: interface_type, switch_id, allocation metadata
Finder->>IpAddressMatch: classify address
Finder->>IpAddressMatch: format classification message
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/ok to test b921d16 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4395.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/api-core/src/handlers/finder.rs (1)
515-579: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse carbide-test-support table-driven helpers instead of a manual for-loop.
machine_interface_address_is_static_bmcis a total function (segment_name, allocation_type, interface_type) → bool, which fitsvalue_scenarios!/check_valuesper the repository's testing conventions rather than a hand-rolledforloop withassert_eq!.As per coding guidelines, "Use the
carbide-test-supporttable-driven testing helpers for functions mapping inputs to outputs or errors; usescenarios!forResult,value_scenarios!for total operations, and direct check helpers when macros obscure intent." Also, "Prefer table-driven tests; for Rust, use grouped carbide-test-support scenarios (scenarios!/value_scenarios!) or explicit cases (check_cases/check_values) to cover input variants."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/finder.rs` around lines 515 - 579, The test static_bmc_classification_requires_bmc_interface_type currently uses a manual loop and assertions for a total function. Replace the loop with the repository’s carbide-test-support table-driven helper, using value_scenarios! with check_values or the applicable direct helper, while preserving all existing input combinations, expected booleans, and case labels.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/api-core/src/handlers/finder.rs`:
- Around line 515-579: The test
static_bmc_classification_requires_bmc_interface_type currently uses a manual
loop and assertions for a total function. Replace the loop with the repository’s
carbide-test-support table-driven helper, using value_scenarios! with
check_values or the applicable direct helper, while preserving all existing
input combinations, expected booleans, and case labels.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 03620869-a42a-4bd4-9143-b618bba7a0de
📒 Files selected for processing (3)
crates/api-core/src/handlers/finder.rscrates/api-core/src/tests/finder.rscrates/api-db/src/machine_interface_address.rs
5382697 to
549ea90
Compare
Signed-off-by: Leah Itagaki <litagaki@nvidia.com>
… IPs (NVIDIA#4395) Previously our search IP address logic mistakenly categorized all statically allocated machine interface addresses as static BMC IPs. Now it looks to the interface type as well as the allocation type to categorize the interface. Additionally, if the response to an IP address search is associated with a switch, the CLI now prints the switch. ## Related issues Internal issue ## Type of Change - [x] **Change** - Changes in existing functionality - [x] **Fix** - Bug fixes ## Breaking Changes - [ ] **This PR contains breaking changes** ## Testing - [x] Unit tests added/updated - [x] Manual testing performed ## Additional Notes --------- Signed-off-by: Leah Itagaki <litagaki@nvidia.com> Signed-off-by: Alex Ball <aball@nvidia.com>
Previously our search IP address logic mistakenly categorized all statically allocated machine interface addresses as static BMC IPs. Now it looks to the interface type as well as the allocation type to categorize the interface.
Additionally, if the response to an IP address search is associated with a switch, the CLI now prints the switch.
Related issues
Internal issue
Type of Change
Breaking Changes
Testing
Additional Notes