Skip to content

Make all zero values have .RR().Data == ""#166

Merged
mholt merged 1 commit intolibdns:masterfrom
gucci-on-fleek:missing-ip
Apr 20, 2025
Merged

Make all zero values have .RR().Data == ""#166
mholt merged 1 commit intolibdns:masterfrom
gucci-on-fleek:missing-ip

Conversation

@gucci-on-fleek
Copy link
Copy Markdown
Collaborator

(Noticed in libdns/rfc2136#9 (comment))

The Address.IP field is allowed to be zero-initialized for use in DeleteRecords:

libdns/libdns.go

Lines 200 to 205 in 5ab1d4d

// As a special case, you may leave any of the fields [libdns.Record.Type],
// [libdns.Record.TTL], or [libdns.Record.Value] empty ("", 0, and ""
// respectively). In this case, DeleteRecords will delete any records that
// match the other fields, regardless of the value of the fields that were left
// empty. Note that this behavior does *not* apply to the [libdns.Record.Name]
// field, which must always be specified.

Currently, calling .RR() on such an Address gives rr.Data == "invalid IP", but this is a fairly surprising zero value, so this PR changes this to use the more conventional "".

@gucci-on-fleek gucci-on-fleek requested a review from mholt April 19, 2025 07:21
@gucci-on-fleek gucci-on-fleek changed the title Make missing IP addresses map to "" Make all zero values have .RR().Data == "" Apr 19, 2025
@gucci-on-fleek gucci-on-fleek changed the title Make all zero values have .RR().Data == "" Make all zero values have .RR().Data == "" Apr 19, 2025
@gucci-on-fleek
Copy link
Copy Markdown
Collaborator Author

Hmm, I suppose that this really should apply to the zero values for all Records, not just Addresses.

mholt
mholt previously approved these changes Apr 19, 2025
Copy link
Copy Markdown
Contributor

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I hadn't thought about this 😅 I did have one nit, but it's totally bikeshedding. Just wanted your thoughts on it. I'm good to merge it either way.

@gucci-on-fleek
Copy link
Copy Markdown
Collaborator Author

Ok, I've rebased onto master and fixed the data checks. Can you please tag a -beta.2 once this is merged so that I can use it at libdns/rfc2136? Thanks again.

Copy link
Copy Markdown
Contributor

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you!!

@mholt mholt merged commit 10a2b0b into libdns:master Apr 20, 2025
gucci-on-fleek added a commit to gucci-on-fleek/libdns-rfc2136 that referenced this pull request Apr 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants