chore: clean up additional internal references - #5552
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@coderabbitai full_review, thanks! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (9)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. Summary by CodeRabbit
WalkthroughThe change removes obsolete issue and Slack references, updates DHCP server author metadata, and reformats UFM errors for unsupported combined requests. Runtime request handling remains unchanged. ChangesReference and error-text cleanup
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This PR removes inaccessible internal references and adjusts related comments and error wording without changing runtime behavior; the managed cron file may be rewritten once but does not execute. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
🐇 ✅ Action performedFull review finished. |
chet
changed the title
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
|
@coderabbitai full_review, thanks! |
|
🐇🔍 ✅ Action performedFull review finished. |
We're a public repo, but a handful of comments still point at NVIDIA-internal bug trackers, an internal Jira, and a Slack thread. Anyone reading this from outside NVIDIA hits a link they can't open. One was worse than a comment: it sat inside an error message the InfiniBand code hands back to its caller, so the internal URL went out with the error. Deleting those links would lose the trail, so where a tracked issue still had a story worth keeping, I read it, wrote the underlying problem up without the internal detail, and filed it here as a closed issue the comment can point at instead. That's NVIDIA#5555, NVIDIA#5556, NVIDIA#5557, and NVIDIA#5558 for the bug-tracker links, and NVIDIA#5559 for the Jira one. Two had nothing left to preserve -- one Jira issue no longer exists, and a Slack permalink was never more than a pointer -- so those comments just lose the link and keep the explanation they already had. A few notes: - The InfiniBand error now names UFM as the thing that won't take both `qos_conf` and `guids_data` in one request. The link moved to the comment, where a caller reading the message doesn't have to follow it. - `forge-dhcp-server` had an internal Slack channel as its clap `author`. Nothing actually renders that value -- clap 4 only emits `author` through a help template or `clap_mangen`, and this binary has neither -- so it's inert metadata rather than anything an operator sees. It now matches the public repo URL `nico-admin-cli` uses. - `apt-clean` ships through duppet, which compares content by hash, so **every managed DPU rewrites `/etc/cron.daily/apt-clean` once** and reports it updated. There's no `with_exec_on_change()` on that entry, so nothing runs and the cron behavior is identical. This is deliberately narrow. Internal hosts that stand in for real endpoints in test fixtures and sample config stay as they are, as does the build comment on the `dev/docker` images, and `.forge` in-cluster names and the `forge` product name were never internal references to begin with. Existing tests across the touched crates still pass; comment changes didn't need new ones. This supports NVIDIA#5414 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
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. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-5552.docs.buildwithfern.com/infra-controller |
|
🐇🔍 ✅ Action performedFull review finished. |
2 participants
We're a public repo, but a handful of comments still point at NVIDIA-internal bug trackers, an internal Jira, and a Slack thread. Anyone reading this from outside NVIDIA hits a link they can't open. One was worse than a comment: it sat inside an error message the InfiniBand code hands back to its caller, so the internal URL went out with the error.
Deleting those links would lose the trail, so where a tracked issue still had a story worth keeping, I read it, wrote the underlying problem up without the internal detail, and filed it here as a closed issue the comment can point at instead. That's #5555, #5556, #5557, and #5558 for the bug-tracker links, and #5559 for the Jira one. Two had nothing left to preserve -- one Jira issue no longer exists, and a Slack permalink was never more than a pointer -- so those comments just lose the link and keep the explanation they already had.
#5353 covers the
gitlab-masterreferences; this is a narrow follow-on.A few notes:
qos_confandguids_datain one request. The link moved to the comment, where a caller reading the message doesn't have to follow it.forge-dhcp-serverhad an internal Slack channel as its clapauthor. Nothing renders that value on this binary -- clap 4 only emitsauthorthrough a help template orclap_mangen, and it has neither -- so it's inert metadata rather than anything an operator sees. It now matches the public repo URLnico-admin-cliuses.apt-cleanships through duppet, which compares content by hash, so every managed DPU rewrites/etc/cron.daily/apt-cleanonce and reports it updated. There's nowith_exec_on_change()on that entry, so nothing runs and the cron behavior is identical.This is deliberately narrow. Internal hosts that stand in for real endpoints in test fixtures and sample config stay as they are, as does the build comment on the
dev/dockerimages, and.forgein-cluster names and theforgeproduct name were never internal references to begin with.Related issues
This supports #5414
Type of Change
Breaking Changes
Testing
Unit tests added/updated
Integration tests added/updated
Manual testing performed
No testing required (docs, internal refactor, etc.)
Full lint gate plus the existing suites for the touched crates; comment changes needed no new tests.
Ran
forge-dhcp-server --helpto confirm clap does not render theauthorvalue on this binary.Review Findings
Model Findings Overview
All four local reviewers completely covered the same stable pre-fix tree. The counts below cover the review of the wider original change; the owner then narrowed its scope, which resolved several findings by removing the code they described.
Model Findings Details
Claude self-review
CodeRabbit CLI
IbErrorandUFMErroralready supply a lowercase prefix, so the rendered text was mixed.Codex CLI
No findings.
common-nits-reviewer
authorvalue is never rendered on this binary, so describing it as a--helpchange was wrong. Adopted: verified against the real binary and corrected the wording.GetPartitionOptions, add a null check inmachine_get_interface_mtu, drop the stale/etc/rc.localentry, and restore an investigation trail to theupgrade.rsandmtu.rsworkarounds. Declined as four separate items: each is pre-existing and lives outside this change.## Designheld no design content once its only sentence was removed, andrumdlreported two findings on that README. Both adopted at the time; declined in the end, since that file is no longer part of this change.Closes #5414