Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 20, 2025
Merged

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

merged 1 commit into from
Apr 20, 2025

Conversation

gucci-on-fleek
Copy link
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
Collaborator Author

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

@gucci-on-fleek gucci-on-fleek mentioned this pull request Apr 19, 2025
Merged
mholt
mholt previously approved these changes Apr 19, 2025
View reviewed changes
Copy link
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
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.

mholt
mholt approved these changes Apr 20, 2025
View reviewed changes
Copy link
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
@gucci-on-fleek
Update go.mod to depend on libdns/libdns#166
116f7d3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mholt mholt mholt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gucci-on-fleek @mholt