-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add NULL record #840
Add NULL record #840
Conversation
Codecov Report
@@ Coverage Diff @@
## master #840 +/- ##
=========================================
- Coverage 57.25% 57.2% -0.05%
=========================================
Files 41 41
Lines 10534 10583 +49
=========================================
+ Hits 6031 6054 +23
- Misses 3422 3441 +19
- Partials 1081 1088 +7
Continue to review full report at Codecov.
|
0df5fca
to
5425b6d
Compare
rebased and regenerated. Only needs a little test. Hard to validate if the implementation is interopereable because there is no presentation format. |
|
Could you add a test against a fixed record data that was generated by something else? |
I don't know how another DNS implementations can be made to generate these,
but I'll take a look
…On Sat, 1 Dec 2018, 10:20 Tom Thorogood ***@***.*** wrote:
Could you add a test against a fixed record data that was generated by
something else?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#840 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAVkW3BhgnVAbrx8XfOo4Lg5zDnig_h7ks5u0lgEgaJpZM4Y4TBB>
.
|
If they’re used by tunneling implementations, maybe one of those could be used to generate the record? It’s not obvious without a presentation format. |
At the very least, you could add a regression test by using this implementation to generate the record to make sure it doesn’t accidentally change. |
b392f2b
to
072a1a3
Compare
`t.example.com. IN TXT aaa ;`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\"", | ||
`t.example.com. IN TXT aaa aaa;`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\" \"aaa\"", | ||
`t.example.com. IN TXT aaa aaa`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\" \"aaa\"", | ||
`t.example.com. IN TXT aaa`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\"", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miekg I think you need to update gofmt
, see https://golang.org/doc/go1.11#gofmt. The original formatting of these lines was correct as of go1.11.
[ Quoting <notifications@github.com> in "Re: [miekg/dns] Add NULL record (#8..." ]
tmthrgd requested changes on this pull request.
> - `t.example.com. IN TXT "abc" "DEF"`: "t.example.com.\t3600\tIN\tTXT\t\"abc\" \"DEF\"",
- `t.example.com. IN TXT "abc" ( "DEF" )`: "t.example.com.\t3600\tIN\tTXT\t\"abc\" \"DEF\"",
- `t.example.com. IN TXT aaa ;`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\"",
- `t.example.com. IN TXT aaa aaa;`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\" \"aaa\"",
- `t.example.com. IN TXT aaa aaa`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\" \"aaa\"",
- `t.example.com. IN TXT aaa`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\"",
+ `t.example.com. IN TXT ""`: "t.example.com.\t3600\tIN\tTXT\t\"\"",
+ `t.example.com. IN TXT "a"`: "t.example.com.\t3600\tIN\tTXT\t\"a\"",
+ `t.example.com. IN TXT "aa"`: "t.example.com.\t3600\tIN\tTXT\t\"aa\"",
+ `t.example.com. IN TXT "aaa" ;`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\"",
+ `t.example.com. IN TXT "abc" "DEF"`: "t.example.com.\t3600\tIN\tTXT\t\"abc\" \"DEF\"",
+ `t.example.com. IN TXT "abc" ( "DEF" )`: "t.example.com.\t3600\tIN\tTXT\t\"abc\" \"DEF\"",
+ `t.example.com. IN TXT aaa ;`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\"",
+ `t.example.com. IN TXT aaa aaa;`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\" \"aaa\"",
+ `t.example.com. IN TXT aaa aaa`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\" \"aaa\"",
+ `t.example.com. IN TXT aaa`: "t.example.com.\t3600\tIN\tTXT\t\"aaa\"",
@miekg I think you need to update `gofmt`, see https://golang.org/doc/go1.11#gofmt. The original formatting of these lines was correct as of go1.11.
ah.. I was wondering about this. Must be ages since I go-installed gofmt
|
ok, conflict resolved and rebased (manually edited zmsg.go, because it can't 'go install' the right version of the go library to make go generate work). PTAL |
@tmthrgd I think this is good to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from that one nit, this LGTM.
Sorely missing from this library. Add it. As there is no presentation format the String method for this type puts a comment in front of it. Signed-off-by: Miek Gieben <miek@miek.nl>
Sorely missing from this library. Add it.