refactor(dhcp,pxe): adopt style guide rules for pub/module visibility - #4647
Conversation
|
@coderabbitai full_review, thanks! |
|
✅ Action performedFull review finished. |
Summary by CodeRabbit
WalkthroughThe PR narrows internal API visibility across the DHCP server, DHCP, DHCP test-support, and PXE crates. It also gates test-support dependencies, removes one exported C-ABI function, removes unused PXE error variants, and limits metric label propagation. ChangesInternal API encapsulation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/dhcp/tests/common/mod.rs (1)
23-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope shared test fixtures instead of suppressing lints.
These
#[allow(dead_code)]and#[allow(unused_imports)]attributes hide unused fixtures and re-exports in individual integration-test targets. Move each fixture or re-export into the test modules that use it. Keepcommon/mod.rslimited to genuinely shared items.As per coding guidelines, avoid
#[allow(...)], especially#[allow(dead_code)]; remove unused code or gate test- and feature-specific code appropriately.Also applies to: 31-32, 34-37, 39-40, 42-47, 53-55
🤖 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/dhcp/tests/common/mod.rs` around lines 23 - 24, Remove the #[allow(dead_code)] and #[allow(unused_imports)] suppressions from common/mod.rs, including the declarations around kea_v6 and the other listed fixtures/re-exports. Move each fixture or re-export into the specific integration-test module that uses it, leaving common/mod.rs only with genuinely shared items and gating test- or feature-specific code appropriately.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/pxe/src/middleware/metrics.rs`:
- Around line 157-161: Bound the endpoint label in MetricLayer by using the
literal "unknown" whenever MatchedPath is absent, rather than copying the
request URI; preserve matched-route labels unchanged. Add regression tests
covering unmatched requests and requests handled by nested services.
---
Nitpick comments:
In `@crates/dhcp/tests/common/mod.rs`:
- Around line 23-24: Remove the #[allow(dead_code)] and #[allow(unused_imports)]
suppressions from common/mod.rs, including the declarations around kea_v6 and
the other listed fixtures/re-exports. Move each fixture or re-export into the
specific integration-test module that uses it, leaving common/mod.rs only with
genuinely shared items and gating test- or feature-specific code appropriately.
🪄 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: a48376ea-4742-43ae-a39a-83bf397b241e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
crates/dhcp-server/src/cache.rscrates/dhcp-server/src/command_line.rscrates/dhcp-server/src/grpc_server.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/metrics.rscrates/dhcp-server/src/modes/controller.rscrates/dhcp-server/src/modes/dpu.rscrates/dhcp-server/src/modes/mod.rscrates/dhcp-server/src/packet_handler.rscrates/dhcp-server/src/rpc/client.rscrates/dhcp-server/src/rpc/mod.rscrates/dhcp-server/src/util.rscrates/dhcp/Cargo.tomlcrates/dhcp/src/cache.rscrates/dhcp/src/discovery.rscrates/dhcp/src/discovery_v6.rscrates/dhcp/src/kea/ffi.rscrates/dhcp/src/kea_logger.rscrates/dhcp/src/lib.rscrates/dhcp/src/machine.rscrates/dhcp/src/machine_v6.rscrates/dhcp/src/metrics.rscrates/dhcp/src/mock_api_server.rscrates/dhcp/src/tls.rscrates/dhcp/tests/common/dhcp_factory.rscrates/dhcp/tests/common/dhcpv6_factory.rscrates/dhcp/tests/common/kea.rscrates/dhcp/tests/common/kea_v6.rscrates/dhcp/tests/common/mod.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/extractors/machine_architecture.rscrates/pxe/src/extractors/mod.rscrates/pxe/src/main.rscrates/pxe/src/metrics.rscrates/pxe/src/middleware/metrics.rscrates/pxe/src/middleware/mod.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/ipxe.rscrates/pxe/src/routes/metrics.rscrates/pxe/src/routes/mod.rscrates/pxe/src/routes/tls.rscrates/pxe/src/rpc_error.rs
This adopts the new style guide rules around module visibility introduced in NVIDIA#4522, applying the correct visibility throughout DHCP, DHCP Server, and PXE. Primary callouts are: - Replace unrestricted `pub` throughout `crates/dhcp/**`, `crates/dhcp-server/src/**`, and `crates/pxe/src/**` with private, `pub(super)`, or `pub(crate)` visibility based on actual callers. - Keep the cbindgen-declared Kea hook ABI public and leave generated tonic declarations untouched behind a private module boundary. - Gate cross-crate `mock_api_server` support behind an explicit `test-support` feature, with its mock-only dependency edges enabled by that feature and failure injection kept unit-only. - Narrow DHCP integration-test fixtures under `tests/common/**` to the test crates and parent modules that actually use them. - Remove callerless DHCP response/factory helpers, the redundant `machine_free_ntpservers` symbol, PXE's unused `ExtraMetricLabels` extension, and unused PXE error variants. - Leave the cbindgen-declared Kea hook ABI, gRPC contract, DHCP/PXE protocol behavior, and metric names unchanged. Tests updated! This supports NVIDIA#4552. 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. |
|
@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 `@crates/dhcp/tests/common/mod.rs`:
- Around line 22-55: Remove the broad dead_code and unused_imports suppressions
in the shared DHCP test module, including those on kea_v6, factory re-exports,
Kea types, and helper functions. Keep only helpers and re-exports with actual
parent-module callers; move target-specific helpers into the integration-test
targets that use them or gate them with an explicit test-support configuration,
while preserving clippy-clean compilation without broad allows.
🪄 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: d2b53461-0e0f-4a16-b779-f3a2868580ff
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (43)
crates/dhcp-server/src/cache.rscrates/dhcp-server/src/command_line.rscrates/dhcp-server/src/grpc_server.rscrates/dhcp-server/src/main.rscrates/dhcp-server/src/metrics.rscrates/dhcp-server/src/modes/controller.rscrates/dhcp-server/src/modes/dpu.rscrates/dhcp-server/src/modes/mod.rscrates/dhcp-server/src/packet_handler.rscrates/dhcp-server/src/rpc/client.rscrates/dhcp-server/src/rpc/mod.rscrates/dhcp-server/src/util.rscrates/dhcp/Cargo.tomlcrates/dhcp/src/cache.rscrates/dhcp/src/discovery.rscrates/dhcp/src/discovery_v6.rscrates/dhcp/src/kea/ffi.rscrates/dhcp/src/kea_logger.rscrates/dhcp/src/lib.rscrates/dhcp/src/machine.rscrates/dhcp/src/machine_v6.rscrates/dhcp/src/metrics.rscrates/dhcp/src/mock_api_server.rscrates/dhcp/src/tls.rscrates/dhcp/tests/common/dhcp_factory.rscrates/dhcp/tests/common/dhcpv6_factory.rscrates/dhcp/tests/common/kea.rscrates/dhcp/tests/common/kea_v6.rscrates/dhcp/tests/common/mod.rscrates/pxe/src/common.rscrates/pxe/src/config.rscrates/pxe/src/extractors/machine_architecture.rscrates/pxe/src/extractors/mod.rscrates/pxe/src/main.rscrates/pxe/src/metrics.rscrates/pxe/src/middleware/metrics.rscrates/pxe/src/middleware/mod.rscrates/pxe/src/routes/cloud_init.rscrates/pxe/src/routes/ipxe.rscrates/pxe/src/routes/metrics.rscrates/pxe/src/routes/mod.rscrates/pxe/src/routes/tls.rscrates/pxe/src/rpc_error.rs
This adopts the new style guide rules around module visibility introduced in #4522, applying the correct visibility throughout DHCP, DHCP Server, and PXE.
Primary callouts are:
pubthroughoutcrates/dhcp/**,crates/dhcp-server/src/**, andcrates/pxe/src/**with private,pub(super), orpub(crate)visibility based on actual callers.mock_api_serversupport behind an explicittest-supportfeature, with its mock-only dependency edges enabled by that feature and failure injection kept unit-only.tests/common/**to the test crates and parent modules that actually use them.machine_free_ntpserverssymbol, PXE's unusedExtraMetricLabelsextension, and unused PXE error variants.Tests updated!
Related issues
This supports #4552.
This supports the updated Rust visibility scoping guidelines in
STYLE_GUIDE.md, established in #4522.Type of Change
Breaking Changes
Testing
All 59 DHCP unit tests, 38 DHCP integration tests, 29 DHCP Server tests, and 28 PXE tests pass. I also ran:
Additional Notes
The initial compiler inventory found 242 overbroad declarations: 184 in DHCP, 35 in DHCP Server, and 23 in PXE. The final 44-file diff also includes the integration-test boundaries and callerless code exposed by that cleanup. The remaining unrestricted
pubdeclarations are limited to the cbindgen-declared C ABI, explicit feature-gated integration-test support, and generated tonic declarations behind a private module.Ubuntu's path-specific AppArmor profiles for
/usr/sbin/kea-dhcp4and/usr/sbin/kea-dhcp6denied the test harness's temporary PID and lock paths. I ran the complete DHCP integration suite against SHA-256-identical executable copies outside those profiles, then restored the two temporary launch-path edits byte-for-byte; no temporary path remains in this diff.Local CodeRabbit and the independent fresh-eyes review returned zero findings. The applicable read-only Claude findings around optional mock dependencies, private cache state, stale comments, and scope-equivalent visibility edits are incorporated. I declined one suggestion to expose unit-only failure injection through the integration-test feature because no integration caller needs that API.
Closes #4552