-
Notifications
You must be signed in to change notification settings - Fork 409
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
Request for Comments: Include the provider type in creds.json, remove it from dnsconfig.js #1457
Comments
For me, proposal C would fit best. |
I prefer A, because JSON shouldn't be imposing structure inside keys using colon-delimited fields; C is my second choice, B makes little sense to me. I can adapt to whatever. My preference for A is by quite a wide margin, because whenever I'm using JSON I end up doing ad-hoc queries with tools such as |
@philpennock That an interesting point about wanting to be able to easily manipulate creds.json using something like jq. Thoughts? |
Duplicate possibilities mean you have to detect both and handle both, or so it's more work and more fragility, with less testing that scenarios are fully handled. Pick one and work with it. |
I'm taking a look at how Plan A might be implemented. One thing I've noticed is that it might be more clear to specify "TYPE" instead of "PROVIDER". The code and other docs refer to that field as the |
I have a draft PR of "Plan A" in #1485 Feedback appreciated! |
Thanks to everyone for their feedback! I went with "Plan A" as described. It turns out Plan A wasn't as difficult to implement as I had hoped. Thanks to philpennock for pushing in that direction. |
Problem:
Many commands and functions require the user to specify the provider name (such as
ROUTE53
) and the cred-name. This is redundant, error prone, and additional cognitive load on the user.Proposed solution: Store the provider name in the individual
creds.json
entries themselves; thus removing the need to specify them in dnsconfig.js or on the command line.TL;DR:
Should we change creds.json to (A) include the provider name as a field, (B) change cred-names to be name:PROVIDER, (C) change cred-names to be PROVIDER:name? All of this will be done with a smooth transition.
Defintions:
"provider name": The identifier of a service provider, as embedded in the code. For example,
CLOUDFLAREAPI
,GANDI_V5
,BIND
,AZURE_DNS
,ROUTE53
,GCLOUD
, and so on. (complete list)"cred-name": The name given to a set of credentials in
creds.json
. In the examples below, they arecloudflare_tal
,gandi
, andinside
. There may be multiple cred-names that point to the same provider name. For example, a user with domains in many Azure accounts may have multiple cred-names, one for each account, but all using theAZURE_DNS
provider.Detailed description:
There are a number of commands and functions that require the user to specify both the provider name and the cred-name.
For the purpose of this discussion, assume this is the
creds.json
file:CURRENT SITUATION: Here are some examples where both the provider name and the cred-name must be specified:
dnsconfig.js:
On the command line:
In fact, nearly everywhere a
creds.json
entry name is specified, the provider name is specified. That is, nearly any time one specifiescloudflare_tal
the user must also specifyCLOUDFLAREAPI
; every time one specifiesgandi
the user must also specifyGANDI_V5
; every time one specifiesinside
the user must also specifyBIND
.The one exception is the command line tool's (confusingly named)
--providers
flag which accepts a list of cred-names and does not require (in fact, it won't accept) provider names.As you can see from the above examples, this is redundant and inconvenient. In the case of
get-zones
it is confusing to the users that have to specify two parameters where it seems like one would do (the reason, of course, is thatget-zones
only accesses creds.json, and never the dnsconfig.js file, which is where the name of the provider is stored).The goal would be to remove this repetition by placing the provider name in the creds.json file, not the dnsconfig.js file or on the command line.
PROPOSED SITUATION: If we are successful, the above examples would look like this:
On the command line:
This is cleaner, clearer, and less typing!
BACKWARDS COMPATIBILITY:
It is a hard requirement that users must be able to update their
creds.json
file to comply with the new format before the new format becomes a requirement. That is, the new format must work on 3.x releases (with new elements silently ignored). In 4.0 (our next opportunity for breaking changes) the new format will be required.To that end, we will do the following:
-
will mean "refer to the cred-name in creds.json".-
will generate no warnings or errors).-
as a placeholder, and output a warning if anything else is used (possibly an error if the wrong provider name is specified).Proposal A: provider name as a field
Store the provider name in a field called "PROVIDER" of each entry in creds.json. For example:
Open issues:
PROVIDER
the right field name? Would_PROVIDER
be better? Something else?Backwards compatibility concerns:
Proposal B: name:PROVIDER
Append the provider name to the end of cred-name (i.e.
name:PROVIDER
). For example:Notes:
name
orname:PROVIDER
.foo:ROUTE53
andfoo:GANDI_V5
. (I had considered permitting such duplicates and implementing some kind of "longest match" system but... why add so much complexity?)Backwards compatibility concerns:
:
in a cred-name?:
may become significant in the future and suggest using_
instead.Proposal C: PROVIDER:name
This is the same as Proposal B, but the order is reversed. i.e. the credential keys would be
PROVIDER:name
instead ofname:PROVIDER
.For example:
With this format change, the examples, open issues, and backwards compatibility issues are the same as Proposal B.
Personal option: I think this makes the
creds.json
file look better. The key looks nicer and sorting the keys will group all the keys of a single provider. However when someone specifies a key on the command line, it looks ugly to me because normally abbreviations chop off the end, not the beginning.The text was updated successfully, but these errors were encountered: