Fix iPXE NICo branding#2583
Merged
Merged
Conversation
Contributor
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
Summary by CodeRabbit
WalkthroughRebrands all iPXE-related assets from "Carbide" to "NICo": updates branding macros in ChangesCarbide → NICo Rebranding
DHCP Booturl Test Deadline-Based Retry Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
osu
force-pushed
the
fix/ipxe-nico-branding
branch
2 times, most recently
from
0c68917 to
f72ef93
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ipxe-renderer/templates.yaml (1)
329-392: 🏗️ Heavy liftAvoid drift between the static template and
pxe/ipxe/local/embed.ipxe.This block is effectively a second copy of the embedded boot menu script. This PR needed mirrored edits in both places; future changes can diverge silently. Please consider generating one from the other (or sharing a single source snippet) to keep control-flow and branding synchronized.
As per coding guidelines, "Prefer simple, explicit code over clever or heavily abstracted code. Optimize for readability and maintainability first."
🤖 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/ipxe-renderer/templates.yaml` around lines 329 - 392, The NICo Menu template is duplicated between two locations, creating a maintenance burden where changes must be made in both places to keep them synchronized. Consolidate the template code by extracting the common boot menu script into a single source of truth. Modify the template definition to generate or reference this shared source, ensuring the control-flow and branding remain consistent across all uses without requiring duplicate edits in the future.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/ipxe-renderer/templates.yaml`:
- Around line 329-392: The NICo Menu template is duplicated between two
locations, creating a maintenance burden where changes must be made in both
places to keep them synchronized. Consolidate the template code by extracting
the common boot menu script into a single source of truth. Modify the template
definition to generate or reference this shared source, ensuring the
control-flow and branding remain consistent across all uses without requiring
duplicate edits in the future.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7cc36479-b676-4029-9dc0-1c073af7a1e7
📒 Files selected for processing (4)
crates/ipxe-renderer/src/lib.rscrates/ipxe-renderer/templates.yamlpxe/ipxe/local/branding.hpxe/ipxe/local/embed.ipxe
osu
force-pushed
the
fix/ipxe-nico-branding
branch
3 times, most recently
from
ef695ed to
8235236
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/ipxe-renderer/src/lib.rs (1)
1061-1108: 💤 Low valueConsider consolidating redundant assertions.
The test asserts
menu_template.template == STATIC_IPXE_MENU_TEMPLATEon line 1070, then checks both independently in the subsequent loops. Since equality is already established, one of them could be removed from the iteration arrays to eliminate redundant checks.♻️ Suggested consolidation
for (name, contents) in [ ("embedded iPXE script", STATIC_IPXE_MENU_TEMPLATE), ("iPXE branding header", BRANDING_H), - ( - "renderer static menu template", - menu_template.template.as_str(), - ), ( "renderer static menu description", menu_template.description.as_str(), ), ] { assert!(contents.contains("NICo"), "{name} should mention NICo"); } for (name, contents) in [ ("embedded iPXE script", STATIC_IPXE_MENU_TEMPLATE), ("iPXE branding header", BRANDING_H), - ( - "renderer static menu template", - menu_template.template.as_str(), - ), ( "renderer static menu description", menu_template.description.as_str(), ), ] {🤖 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/ipxe-renderer/src/lib.rs` around lines 1061 - 1108, The test_static_ipxe_menu_uses_nico_branding function establishes equality between menu_template.template and STATIC_IPXE_MENU_TEMPLATE with an assert_eq call, then redundantly checks both strings independently in the subsequent iteration loops. Since you have already verified they are equal, remove one of these duplicate entries from the iteration arrays in both the NICo assertion loop and the Carbide/Forge assertion loop. You can either remove the "embedded iPXE script" entry that uses STATIC_IPXE_MENU_TEMPLATE or the "renderer static menu template" entry that uses menu_template.template.as_str() since they represent the same content.
🤖 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/ipxe-renderer/src/lib.rs`:
- Around line 1061-1108: The test_static_ipxe_menu_uses_nico_branding function
establishes equality between menu_template.template and
STATIC_IPXE_MENU_TEMPLATE with an assert_eq call, then redundantly checks both
strings independently in the subsequent iteration loops. Since you have already
verified they are equal, remove one of these duplicate entries from the
iteration arrays in both the NICo assertion loop and the Carbide/Forge assertion
loop. You can either remove the "embedded iPXE script" entry that uses
STATIC_IPXE_MENU_TEMPLATE or the "renderer static menu template" entry that uses
menu_template.template.as_str() since they represent the same content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f94c8cea-19de-433b-bba3-778048b7fc24
📒 Files selected for processing (4)
crates/ipxe-renderer/src/lib.rscrates/ipxe-renderer/templates.yamlpxe/ipxe/local/branding.hpxe/ipxe/local/embed.ipxe
✅ Files skipped from review due to trivial changes (1)
- pxe/ipxe/local/branding.h
🚧 Files skipped from review as they are similar to previous changes (1)
- pxe/ipxe/local/embed.ipxe
osu
force-pushed
the
fix/ipxe-nico-branding
branch
from
8235236 to
730eae8
Compare
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/dhcp/tests/booturl.rs (1)
31-60: 💤 Low valueDocument the retransmit-on-timeout behavior.
The function intentionally retransmits the DHCP request after each socket timeout, which aligns with standard DHCP/UDP retry semantics and addresses Kea startup timing issues. However, this retry-with-retransmit strategy is not immediately apparent from the code structure. A brief comment would aid future maintainers in understanding this is deliberate behavior rather than an oversight.
📝 Suggested documentation
fn recv_offer(socket: &UdpSocket, request: &[u8]) -> Result<v4::Message, eyre::Report> { let deadline = Instant::now() + OFFER_TIMEOUT; let mut recv_buf = [0u8; 1500]; // packet is 470 bytes, but allow for full MTU + // Retry loop: retransmit the DHCP request on each timeout to handle + // Kea startup timing and UDP packet loss, per standard DHCP retry behavior. loop { socket.send(request)?;🤖 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/booturl.rs` around lines 31 - 60, The retry-with-retransmit behavior in the recv_offer function is not documented, making it unclear whether the loop that calls socket.send(request) before each recv attempt is intentional. Add a comment at the start of the loop or before the socket.send call to document that the function deliberately retransmits the DHCP request on each timeout as part of standard DHCP/UDP retry semantics to address Kea startup timing issues, so future maintainers understand this is deliberate behavior rather than an oversight.
🤖 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/dhcp/tests/booturl.rs`:
- Around line 31-60: The retry-with-retransmit behavior in the recv_offer
function is not documented, making it unclear whether the loop that calls
socket.send(request) before each recv attempt is intentional. Add a comment at
the start of the loop or before the socket.send call to document that the
function deliberately retransmits the DHCP request on each timeout as part of
standard DHCP/UDP retry semantics to address Kea startup timing issues, so
future maintainers understand this is deliberate behavior rather than an
oversight.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aedaaaf4-58ad-47d4-9957-2a64cb79aa87
📒 Files selected for processing (5)
crates/dhcp/tests/booturl.rscrates/ipxe-renderer/src/lib.rscrates/ipxe-renderer/templates.yamlpxe/ipxe/local/branding.hpxe/ipxe/local/embed.ipxe
🚧 Files skipped from review as they are similar to previous changes (4)
- pxe/ipxe/local/branding.h
- pxe/ipxe/local/embed.ipxe
- crates/ipxe-renderer/src/lib.rs
- crates/ipxe-renderer/templates.yaml
Signed-off-by: Hasan Khan <hasank@nvidia.com>
osu
force-pushed
the
fix/ipxe-nico-branding
branch
from
730eae8 to
29e1411
Compare
osu
added
the
bug
ajf
approved these changes
Jun 15, 2026
| :carbide | ||
| :nico | ||
| ifconf -c dhcp | ||
| time chain --autofree http://${next-server}:8080/api/v0/pxe/boot?buildarch=${buildarch}&platform=${platform}&manufacturer=${manufacturer}&product=${product}&serial=${serial} && exit 1 || goto error_handler |
| @@ -1,11 +1,11 @@ | |||
| #undef PRODUCT_NAME | |||
| #define PRODUCT_NAME "Carbide" | |||
| #define PRODUCT_NAME "NICo" | |||
Collaborator
There was a problem hiding this comment.
Let's make this "NVIDIA Infra Controller". This is the actual full name of NICo.
| } | ||
|
|
||
| #[test] | ||
| fn test_static_ipxe_menu_uses_nico_branding() { |
Collaborator
There was a problem hiding this comment.
This test is pretty funny, I love it.
wminckler
approved these changes
Jun 15, 2026
|
🌿 Preview your docs: https://nvidia-preview-pull-request-2583.docs.buildwithfern.com/infra-controller |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2541
Verification
Note: the host PATH still does not provide cargo/cargo-make/rustfmt; the focused Rust validation above was run in Docker against an isolated copy.