fix: DNS record RRset display/deletion, egress create reporting, command reference validation#28
Conversation
…and reference validation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR adds PowerDNS record-set support (new ChangesDNS Record-Set Support
Egress Rule Creation Retry
Documentation and Release Notes
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/api/egress/egress.go (2)
154-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider wrapping
ctx.Err()for consistency.Every other error path in this function uses
fmt.Errorfwith context (network slug, operation). Returningctx.Err()bare loses that context when the caller needs to distinguish it from other egress errors.🤖 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 `@pkg/api/egress/egress.go` around lines 154 - 155, The ctx.Done() return in egress behavior should match the other error paths by preserving operation context instead of returning ctx.Err() directly. Update the ctx.Done() branch in the egress flow to wrap ctx.Err() with fmt.Errorf, including the same identifying details used elsewhere in this function such as the network slug and current operation so callers can distinguish cancellation from other egress failures.
152-158: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
time.Afterinside the retry loop can leak a timer ifctxcancels first.Since the timer is bounded to at most 2 iterations, the practical leak impact is negligible here, but
time.NewTimer+Stop()is the idiomatic pattern forselectinside loops.🤖 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 `@pkg/api/egress/egress.go` around lines 152 - 158, The retry wait in egress list attempts currently uses time.After inside the loop, which can leave an unneeded timer alive when ctx.Done() wins the select. Update the retry branch in the egress listing logic to use time.NewTimer with a deferred/conditional Stop and drain pattern, keeping the same 2-second delay behavior while ensuring the timer is cleaned up properly. Focus on the select inside the listAttempts loop in the egress retrieval flow.internal/commands/dns.go (1)
339-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider marking
--record-iddeprecated via Cobra.The flag is documented as legacy in its help text, but Cobra's
MarkDeprecatedwould surface a runtime warning automatically instead of relying on users reading--help.♻️ Suggested tweak
cmd.Flags().IntVar(&recordID, "record-id", 0, "Legacy: numeric record ID (most deployments do not expose IDs)") + _ = cmd.Flags().MarkDeprecated("record-id", "most deployments no longer expose record IDs; use --name/--type instead") cmd.Flags().BoolVar(&yes, "yes", false, "Skip confirmation prompt")🤖 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 `@internal/commands/dns.go` around lines 339 - 370, Mark the legacy numeric identifier flag in newDNSRecordDeleteCmd as deprecated using Cobra’s deprecation support so users get a runtime warning automatically; keep the existing runDNSRecordDelete legacy path for compatibility, but update the --record-id flag registration to surface the deprecation notice instead of relying only on the help text.
🤖 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 `@pkg/api/egress/egress.go`:
- Around line 140-151: The fallback rule match in the egress lookup is too broad
for ICMP traffic and can return the wrong rule when ICMPType or ICMPCode differ.
Update the matching logic in the egress rule scan (the loop in the egress API
lookup) so that, when req.Protocol indicates ICMP, it also compares the ICMPType
and ICMPCode fields in addition to protocol and CIDR, while keeping the existing
port-based behavior for non-ICMP protocols.
In `@RELEASE_NOTES.md`:
- Around line 5-10: The release note paragraph is too broad about record IDs and
should be scoped to the PowerDNS backend only. Update the wording in
RELEASE_NOTES so the no-ID and contents-array behavior is clearly tied to
PowerDNS, while leaving the legacy record-id path intact for backends that still
expose IDs; keep the phrasing consistent with the later notes about `dns
record-delete` and record tables.
---
Nitpick comments:
In `@internal/commands/dns.go`:
- Around line 339-370: Mark the legacy numeric identifier flag in
newDNSRecordDeleteCmd as deprecated using Cobra’s deprecation support so users
get a runtime warning automatically; keep the existing runDNSRecordDelete legacy
path for compatibility, but update the --record-id flag registration to surface
the deprecation notice instead of relying only on the help text.
In `@pkg/api/egress/egress.go`:
- Around line 154-155: The ctx.Done() return in egress behavior should match the
other error paths by preserving operation context instead of returning ctx.Err()
directly. Update the ctx.Done() branch in the egress flow to wrap ctx.Err() with
fmt.Errorf, including the same identifying details used elsewhere in this
function such as the network slug and current operation so callers can
distinguish cancellation from other egress failures.
- Around line 152-158: The retry wait in egress list attempts currently uses
time.After inside the loop, which can leave an unneeded timer alive when
ctx.Done() wins the select. Update the retry branch in the egress listing logic
to use time.NewTimer with a deferred/conditional Stop and drain pattern, keeping
the same 2-second delay behavior while ensuring the timer is cleaned up
properly. Focus on the select inside the listAttempts loop in the egress
retrieval flow.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f2c1aa3-a391-491d-b28b-475c0d9dfbed
📒 Files selected for processing (8)
CHANGELOG.mdRELEASE_NOTES.mddocs/commands.mddocs/development/RELEASE.mdinternal/commands/dns.gopkg/api/dns/dns.gopkg/api/dns/dns_test.gopkg/api/egress/egress.go
…gress rule reporting
…es for DNS record handling
…pt for proper checkout handling on Windows
1 participant
Summary by CodeRabbit
New Features
Bug Fixes