I've been updating the Cloudflare provider in a branch that uses this branch. I'm testing with a SRV record currently, since it's a more complicated record type.

So far, the effects of this refactor look promising.

Except... something that doesn't really have much to do with this PR...

Since the Cloudflare documentation shows a "content" field and optional "data" components, I figured I can just pass in the content of the record as a single string and call it a day. (That's easy now with the RR type.) It's MUCH simpler with this PR, than we had before.

But, unfortunately, either their docs are wrong or their API is wrong: https://community.cloudflare.com/t/creating-srv-record-with-content-string-instead-of-individual-component-fields/781178?u=mholt -- I am getting errors when omitting the individual data components, despite the docs saying they are optional.

So, I'm hoping their API is just buggy and the docs are right, since otherwise I have to type-assert every single record type to use their API. Doable, and inevitably some providers will require this, but kinda lame.

@gucci-on-fleek Thanks for the thorough review!

I believe I have addressed (pun intended?) all the comments, including combining A and AAAA into an Address struct, except for the matter of SVCB records.

Real quick: the two downsides to combining into the Address type, that I can think of, are:

• Users might be looking for A or AAAA in the godoc, and won't find it, unless they do a Find-in-Page. (Simply pressing "F" won't bring up Address, at least I assume not, because "A" and "AAAA" are not in the type definition itself.)
• There is no validation to ensure IPv4 or IPv6.

However, given that users don't really care what the size of the address is in Go code, I think at least that second downside is acceptable. The code automatically assigns the correct RR-type either way.

As for SVCB records, I'm inclined to keep them separate since they have slightly different semantics around the name, compared to HTTPS records. Do you agree?

What about adding some test helpers to libdns/libdns, or subpackage, or other testing-module, that provider implementers can run on their provider? I think it would be like what is suggested in issue #44. I can write some of these tests.

I imagine the tests would be run with a provider impelmentation, zone name, and "subzone" name, and the tests would get/add/set/delete records and see the changes are reflected properly, and check the names are well formed (relative names, "@" instead of empty name, nothing outside the zone).

We could add an Options struct to influence behaviour to check for, such as skipping checks for whether exact TTLs are set (some DNS providers appear to only allow setting a small set of TTLs). The options can be expanded later on.

We could check context cancellations, do concurrent requests and test with the race checker on. We would take care not to touch anything outside the subzone, but of course bugs in provider implementations could do the wrong thing... Hopefully implementers are in a position to create an isolated account/zone.

@gucci-on-fleek Awesome, thanks for trying out the new APIs!

The only challenge that I had was converting all Target fields to FQDNs in the test code. This was a little bit awkward since I had to hardcode the list of types with target fields:

https://github.com/gucci-on-fleek/libdns-rfc2136/blob/4d49bd/provider_test.go#L66-L89

But testing code is often pretty awkward, so I think that this is fine.

Oof, you're right, that does look a bit tedious. Is that kind of enumeration only going to happen in tests? If so, I think I'm okay with it. If it's going to be in production code, I wonder if we could assist with a common method that could be used with a type assertion, so it works with all those types with targets.

And similarly, == doesn't work on the HTTPS structs, but I don't think that you ever need to compare records outside of testing code, so I don't think that there's any need to add an Equal method.

Yeah... also, equality is not obvious for structures like that. Hence why Go doesn't just support it already. So it's probably best if users implement their own equality logic. There are many ways to do it.

I've got an exam tomorrow, so I'll be busy studying tonight, but I've got time tomorrow evening, so I'll submit a PR then.

Rock on, good luck!! Thank you again -- looking forward to wrapping it up :)

@mjl- Thanks for your feedback, and for testing out the new APIs!

What about adding some test helpers to libdns/libdns, or subpackage, or other testing-module, that provider implementers can run on their provider? I think it would be like what is suggested in issue #44. I can write some of these tests.

Ah yeah, this -- and the rest of your comment -- would be great to discuss in either that issue or a new one, to give it all the attention it deserves. How does that sound? We could look at it after this PR is merged (although, I will be having my hands full getting all the providers notified and to try to update, etc, so it might be more you and other collaborators working on it).

@mjl-

What about adding some test helpers to libdns/libdns, or subpackage, or other testing-module, that provider implementers can run on their provider? I think it would be like what is suggested in issue #44. I can write some of these tests.

I'd certainly be in favour of that. I've added a few tests at libdns/rfc2136@4d49bd3, but nothing too extensive yet. I'd recommend looking at DNSControl's tests for inspiration since they've thought of most of the weird edge cases there already.

(And if either of you want the credentials to a test zone on my DNS server, just email me and I'll happily send them over)

@mholt

The only challenge that I had was converting all Target fields to FQDNs in the test code. This was a little bit awkward since I had to hardcode the list of types with target fields:
https://github.com/gucci-on-fleek/libdns-rfc2136/blob/4d49bd/provider_test.go#L66-L89
But testing code is often pretty awkward, so I think that this is fine.

Oof, you're right, that does look a bit tedious. Is that kind of enumeration only going to happen in tests?

I think so? I mean, I guess if you were writing a “DNS Target Verifier” program that makes sure that all the targets in your zone point at valid domains this might come up, but that seems like a fairly contrived example.

And similarly, == doesn't work on the HTTPS structs, but I don't think that you ever need to compare records outside of testing code, so I don't think that there's any need to add an Equal method.

Yeah... also, equality is not obvious for structures like that. Hence why Go doesn't just support it already. So it's probably best if users implement their own equality logic. There are many ways to do it.

The runtime panic might catch people by surprise though, especially since every other Record type is comparable, so you could easily miss it in testing:

package main

import (
	"fmt"

	"github.com/libdns/libdns"
)

func main() {
	var a libdns.Record = libdns.HTTPS{
		Name: "test",
		Value: libdns.SvcParams{
			"key": []string{"value1", "value2"},
		},
	}
	b := a

	if a == b {
		fmt.Println("equal")
	} else {
		fmt.Println("not equal")
	}
}

So maybe it might be better to instead define

type HTTPS struct {
	Name     string
	TTL      time.Duration
	Priority uint16
	Target   string
	Value    *SvcParams
}

That should stop it from panicking, but I don't know enough about Go to know the greater implications to making a struct field a pointer.

Ah yeah, this -- and the rest of your comment -- would be great to discuss in either that issue or a new one, to give it all the attention it deserves. How does that sound? We could look at it after this PR is merged (although, I will be having my hands full getting all the providers notified and to try to update, etc, so it might be more you and other collaborators working on it).

Depending on the timeline of all of this, it might be good to have the tests ready before we notify all of the provider maintainers. Having tests would probably make it easier to port all the providers open, and it would be mildly frustrating to finish porting a provider and then see that all the tests are failing after. But porting libdns/rfc2136 over was easy enough that I don't think that this should be a huge concern either way.

Ok, I've pushed support for SVCB records. The code is a bit of a mess, so please feel free to change it as you wish. All of the current tests pass, but I'll add a few more tests tomorrow. Anyways, please let me know what you think.

Working on updating https://github.com/libdns/domainnameshop for this and made good headway.
However the biggest thing I'm missing from the old is the ID field on RR....

There is a need for a unique identifier that can be kept track of, Name is not good enough since it's not a unique field.

In the case of Domanname.shop the API returns an ID when you create a record and you need an ID to remove a record.
And querying for a specific record is also based on this ID.

Without an ID field in the RR type I have to keep track of this ID some other way since I can't expect it to be carried by a libdns.Record.
Or I have to query the entire zone all the time and then compare the name and data and guess that it matches.

Working on updating https://github.com/libdns/domainnameshop for this and made good headway. However the biggest thing I'm missing from the old is the ID field on RR....

There is a need for a unique identifier that can be kept track of, Name is not good enough since it's not a unique field.

In the case of Domanname.shop the API returns an ID when you create a record and you need an ID to remove a record. And querying for a specific record is also based on this ID.

See #153 (comment)

Without an ID field in the RR type I have to keep track of this ID some other way since I can't expect it to be carried by a libdns.Record. Or I have to query the entire zone all the time and then compare the name and data and guess that it matches.

Yes, that's the only way, here's an example.
https://github.com/libdns/westcn/blob/b396bede1b721a69d468184711bd58495771788f/provider.go#L122
https://github.com/libdns/huaweicloud/blob/080d29a84403f403b5602428eb30ae71032f2c42/client.go#L133

That's a bit of hassle, but I've made it work and updated my module, got it working with Caddy which is my main focus.
Though some edge cases of multiple "identical"/similar records is not cleanly solved.

@JMyklebust Thanks for the feedback / update! Yeah, the ID field was a tricky one... At first it seemed obvious we should have it, but then there's some very subtle and potentially harmful outcomes of doing that, considering cross-zone and cross-provider compatibility... anyway you probably read the discussion and get the gist.

Thanks for maintaining your package -- changes look good, at a quick glance 👍

See also #119 (comment)