-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Run DNS lookup for --api endpoint provided in CLI #5372
Conversation
9a1d041
to
03c1752
Compare
Nice! We usually don't bother unit testing small helper functions like that but that's just because we're horrible, lazy coders so I definitely won't say no to it. Mind adding an integration test? You can find the others in |
@Stebalien – added a sharness test, modelled after t0235. Only testing the appearance of the POST, which is enough to assert that the DNS resolution is happening. Had to use an existing domain that resolves to |
Actually, I'd much rather use localhost. That way the tests pass while offline (e.g., when on a plane). Really, we don't care about actually using DNS, we just want to hit the multiaddr-dns resolver. |
f1d7c21
to
3b24eb8
Compare
License: MIT Signed-off-by: Raúl Kripalani <[email protected]>
Resolves ipfs#5249. Calls multiaddr-dns, and picks the first result. Uses a fixed timeout of 10 seconds. Adds test cases for one, multiple, and no DNS results. License: MIT Signed-off-by: Raúl Kripalani <[email protected]>
License: MIT Signed-off-by: Raúl Kripalani <[email protected]>
License: MIT Signed-off-by: Raúl Kripalani <[email protected]>
License: MIT Signed-off-by: Steven Allen <[email protected]>
3b24eb8
to
c89102a
Compare
(figured I might as well just fix that rather than make you do yet another round of CR). Rebased and probably ready... |
Thanks @Stebalien, somehow I had missed this. I had wrongly assumed that All good, thanks 👍 |
This is my first PR to go-ipfs, so please be gentle ;-)
Imports
go-multiaddr-dns
and callsmadns.Resolve()
with the provided multiaddr. There's no need to callMatches()
explicitly, as the resolution will do no-op if no DNS protocols are found in the input multiaddr (I'm assuming this is part of the contract ofmadns.Resolve()
).If multiple results are returned, only the first one is used. See #5249 (comment).
I've added several unit tests in the
dnsresolve_test.go
file. Not sure if that's the best place. I'm happy to change it.I initially added the call to
madns.Resolve()
inapiClientForAddr()
but I couldn't assert on the address ofhttp.Client
as its private, so I had to put it in a separate function to enable unit testing.Fixes #5249.