-
Notifications
You must be signed in to change notification settings - Fork 268
fix: display actual CNAME target in -cname output #906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
WalkthroughUpdated Runner.outputRecordType to adjust the silent-output path for CNAME records: it now prints "domain -> target" (with target lowercased) before details. Non-CNAME records retain the prior "domain plus details" format. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant R as Runner
participant O as outputRecordType
participant S as Stdout
U->>R: run with -silent [-cname]
R->>O: format(recordType, domain, items, details)
alt recordType == CNAME and silent
O->>S: "domain -> target" + details
else other record types or modes
O->>S: "domain" + details
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal). Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/runner/runner.go (1)
825-831
: Nit: avoid leading/trailing spacing in details construction.Current concatenation can yield superfluous spaces when only ASN is present. Minor cleanup:
Apply this diff:
- var details string - if cdnName != "" { - details = fmt.Sprintf(" [%s]", cdnName) - } - if asn != nil { - details = fmt.Sprintf("%s %s", details, asn.String()) - } + details := "" + parts := make([]string, 0, 2) + if cdnName != "" { + parts = append(parts, fmt.Sprintf("[%s]", cdnName)) + } + if asn != nil { + parts = append(parts, asn.String()) + } + if len(parts) > 0 { + details = " " + strings.Join(parts, " ") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/runner/runner.go
(1 hunks)
🔇 Additional comments (2)
internal/runner/runner.go (2)
857-857
: Confirm intent: only first CNAME printed in silent mode.Due to
break
, if multiple CNAMEs are present (rare but possible in responses), only the first target is shown. If that’s desired, we’re good; otherwise consider iterating all and printing one line per target.
850-856
: Fix surfaces actual CNAME target in silent output — LGTM.Sandbox lacks dnsx (dnsx: command not found); run the supplied script locally to verify -silent, -resp and -resp-only outputs.
File: internal/runner/runner.go (lines 850–856).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oxqnd The current behavior is how the tool is supposed to work, it just prints out the original domain name if it has the specific record type. In order to retrieve the record, it's necessary to use the -resp
flag
Fixes #905
Problem
The
-cname
flag only printed the input domain instead of the actual CNAME record target.Example:
This was inconsistent with the expected behavior (e.g.,
dig cname
), making it impossible to retrieve the true CNAME target withdnsx
.Root Cause
In
outputRecordType()
, silent mode handling stopped after printing only the domain name.CNAME record values (
dnsData.CNAME
) were discarded in this branch.Changes
Updated
outputRecordType
logic:CNAME
, print both the queried domain and the resolved target.domain -> target
Added inline comment for clarity.
Result
Before:
After:
Summary by CodeRabbit