Skip to content

Conversation

oxqnd
Copy link

@oxqnd oxqnd commented Sep 17, 2025

Fixes #905

Problem

The -cname flag only printed the input domain instead of the actual CNAME record target.
Example:

$ echo jfkt.github.shopify.com | dnsx -silent -cname
jfkt.github.shopify.com

This was inconsistent with the expected behavior (e.g., dig cname), making it impossible to retrieve the true CNAME target with dnsx.

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:

    • For CNAME, print both the queried domain and the resolved target.
    • Example format: domain -> target
  • Added inline comment for clarity.

Result

Before:

$ echo jfkt.github.shopify.com | dnsx -silent -cname
jfkt.github.shopify.com

$ echo eu2.deployment.api.varonis.io | dnsx -silent -cname
eu2.deployment.api.varonis.io

After:

$ echo jfkt.github.shopify.com | ./dnsx -silent -cname
jfkt.github.shopify.com -> github.com.

$ echo eu2.deployment.api.varonis.io | ./dnsx -silent -cname
eu2.deployment.api.varonis.io -> eastus2-prd-distributed-deployment-ep.azureedge.net.

Summary by CodeRabbit

  • New Features
    • Improved CNAME result formatting in silent output mode: now displays "domain -> target" followed by details for clearer readability.
    • All other DNS record types retain existing formatting.
    • No changes to command options or public APIs.

Copy link

coderabbitai bot commented Sep 17, 2025

Walkthrough

Updated 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

Cohort / File(s) Summary
Runner output formatting
internal/runner/runner.go
Adjust silent mode formatting: for CNAME records, print "domain -> target" followed by details; other record types unchanged. No API/signature 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
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my whiskers, small and bright,
CNAMEs now hop into the light—
Domain -> target, clear in view,
No more shadows hiding who.
Thump-thump! I stamp in pure delight,
A tidy trail of records right.
(_/)\u2003(•‿•)

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title succinctly and accurately summarizes the primary change: it fixes the -cname silent-mode output to display the actual CNAME target. This directly corresponds to the PR description and the linked issue and is clear and specific for changelog/history scanning.
Linked Issues Check ✅ Passed The changes address issue #905 by updating outputRecordType so the silent branch includes the CNAME target (printing "domain -> target") using dnsData.CNAME, which matches the reported bug and the examples in the PR summary; no public API signatures were modified and the behavior aligns with the expected dig-like output. The raw_summary and PR objectives both describe this exact fix and examples demonstrating the corrected output.
Out of Scope Changes Check ✅ Passed Based on the provided raw_summary and PR objectives, the only code change is within internal/runner/runner.go adjusting silent-mode CNAME formatting; no other files or exported signatures were changed, so there are no apparent out-of-scope modifications. The change is narrowly scoped to the reported issue.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 92dbe0b and 936bbbf.

📒 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).

Copy link
Member

@Mzack9999 Mzack9999 left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem in '-cname' argument
2 participants