Add: support dns (lookup) command #6
Conversation
patrykcieszkowski
force-pushed
the
2-dns-support
branch
from
79c472c to
09b69c0
Compare
|
@zarianec I don't think this DeepSource linter is configured in accordance with our project eslint. |
You're admin of deepsource. Can you please configure it as you see fit? |
|
yep, we just enabled it so it may require rules tweaking to make it work as expected. It's not necessary DeepSource checks to always pass. The main goal is to provide additional information and insights. |
|
and some rules in DeepSource are really stupid I must say 🤯 |
src/command/dns-command.ts
Outdated
| type: Joi.string().valid('dns'), | ||
| target: Joi.string(), | ||
| query: Joi.object({ | ||
| type: Joi.string().valid(...allowedTypes).optional(), |
There was a problem hiding this comment.
Why not provide default value here? You can make the property required in the DnsOptions type then and avoid non-null assertions later.
src/command/dns-command.ts
Outdated
| target: Joi.string(), | ||
| query: Joi.object({ | ||
| type: Joi.string().valid(...allowedTypes).optional(), | ||
| address: Joi.string().optional(), |
There was a problem hiding this comment.
resolver or nameServer should be a better name.
src/command/dns-command.ts
Outdated
|
|
||
| const args = [ | ||
| options.target, | ||
| `@${options.query.address ?? getDnsServers().pop()!}`, |
There was a problem hiding this comment.
This is an optimistic approach 🙂 . Dns servers returned by dns.getServers() may include addresses with a port as well as ipv6 addresses. Why not just avoid the @servername option if no options.query.address provided?
There was a problem hiding this comment.
Maybe. That's a leftover from the previous library - it wouldn't find its own defaults.
There was a problem hiding this comment.
We should let dig use the system defaults for now, so dont provide anything manually
src/command/dns-command.ts
Outdated
| options.target, | ||
| `@${options.query.address ?? getDnsServers().pop()!}`, | ||
| ['-t', options.query.type ?? 'A'], | ||
| ['-p', options.query.port ?? '53'], |
There was a problem hiding this comment.
-4 +time=1 +tries=2 params missing. They must be hardcoded.
|
I guess it's not possible with |
It doesn't, it outputs a promise resolution after completed query. If we want the progress event to be outputted, we'll have to build our own library (or just copy the codebase of |
|
@patrykcieszkowski tests missing for this new command. |
read the top message |
Not true. Yes, parsing is done by a 3rd-party library but we send this output to the API using websockets and we must be sure that this output won't change after we update the library, change the code, etc. |
|
My goal is eventually to have 100% code coverage. |
|
Following the discussion in private channels, it's not much to test there since we use an external library and don't have a mechanism of mocking/testing calls for 3rd-party system components (ping, dig, etc.). |
resolve #2
There's not many actively maintained libraries around, since node's official package resolves DNS pretty well (https://nodejs.org/api/dns.html).
It doesn't however seem to support protocol selection, so that won't work for us.
My initial commit implemented domain-info package, but I decided to ditch it since it doesn't seem to be actively maintained either, its last build failed. It supports multi dns type queries, which is interesting. The package @zarianec (node-dig-dns) has proposed on the other hand, doesn't include types so they had to be included manually.
No tests, since theres nothing to test. Parsing is handled by the library.