feat(dns): add --replace-conflicting-types flag, fix CNAME conflict errors#102
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the DNS command implementation into per-subcommand modules while updating the Domains v3 API surface (vendored spec + generated client expectations) and improving user-facing validation errors (notably CNAME/name-exclusivity conflicts) by supporting the v3 top-level details[] error shape. It also updates domain purchase to align with the refreshed spec by treating consent attribution (agreedBy) as server-derived and adding a local fail-fast check for customer-scoped OAuth identities.
Changes:
- Update
domain purchaseconsent handling: remove caller-suppliedagreedBy, add local customer-identity validation, and harden cached agreement-type parsing with a clear stale-cache error. - Make API error rendering aware of v3 top-level
details[]validation errors and reuse the formatter acrossdnsanddomain. - Split DNS functionality into
list/add/set/deletemodules with shared helpers and add improved name-exclusivity conflict diagnosis plus an opt-in auto-remediation flag fordns set.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/src/domain/purchase.rs | Align purchase execution with updated v3 consent model; fail-fast on non-customer OAuth subjects; validate cached agreement types via parsing. |
| rust/src/domain/mod.rs | Re-export format_api_error for cross-module reuse (e.g., DNS). |
| rust/src/domain/guides/domain-purchase.md | Remove documentation for the dropped --agreed-by flag. |
| rust/src/domain/common.rs | Export format_api_error and support v3 top-level details[] validation errors in friendly formatting. |
| rust/src/dns/mod.rs | Replace monolithic implementation with per-command modules + shared helpers. |
| rust/src/dns/records.rs | Centralize DNS arg parsing/validation, record construction, pagination, and verify-next-action helper. |
| rust/src/dns/conflicts.rs | Add structured detection/diagnosis for DUPLICATE_RECORD conflicts and render actionable explanations. |
| rust/src/dns/list.rs | Implement dns list using shared record fetching + type/name filters. |
| rust/src/dns/add.rs | Implement dns add with per-record outcomes and conflict-aware error rendering. |
| rust/src/dns/set.rs | Implement dns set reconcile plan, partial-failure reporting, and optional auto-delete+retry on cross-type conflicts. |
| rust/src/dns/delete.rs | Implement dns delete with per-record delete outcomes and summary error reporting. |
| rust/domains-client/src/lib.rs | Update generated-client-facing tests/fixtures to match new spec enums and agreement type changes. |
| rust/domains-client/openapi/swagger_domains.v3.yaml | Refresh vendored v3 spec to 3.1.0 with updated consent/agreement semantics and enums. |
| rust/domains-client/openapi/domains.oas3.json | Refresh merged OAS3 JSON to reflect the updated v3 schema/enum changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jpage-godaddy
added a commit
that referenced
this pull request
Jul 14, 2026
…e detail format Copilot review feedback on PR #102: - dns add's DUPLICATE_RECORD remediation suggested `dns set --replace-conflicting-types` without its required args; point back at the reconstructable `dns add` command instead. - dns set's partial-failure remediation suggested `gddy dns set` bare, which can't be reconstructed (missing --data); rephrase to "the original command". - dns set's plain-delete outcomes reported the raw record id while --replace-conflicting-types deletes reported "<TYPE> <DATA>", mixing identifiers in the same breakdown; both now use the type+data format. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
jpage-godaddy
changed the title
…flict errors Refresh the Domains v3 OpenAPI spec and use it to fix `dns set`/`dns add` surfacing an opaque error on a CNAME name-exclusivity conflict, plus a related fix in `domain purchase`. Also break up the ~1760-line dns/mod.rs into per-command files (list/add/set/delete) with shared helpers split into records.rs and conflicts.rs, mirroring the domain/ module's layout for easier navigation. No behavior change from the reorganization itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e detail format Copilot review feedback on PR #102: - dns add's DUPLICATE_RECORD remediation suggested `dns set --replace-conflicting-types` without its required args; point back at the reconstructable `dns add` command instead. - dns set's partial-failure remediation suggested `gddy dns set` bare, which can't be reconstructed (missing --data); rephrase to "the original command". - dns set's plain-delete outcomes reported the raw record id while --replace-conflicting-types deletes reported "<TYPE> <DATA>", mixing identifiers in the same breakdown; both now use the type+data format. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The DUPLICATE_RECORD remediation hint for a CNAME-vs-other-types conflict only named the first conflicting type's delete command, so a name with e.g. both A and MX records would leave the MX record behind after following the hint, and the re-run `dns add` would still fail. Also correct a doc comment claiming `conflicting_records_at`'s no-type-filter fetch matches what `dns list --name` does — `dns list` actually requires `--type` whenever `--name` is given, so this helper does something the CLI doesn't expose.
…ription
The v3 error-details schema documents description as unstable ("MAY
change... MUST NOT depend on this value") while issue is the stable
fine-grained code. friendly_field_errors dropped any detail with an
empty description, producing an opaque fallback message even when a
usable issue code was present.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The partial-failure hints told users to "re-run gddy dns add"/"gddy dns delete" without the required domain positional or --type/--name flags, so they couldn't be copy-pasted as-is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The "re-run gddy dns list" hint for a record returned without a recordId omitted the domain positional and --type/--name flags, same class of bug as the add/delete recovery hints fixed earlier. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ests - purchase.rs: name the discarded consent_principal() result and explain why only its validation (not the customer id) is needed here. - dns/set.rs: cover write_with_conflict_handling's DUPLICATE_RECORD branching — direct success, conflict without --replace-conflicting-types, and conflict-with-the-flag (delete_conflicts outcome then retry outcome, in order) — against a mocked domains-client server. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ement types A re-quote won't help when the API has introduced a genuinely new agreement type rather than the cache just being stale — that requires a newer CLI build with the updated generated AgreementType enum.
A name-exclusivity conflict can span multiple existing records (e.g. A + MX both blocking a CNAME write); "remove it automatically" implied just one, unlike the matching CLI help text which already says "conflicting record(s)".
jpage-godaddy
force-pushed
the
agreement-key
branch
from
July 20, 2026 17:09
f2682f7 to
ccde55f
Compare
Collaborator
Author
|
Disappointingly I've asked for this to be reviewed several times and it hasn't been. It's now a week old. Merging to block the outstanding ask. |
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
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
dns set/dns addsurfacing an opaque validation error on a CNAME name-exclusivity conflict, by refreshing the Domains v3 OpenAPI spec and teachingformat_api_errorto render the v3 top-leveldetails[]error shape.dns setalso gains an opt-in--replace-conflicting-typesflag that auto-removes the conflicting record(s) and retries the write, instead of only pointing at the manualdns deletecommand.domain purchase: validate the OAuth token's customer identity locally rather than relying on a caller-supplied consent principal, matching the refreshed spec (the register endpoint now derivesagreedByserver-side).dns/mod.rsinto per-command files (list.rs,add.rs,set.rs,delete.rs) with shared helpers inrecords.rsandconflicts.rs, mirroring thedomain/module's layout. Pure reorganization of this file — no behavior change from the split itself (the CNAME-conflict and--replace-conflicting-typesbehavior above are separate, intentional changes).gddy dns add/dns delete/dns listwithout the required domain positional or--type/--nameflags.Slack thread for context.
Test plan
cargo fmt --checkcargo build --workspacecargo test --workspace(263 passing)cargo clippy --workspace -- -D warningsdns set/dns addagainst a real domain with an existing CNAME conflict🤖 Generated with Claude Code