add icann.dns to meta-TLD #67
Conversation
added icann.dns support to the http server returns dns response json
| authority: [] | ||
| additional: [] | ||
| edns_options: [] | ||
| payload: undefined} |
There was a problem hiding this comment.
It's potentially dangerous to do it this way, since the format could change in the future if native-dns gets updated. If you need to create a new empty packet, then just create an instance of the class that already exists (i.e. in your resolve: method). For oldDNSLookup: you should just keep and use the parameter that was passed in.
There was a problem hiding this comment.
Yeah, this felt a little gross. Problem is, while we can copy the resultant packet from the request in oldDNSLookup (and replace this line) there are no packets to copy from in resolve; it creates a new packet where there was only a 'path' string before.
There was a problem hiding this comment.
Problem is, while we can copy the resultant packet from the request in oldDNSLookup (and replace this line)
So do it. 👍 (Unless there's a reason not to...?)
there are no packets to copy from in resolve; it creates a new packet where there was only a 'path' string before.
Did you have a look at this giant comment above callback:?
It explains the way native-dns works, and where to look for classes and whatnot. But I'm pretty sure you know that since you wrote:
# fields from: https://github.com/tjfontaine/native-dns-packetSo then you should know that you can simply:
req = new require('native-dns-packet')()(Except I wouldn't do it with an inline require like that.)
|
Good job @wemeetagain! See my review above. I tested it and it seems to be working correctly. Some changes probably need to be made as per the review (and some questions need answering). Also, can you add the ability to specify the record type? The reason is that by default this just returns |
| @oldDNSLookup req, res | ||
| @oldDNSLookup req, (packet, code) -> | ||
| _.assign res, _.pick packet, ['edns_version', 'edns_options', | ||
| 'edns', 'answer', 'authority', 'additional'] |
There was a problem hiding this comment.
We're copying over a list of properties from packet, the output of oldDNSLookup with the answers, into res, the packet with the proper _socket that was passed into callback.
Sorry, I guess I wasn't clear enough: this is already done in on 'message'. DRY.
This should be done once only, somewhere, whether here or elsewhere. One has to go.
|
@wemeetagain OK, review done! |
| @@ -43,6 +43,7 @@ | |||
| "lodash": "~2.4.1", | |||
| "lodash-contrib": "^241.4.14", | |||
| "native-dns": "git+https://github.com/okTurtles/node-dns.git#08433ec98f517eed3c6d5e47bdf62603539cd402", | |||
| "native-dns-packet": "^0.1.1", | |||
There was a problem hiding this comment.
Use precise versioning (remove the ^). See #78.
|
@wemeetagain Can you rebase your stuff off the latest master and |
wemeetagain
force-pushed
the
icann.dns
branch
from
663010c to
ac5eb2d
Compare
added icann.dns support to the http server
returns dns response json