refactor(bmc-mock): adopt style guide rules for pub/module visibility - #4635
Conversation
Summary by CodeRabbit
WalkthroughThe pull request narrows public visibility across the BMC mock’s hardware, routing, state, Redfish, injection, and support APIs. It removes selected unused fields, constants, and state handling while preserving most runtime behavior. It also updates DPU password documentation. ChangesBMC mock visibility and API cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/bmc-mock/src/lib.rs (1)
61-61: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUpdate the remaining documentation reference.
docs/development/machine-a-tron-deployment.md:213still namesDUMMY_FACTORY_DPU_PASSWORD, which no longer exists.🤖 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/bmc-mock/src/lib.rs` at line 61, Update the remaining documentation reference at the machine-a-tron deployment instructions to replace DUMMY_FACTORY_DPU_PASSWORD with the current configuration or credential name used by the project; do not change unrelated documentation.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/bmc-mock/src/mac_address_pool.rs`:
- Around line 348-355: Restore public visibility for
MacAddressPool::allocate_range so downstream users retain the existing API,
while preserving its current allocation behavior through allocate_range_config
and Self::new_pool.
In `@crates/bmc-mock/src/machine_info.rs`:
- Around line 717-722: Update PoolConfig::new and the Lenovo-specific allocation
path to reject host_bits values below 4, since Lenovo requires 15 MAC
allocations and smaller pools exhaust. Preserve valid pool creation for
host_bits = 4, and add boundary tests covering rejection at host_bits = 3 and
acceptance at host_bits = 4.
In `@crates/bmc-mock/src/redfish/leak_detector.rs`:
- Around line 49-70: Update LeakDetector and its builder/to_json flow to retain
a configurable detector state instead of hardcoding "DetectorState": "OK";
restore the state-to-Status mapping so Warning and Critical states produce their
corresponding Redfish status values while preserving the existing OK behavior.
Add mock response coverage exercising Warning and Critical states so the
leak-detector collector paths are validated.
---
Outside diff comments:
In `@crates/bmc-mock/src/lib.rs`:
- Line 61: Update the remaining documentation reference at the machine-a-tron
deployment instructions to replace DUMMY_FACTORY_DPU_PASSWORD with the current
configuration or credential name used by the project; do not change unrelated
documentation.
🪄 Autofix
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: b8c67dad-13b3-445e-8aa2-e8d7d6a6b44d
📒 Files selected for processing (86)
crates/bmc-mock/src/auth_router.rscrates/bmc-mock/src/bmc_state.rscrates/bmc-mock/src/command_line.rscrates/bmc-mock/src/hw/bluefield3.rscrates/bmc-mock/src/hw/bluefield4.rscrates/bmc-mock/src/hw/dell_poweredge_r750.rscrates/bmc-mock/src/hw/dell_poweredge_r760_bf4.rscrates/bmc-mock/src/hw/delta_power_shelf.rscrates/bmc-mock/src/hw/dgx_gb300_nvl.rscrates/bmc-mock/src/hw/dgx_vr_nvl.rscrates/bmc-mock/src/hw/generic_ami.rscrates/bmc-mock/src/hw/hpe_proliant_dl380a_gen11.rscrates/bmc-mock/src/hw/lenovo_gb300_nvl.rscrates/bmc-mock/src/hw/liteon_power_shelf.rscrates/bmc-mock/src/hw/mod.rscrates/bmc-mock/src/hw/nic.rscrates/bmc-mock/src/hw/nic_intel_e810.rscrates/bmc-mock/src/hw/nic_intel_i210.rscrates/bmc-mock/src/hw/nic_intel_x550.rscrates/bmc-mock/src/hw/nic_nvidia_cx7.rscrates/bmc-mock/src/hw/nvidia_dgx_h100.rscrates/bmc-mock/src/hw/nvidia_gb200.rscrates/bmc-mock/src/hw/nvidia_gb300.rscrates/bmc-mock/src/hw/nvidia_gbx00.rscrates/bmc-mock/src/hw/nvidia_switch_n5700_ld.rscrates/bmc-mock/src/hw/nvidia_switch_nd5200_ld.rscrates/bmc-mock/src/hw/supermicro_gb300_nvl.rscrates/bmc-mock/src/hw/wiwynn_gb200_nvl.rscrates/bmc-mock/src/hw/wiwynn_gb200_nvl72_rack.rscrates/bmc-mock/src/injection/mod.rscrates/bmc-mock/src/injection/presets.rscrates/bmc-mock/src/injection/store.rscrates/bmc-mock/src/ipmi.rscrates/bmc-mock/src/ipmi_sim.rscrates/bmc-mock/src/json.rscrates/bmc-mock/src/lib.rscrates/bmc-mock/src/mac_address_pool.rscrates/bmc-mock/src/machine_info.rscrates/bmc-mock/src/middleware_router.rscrates/bmc-mock/src/redfish/account_service.rscrates/bmc-mock/src/redfish/assembly.rscrates/bmc-mock/src/redfish/bios.rscrates/bmc-mock/src/redfish/boot_option.rscrates/bmc-mock/src/redfish/chassis.rscrates/bmc-mock/src/redfish/collection.rscrates/bmc-mock/src/redfish/computer_system.rscrates/bmc-mock/src/redfish/ethernet_interface.rscrates/bmc-mock/src/redfish/expander_router.rscrates/bmc-mock/src/redfish/host_interface.rscrates/bmc-mock/src/redfish/leak_detector.rscrates/bmc-mock/src/redfish/log_service.rscrates/bmc-mock/src/redfish/manager.rscrates/bmc-mock/src/redfish/manager_network_protocol.rscrates/bmc-mock/src/redfish/mod.rscrates/bmc-mock/src/redfish/network_adapter.rscrates/bmc-mock/src/redfish/network_device_function.rscrates/bmc-mock/src/redfish/oem/dell/idrac.rscrates/bmc-mock/src/redfish/oem/dell/mod.rscrates/bmc-mock/src/redfish/oem/dell/network_device_function.rscrates/bmc-mock/src/redfish/oem/mod.rscrates/bmc-mock/src/redfish/oem/nvidia/bluefield.rscrates/bmc-mock/src/redfish/oem/nvidia/mod.rscrates/bmc-mock/src/redfish/oem/supermicro/manager.rscrates/bmc-mock/src/redfish/oem/supermicro/mod.rscrates/bmc-mock/src/redfish/pcie_device.rscrates/bmc-mock/src/redfish/power_subsystem.rscrates/bmc-mock/src/redfish/power_supply.rscrates/bmc-mock/src/redfish/processor.rscrates/bmc-mock/src/redfish/resource.rscrates/bmc-mock/src/redfish/secure_boot.rscrates/bmc-mock/src/redfish/sensor.rscrates/bmc-mock/src/redfish/serial_console.rscrates/bmc-mock/src/redfish/serial_interface.rscrates/bmc-mock/src/redfish/service_root.rscrates/bmc-mock/src/redfish/session_service.rscrates/bmc-mock/src/redfish/software_inventory.rscrates/bmc-mock/src/redfish/storage.rscrates/bmc-mock/src/redfish/task_service.rscrates/bmc-mock/src/redfish/telemetry_service.rscrates/bmc-mock/src/redfish/thermal_subsystem.rscrates/bmc-mock/src/redfish/update_service.rscrates/bmc-mock/src/redfish/virtual_media.rscrates/bmc-mock/src/tar_router.rscrates/bmc-mock/src/test_support/mod.rscrates/machine-a-tron/src/config.rshelm-prereqs/setup-machine-a-tron.sh
💤 Files with no reviewable changes (1)
- crates/bmc-mock/src/ipmi_sim.rs
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/bmc-mock/src/mac_address_pool.rs (1)
348-355: 🗄️ Data Integrity & Integration | 🟠 MajorRestore the public
MacAddressPool::allocate_rangeAPI.The production method was removed, but Lines 348-355 add the operation only as a private test helper. This keeps local tests passing while downstream callers lose the method at compile time. The cross-crate use of
MacAddressPoolincrates/machine-a-tron/src/config.rsLine 23 confirms that local call sites do not define the complete API boundary.Keep a public production wrapper that delegates to
allocate_range_config().map(Self::new_pool), or document and version this breaking change.Proposed compatibility fix
impl MacAddressPool { + pub fn allocate_range(&mut self) -> Result<Self, Error> { + self.allocate_range_config().map(Self::new_pool) + } }As per coding guidelines and the PR objective, visibility changes must preserve required cross-crate APIs.
🤖 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/bmc-mock/src/mac_address_pool.rs` around lines 348 - 355, Restore MacAddressPool::allocate_range as a public production method rather than leaving the current private test helper. Update the allocate_range method in impl MacAddressPool to expose the existing wrapper that delegates to allocate_range_config().map(Self::new_pool), preserving cross-crate callers without changing the allocation behavior.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/bmc-mock/src/redfish/oem/nvidia/bluefield.rs`:
- Line 41: Make the ModeState declaration private by removing its pub(crate)
visibility modifier, while leaving the struct and its usages within bluefield.rs
unchanged.
---
Duplicate comments:
In `@crates/bmc-mock/src/mac_address_pool.rs`:
- Around line 348-355: Restore MacAddressPool::allocate_range as a public
production method rather than leaving the current private test helper. Update
the allocate_range method in impl MacAddressPool to expose the existing wrapper
that delegates to allocate_range_config().map(Self::new_pool), preserving
cross-crate callers without changing the allocation behavior.
🪄 Autofix
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: 9ec48681-2a52-4b9b-95e2-db1c0c43ef03
📒 Files selected for processing (85)
crates/bmc-mock/src/auth_router.rscrates/bmc-mock/src/bmc_state.rscrates/bmc-mock/src/command_line.rscrates/bmc-mock/src/hw/bluefield3.rscrates/bmc-mock/src/hw/bluefield4.rscrates/bmc-mock/src/hw/dell_poweredge_r750.rscrates/bmc-mock/src/hw/dell_poweredge_r760_bf4.rscrates/bmc-mock/src/hw/delta_power_shelf.rscrates/bmc-mock/src/hw/dgx_gb300_nvl.rscrates/bmc-mock/src/hw/dgx_vr_nvl.rscrates/bmc-mock/src/hw/generic_ami.rscrates/bmc-mock/src/hw/hpe_proliant_dl380a_gen11.rscrates/bmc-mock/src/hw/lenovo_gb300_nvl.rscrates/bmc-mock/src/hw/liteon_power_shelf.rscrates/bmc-mock/src/hw/mod.rscrates/bmc-mock/src/hw/nic.rscrates/bmc-mock/src/hw/nic_intel_e810.rscrates/bmc-mock/src/hw/nic_intel_i210.rscrates/bmc-mock/src/hw/nic_intel_x550.rscrates/bmc-mock/src/hw/nic_nvidia_cx7.rscrates/bmc-mock/src/hw/nvidia_dgx_h100.rscrates/bmc-mock/src/hw/nvidia_gb200.rscrates/bmc-mock/src/hw/nvidia_gb300.rscrates/bmc-mock/src/hw/nvidia_gbx00.rscrates/bmc-mock/src/hw/nvidia_switch_n5700_ld.rscrates/bmc-mock/src/hw/nvidia_switch_nd5200_ld.rscrates/bmc-mock/src/hw/supermicro_gb300_nvl.rscrates/bmc-mock/src/hw/wiwynn_gb200_nvl.rscrates/bmc-mock/src/hw/wiwynn_gb200_nvl72_rack.rscrates/bmc-mock/src/injection/mod.rscrates/bmc-mock/src/injection/presets.rscrates/bmc-mock/src/injection/store.rscrates/bmc-mock/src/ipmi.rscrates/bmc-mock/src/ipmi_sim.rscrates/bmc-mock/src/json.rscrates/bmc-mock/src/lib.rscrates/bmc-mock/src/mac_address_pool.rscrates/bmc-mock/src/machine_info.rscrates/bmc-mock/src/middleware_router.rscrates/bmc-mock/src/redfish/account_service.rscrates/bmc-mock/src/redfish/assembly.rscrates/bmc-mock/src/redfish/bios.rscrates/bmc-mock/src/redfish/boot_option.rscrates/bmc-mock/src/redfish/chassis.rscrates/bmc-mock/src/redfish/collection.rscrates/bmc-mock/src/redfish/computer_system.rscrates/bmc-mock/src/redfish/ethernet_interface.rscrates/bmc-mock/src/redfish/expander_router.rscrates/bmc-mock/src/redfish/host_interface.rscrates/bmc-mock/src/redfish/leak_detector.rscrates/bmc-mock/src/redfish/log_service.rscrates/bmc-mock/src/redfish/manager.rscrates/bmc-mock/src/redfish/manager_network_protocol.rscrates/bmc-mock/src/redfish/mod.rscrates/bmc-mock/src/redfish/network_adapter.rscrates/bmc-mock/src/redfish/network_device_function.rscrates/bmc-mock/src/redfish/oem/dell/idrac.rscrates/bmc-mock/src/redfish/oem/dell/mod.rscrates/bmc-mock/src/redfish/oem/dell/network_device_function.rscrates/bmc-mock/src/redfish/oem/mod.rscrates/bmc-mock/src/redfish/oem/nvidia/bluefield.rscrates/bmc-mock/src/redfish/oem/nvidia/mod.rscrates/bmc-mock/src/redfish/oem/supermicro/manager.rscrates/bmc-mock/src/redfish/oem/supermicro/mod.rscrates/bmc-mock/src/redfish/pcie_device.rscrates/bmc-mock/src/redfish/power_subsystem.rscrates/bmc-mock/src/redfish/power_supply.rscrates/bmc-mock/src/redfish/processor.rscrates/bmc-mock/src/redfish/resource.rscrates/bmc-mock/src/redfish/secure_boot.rscrates/bmc-mock/src/redfish/sensor.rscrates/bmc-mock/src/redfish/serial_console.rscrates/bmc-mock/src/redfish/serial_interface.rscrates/bmc-mock/src/redfish/service_root.rscrates/bmc-mock/src/redfish/session_service.rscrates/bmc-mock/src/redfish/software_inventory.rscrates/bmc-mock/src/redfish/storage.rscrates/bmc-mock/src/redfish/task_service.rscrates/bmc-mock/src/redfish/telemetry_service.rscrates/bmc-mock/src/redfish/thermal_subsystem.rscrates/bmc-mock/src/redfish/update_service.rscrates/bmc-mock/src/redfish/virtual_media.rscrates/bmc-mock/src/tar_router.rscrates/bmc-mock/src/test_support/mod.rscrates/machine-a-tron/src/config.rs
💤 Files with no reviewable changes (1)
- crates/bmc-mock/src/ipmi_sim.rs
This adopts the new style guide rules around module visibility introduced in NVIDIA#4522, applying the correct visibility throughout BMC Mock. Primary callouts are: - Replace unrestricted `pub` throughout `crates/bmc-mock/src/**` with private, `pub(super)`, or `pub(crate)` visibility based on actual callers. - Keep the mock server/router, `MachineInfo`, injection controls, IPMI simulator, MAC pools, callback backends, and integration-test support public where they cross crate boundaries. - Make the hardware and Redfish implementation trees internal while keeping `RackElevation` and `RackUnit` available through intentional root re-exports. - Remove unused exports, helpers, and nonserialized state, including stale blanket dead-code allowances. - Preserve HTTP routes, serialized Redfish responses, supported test fixtures, deterministic MAC allocation, and runtime behavior. Tests updated! This supports NVIDIA#4550. This supports the updated Rust visibility scoping guidelines in `STYLE_GUIDE.md`, established in NVIDIA#4522. Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
The remaining deployment-guide reference called out in the review is tracked in #4633 together with the setup-script comment, since those paths share a separate reviewer boundary. |
This adopts the new style guide rules around module visibility introduced in #4522, applying the correct visibility throughout BMC Mock.
Primary callouts are:
pubthroughoutcrates/bmc-mock/src/**with private,pub(super), orpub(crate)visibility based on actual callers.MachineInfo, injection controls, IPMI simulator, MAC pools, callback backends, and integration-test support public where they cross crate boundaries.RackElevationandRackUnitavailable through intentional root re-exports.Tests updated!
Related issues
This supports #4550.
This supports the updated Rust visibility scoping guidelines in
STYLE_GUIDE.md, established in #4522.The corresponding Machine-a-tron deployment-guide and setup-script credential-source corrections are tracked in #4633 so they can stay together behind the required reviewer boundary.
Type of Change
Breaking Changes
Testing
All 84 BMC Mock tests passed. I also checked every direct workspace consumer and ran:
Additional Notes
The initial visibility lint identified 397 overbroad declarations across 48 files. Narrowing the crate's root gateways exposed an additional internal Redfish frontier; the final 85-file diff includes that follow-through plus the directly related Machine-a-tron rustdoc correction. The only unrestricted public surfaces left are backed by cross-crate callers, integration-test consumers, or intentional root re-exports.
Local CodeRabbit and read-only Claude reviews completed over the full diff. All applicable findings are incorporated.
Closes #4550