BUG: Fix get-zones argument parsing to match documentation#4240
BUG: Fix get-zones argument parsing to match documentation#4240cafferata wants to merge 5 commits into
Conversation
TomOnTime
left a comment
There was a problem hiding this comment.
Thanks for fixing this and cleaning how it works!
TomOnTime
left a comment
There was a problem hiding this comment.
nitpick: Previously the code allowed a list of zone names. That's why the -h listing includes [...] at the end of the usage spec:
USAGE:
dnscontrol get-zones [command options] credkey zone [...]
I'm ok with fixing the code or simply updating the docs. I don't know many times when listing multiple zones is needed.
Related: (mostly thinking out loud. These don't have to be part of this PR). I wonder if we should drop the all and just allow an empty list to mean all zones. I've also thought that we should create list-zones as an alias for get-zones --format=nameonly credkey all
…ut requiring provider name. - The provider type is resolved from the `TYPE` field in `creds.json` via `beCompatible()`. This matches the documented v4.0 behavior and fixes DNSControl#4235.
- Move format details, TSV column descriptions, and migration notes to https://docs.dnscontrol.org/commands/get-zones instead of duplicating in the CLI. - Add `Documentation:` link to `check-creds` CLI help.
- Replace terse names like `myr53`/`cfmain` with `my_route53`/`my_cloudflare` so the examples are self-explanatory without the provider name argument.
- Remove the `provider` argument from syntax, replace v3.16/v4.0 migration sections with a single deprecation note, use descriptive `credkey` names in examples, and fix typos (`example.comn`, `glcoud`, `get-zone`).
- With 3+ arguments, detect whether the second argument is a registered provider type to distinguish the deprecated form (`credkey provider zone...`) from the new multi-zone form (`credkey zone1 zone2...`). - Previously, `dnscontrol get-zones my_route53 example.com other.com` incorrectly treated `example.com` as a provider name.
|
Thanks for the approval and feedback! I'll address each point: Multi-zone support: Good catch. I hadn't thought about this scenario (I've never used the multi-zone form myself). Since it's a small fix, I went ahead and implemented it right away by checking whether the second argument is a registered provider type. If it is: deprecated path with warning. If not: new format with multiple zones. This should cover all remaining edge cases around
Removing the v3.16 migration notes from the docs: While cleaning up the documentation I kept the "As of v3.16" section as-is, but I think we can safely remove it entirely. v3.16.0 was released over 4 years ago (May 2022). Anyone still on v3.15 or older is unlikely to consult the current docs for migration guidance. What do you think? |
c167879 to
a52c237
Compare
👍
👍
Agreed. |
I ran into this same issue while working on #4208 and can confirm the behavior @stefanwascoding describes. The only form that works requires the provider name explicitly:
But following that suggestion also fails:
So neither the documented 2 argument form (
transip all) nor the suggested dash form (transip - all) works.This pull request fixes the argument parsing so that
get-zonesaccepts 2 arguments (credkey zone), resolving the provider type from theTYPEfield increds.json. The old 3 argument form still works but now shows a deprecation warning with the correct command:With 3+ arguments, the code now detects whether the second argument is a registered provider type to distinguish the deprecated form (
credkey provider zone...) from the new multi-zone form (credkey zone1 zone2...):Additionally:
credkeynames (my_route53instead ofmyr53) so they are self-explanatory without the provider nameDocumentation:links are added toget-zonesandcheck-credsdocumentation/commands/get-zones.mdis updated to matchTested against the TransIP provider with the
dnscontrol.orgzone from #4204.go test ./commands/dnscontrol get-zones transip all(2 args, new style)dnscontrol get-zones transip dnscontrol.org(specific zone)dnscontrol get-zones transip dnscontrol.org dnscontrol.nl(multiple zones, new style)dnscontrol get-zones transip TRANSIP all(3 args, deprecation warning)dnscontrol get-zones transip(too few args, error message)dnscontrol get-zones --format=zone transip dnscontrol.orgdnscontrol get-zones --format=tsv transip dnscontrol.orgdnscontrol get-zones --format=nameonly transip allCLI diff
Fixes #4235