feat: datumctl dns plugin for zone and record management - #102
Merged
Conversation
scotwells
force-pushed
the
feat/datumctl-dns-plugin
branch
2 times, most recently
from
August 22, 2026 19:46
6af49f9 to
b3af211
Compare
scotwells
force-pushed
the
feat/datumctl-dns-plugin
branch
from
August 22, 2026 22:06
b3af211 to
e60b8c9
Compare
scotwells
force-pushed
the
chore/controller-runtime-v0.23
branch
from
August 22, 2026 22:25
49cb35d to
b124cea
Compare
scotwells
force-pushed
the
feat/datumctl-dns-plugin
branch
4 times, most recently
from
August 22, 2026 23:05
ef0201e to
bb7b93e
Compare
scotwells
force-pushed
the
chore/controller-runtime-v0.23
branch
from
August 22, 2026 23:54
b124cea to
7ad4963
Compare
scotwells
force-pushed
the
feat/datumctl-dns-plugin
branch
from
August 22, 2026 23:54
bb7b93e to
3e68d3b
Compare
scotwells
force-pushed
the
chore/controller-runtime-v0.23
branch
from
August 23, 2026 15:35
7ad4963 to
5dc40f6
Compare
scotwells
force-pushed
the
feat/datumctl-dns-plugin
branch
from
August 23, 2026 15:35
3e68d3b to
7b84e62
Compare
scotwells
force-pushed
the
chore/controller-runtime-v0.23
branch
from
August 24, 2026 15:48
5dc40f6 to
7c009c9
Compare
Adds a first-party `datumctl dns` plugin so developers can manage DNS zones and records from the terminal without writing YAML or knowing that DNSRecordSet exists. The API stores one DNSRecordSet per (zone, record type), so `www`, `api`, and `@` all share an object. The CLI presents flat records instead, flattening on read and reconstructing on write with a resourceVersion precondition. Records are entered with named fields for structured types and positionally for flat ones, and zone-file presentation format is accepted everywhere so values pasted from a provider export or a docs page work without translation. Client-side validation is load-bearing rather than cosmetic: the API server does not check that the typed rdata field matches spec.recordType, and a mismatched entry is admitted and then converted by the backend into a delete of the existing correct RRset. Every write path validates before submitting. Zone import refuses a zone file's apex NS and SOA records and reports the skip with its reason, because importing a provider export's nameserver records would break delegation. Delegation state distinguishes "the registrar points elsewhere" from "we have not looked yet" and only offers registrar instructions for the former. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI enforces errcheck, prealloc, goconst, unparam, unused and staticcheck, and the plugin was written without running it. Mostly mechanical, with three decisions worth recording. The unused `svcbQuotedKeys` in rdata was not dead by accident. internal/pdns keeps the same map, but its quoted-keys branch and its default branch both call quoteIfNeeded, so the two are equivalent and rdata collapsed them — correctly. Removing the map rather than wiring it in keeps Render byte-identical to the backend, and the comment above it no longer claims a mirror that is not there. Bare `fmt.Fprint*` calls in the plugin's output paths now discard explicitly with `_, _ =`, matching the operator's existing style. Writes to a command's own stdout have no recoverable failure mode, and the explicit discard says so rather than leaving it ambiguous. goconst is excluded for test files. Adopting constants in table-driven tests actively hurt them: asserting `testOwner` instead of "www" hides what a case is checking, the repeated literals are the fixtures themselves, and the mechanical substitution silently corrupted three suites by rewriting literals nested inside expected-output strings. The two goconst findings in non-test code are fixed properly, with named constants for the example address and the em dash used for an unknowable cell. The rest: pre-allocated slices where the length is known, dropped two unused return values from test helpers, explicit discards for a discarded error return, and De Morgan on a negated character-class check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…fits A TTL column printed `5` and left the reader to guess whether that meant five seconds, five minutes or five hours. Nothing in the table said which, and the answer matters most exactly when the number is small — a 5s record and a 5m record behave very differently under a change. Every rendered TTL now carries its unit, and picks the largest unit that divides evenly: 300 shows as `5m`, 3600 as `1h`, 86400 as `1d`, 604800 as `1w`. A value that divides evenly into nothing larger stays in seconds (`90s`) rather than becoming a compound like `1m30s` — one unit per cell is easier to scan down a column, and a TTL's meaning is the exact number of seconds. `Auto` still reads as `Auto`, and describe spells it out as `Auto (5m)`. `ParseTTL` gained the same vocabulary so the display round-trips: `d` and `w` are now accepted alongside `s`, `m` and `h`, single or compound, which means a TTL read off `record list` can be pasted straight back into `--ttl`. TestFormatTTLRoundTrips asserts that property across the range rather than on a handful of examples. Go's time.ParseDuration knows nothing of `d` or `w`, so the units are parsed here; the two errors worth distinguishing are kept — a leading sign still earns "is negative" and a fractional value still earns "is not a whole number of seconds", instead of both collapsing into a generic parse failure. Zone files are deliberately untouched: `zone export` still writes `$TTL 300` and bare per-record seconds, because that is a machine format read back by `record apply`, not something anyone scans.
Parsing the units by hand meant "1.5h" started failing with "is not a whole number of seconds", which is both a regression and untrue — 1.5h is 5400 whole seconds, and time.ParseDuration accepted it before. A value with a decimal point now goes to time.ParseDuration and is only rejected when it genuinely lands between seconds, so "1.5h" parses and "1.5s" still does not. The TTL table now covers d, w, uppercase, compound and fractional spellings rather than the three Go durations it started with.
scotwells
force-pushed
the
feat/datumctl-dns-plugin
branch
from
August 24, 2026 15:48
5e27241 to
35e5f27
Compare
The guide had grown into a design document: it explained internals, defended decisions, compared itself to the web portal, and litigated edge cases. Someone who wants to add an A record had to read past all of it. Rewritten against the Google developer documentation style guide — task-based headings in the second person, steps before explanation, tables and lists in place of prose, and callouts kept for the few limitations that actually change what a reader should do. 680 lines to 369. Install is now documented as the catalog flow it will ship as: `datumctl plugin install dns`, with search, list, upgrade and remove. The build-and-trust path is contributor workflow and does not belong in product docs. Every console block is checked against the code that emits it rather than against what looks plausible: the install line against datumctl's install.go, the nameservers block against the order nameservers.go actually prints (the list comes before the Delegation summary), the export block against writeGroup's hand-padded columns, and the record list and apply diffs re-rendered through the same tabwriter configuration the commands use, so the column alignment is real. TTL columns carry units throughout, while zone-file examples keep bare seconds, which is what that format uses. Also corrects two claims: enabling DNS is not immediate — approval can be a manual step by the service provider — and the record status filter's canonical value is `not-owner`.
Three pieces of the plugin were reimplementing something a dependency already provides, and one duplicated another part of the plugin. `util.FindCondition` is `apimeta.FindStatusCondition`, which this repo's own replicator controller already calls. Gone, along with its test — testing upstream's function here proved nothing. `util.RelativeAge` is `duration.HumanDuration`, the formatter behind kubectl's AGE column. Adopting it means an age read out of `zone list` means the same thing as an age read out of `kubectl get`, and it is strictly more informative below ten minutes, where the hand-rolled version rounded the seconds away. The one visible change is at the minute boundary: 60s now reads "60s" rather than "1m", because upstream stays in seconds until two minutes. The em-dash guard for a never-transitioned timestamp stays, since HumanDuration has no notion of "never" and would render the epoch as half a century. `bind` carried its own TTL parser with the same s/m/h/d/w grammar as `rdata`, a duplication introduced when rdata's parser grew the units. bind now calls `rdata.ParseTTLSeconds`, so the grammar has one definition and a value accepted by `--ttl` is accepted in a zone file. bind keeps its own wording, which needed telling "not a TTL" from "a TTL out of range" — hence `rdata.ErrTTLRange`, carried inside `rdata.Error` via Unwrap so `errors.Is` finds it while the sentinel never appears in the text a user reads and the "Fix:" line still resolves. Deliberately not replaced: `rdata.FQDN` and `rdata.qualify` test for a trailing dot with a raw suffix check rather than `dns.IsFqdn`. miekg's version is escape-aware and therefore more correct in the abstract, but internal/pdns qualifies on a bare last-byte comparison, and these two functions exist to mirror it exactly. Adopting the better test would make the CLI and the backend disagree about a name ending in an escaped dot. `ParseRRType` likewise stays scoped to the API's enum rather than `dns.StringToType`, which would accept types the API cannot store.
The content was sound but the shape fought the reader. Rationale — which is the point of an enhancement doc and stays — was buried in five-paragraph callouts, bold pseudo-headings stood in for real ones so nothing was linkable, and several sentences ran past sixty words with two em-dash asides apiece. Reworked to the Google developer documentation style guide: sentence-case headings that say what the section is about, active voice and second person, shorter sentences, lists and tables where prose was carrying a set of parallel items, and real headings in place of bold lead-ins. The long callouts became sections with a one-line callout at the top where the danger is genuine, since a warning that runs five paragraphs stops reading as a warning. Nothing was cut. Every code reference, RFC citation, incident, and bound is still here — verified by diffing the occurrence counts of forty key tokens before and after, where the only changes are repeated mentions collapsed to one. The nine internal links all resolve against the new headings. Line count rises from 684 to 810, entirely from splitting walls of text into navigable sections.
goimports wants the module-local import in its own block. Caught by running golangci-lint at the version CI pins, v2.4.0, rather than the newer one installed locally.
The plugin handles entitlement itself — it offers to enable DNS on first use at a terminal — so a three-step ritual before the first command was ceremony the reader does not need. Install then use. Both facts the section carried are still reachable from Troubleshoot, where someone actually hits them: how to set a project, and how to enable DNS non-interactively.
scotwells
marked this pull request as ready for review
August 24, 2026 19:08
A QA sweep drove the plugin the way a new user would. Six things got in the way; none was a correctness bug, and all six were places the tool knew the answer and did not say it. Arity errors were the worst of them, and they are the first thing anyone sees. Cobra's own text names a count and nothing else — "accepts 1 arg(s), received 0" — and because SilenceUsage suppresses the usage block, that line was the entire output. It never said the missing argument was the domain, and `record describe` managed "accepts between 2 and 3 arg(s)". The command already knows its own shape, so asUsageError now reads the argument list off the Use line and the example off the command's own Example block: "datumctl dns record list takes <domain>" with a Fix of "datumctl dns record list example.com". Cobra's text is kept as the cause, reachable under --verbose. The contrast with rdata validation, which has always answered "not a valid IPv4 address" with a worked example, is what made this worth fixing. Typo suggestions fired only when the typo happened to be a prefix: "desc" found "describe", "lst" never found "list". Cobra defaults SuggestionsMinimumDistance on the root alone and along its own unknown-command path, so every subcommand sat at 0 where the Levenshtein test can never pass. The tree walk that already relabels exit codes now sets it. `record list` printed the empty-zone onboarding block whenever a filter matched nothing, so filtering a zone of a thousand records and missing told the reader to import a zone file. The headline and the advice turn on different questions now — whether filters were given, and whether they are what emptied the result — so a zone with nothing in it still gets the type-aware "Get started" that an earlier test pinned. `zone describe` on a rejected zone offered "Add a record", which is an instruction to do something that will not take effect, at the exact moment the reader is trying to work out what is wrong. Rejected and Error now say the zone is not serving changes and point at the status line and at export. `zone export` gained two warnings on stderr, so the file stays clean when redirected. ALIAS is a provider extension rather than an IANA type, so a standard parser rejects the line and takes the whole file with it — a user exporting to leave Datum, or running named-checkzone before a cutover, hits that. And a zone file has nowhere to record who owns a record, so machine-managed ones import elsewhere as ordinary records for a controller to contend with; on a real staging zone that was 78 of 87. Both are documented in the CLI guide. Each fix has a test, and each test was verified by anti-patching: reverting the suggestion distance, the arity message, the filter advice, the rejected-zone branch and the export warnings each fails its own test and nothing else. All five compiled first, so none of those results is a build error in disguise.
savme
approved these changes
Aug 25, 2026
This was referenced Aug 25, 2026
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
2 participants
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.
Adds a first-party
datumctl dnsplugin so developers can manage DNS zones and records from the terminal — no YAML, and no Kubernetes concepts.One row per record, with live status:
Simple types take their value positionally; multi-part types take named flags; zone-file format works everywhere, so a value pasted from a provider export or an "add this TXT record" page needs no translation.
datumctl dns record create example.com www A 203.0.113.10 datumctl dns record create example.com @ MX --preference 10 --exchange mail.example.com. datumctl dns record create example.com _sip._tcp SRV "10 5 5060 sipserver.example.com."createadds a value at a name;setreplaces everything there.zone import --filereads a provider's zone export, and--discoversnapshots what a domain resolves to today. Apex NS and SOA records are skipped with a reason rather than imported, since importing them breaks delegation.zone nameserversreports whether the registrar is pointed at Datum, and distinguishes that from not having checked yet. It prints registrar instructions only when they apply.For scripting: a stable exit code per failure class, a fix line on every error,
--dry-runon every write, and-o json/-o name.User guide:
docs/cli/datumctl-dns.md