Feat/client features #266
Feat/client features #266
Conversation
hsanjuan
commented
Dec 8, 2017
hsanjuan
commented
- Auto-resolve dns multiaddresses
- Support Host/Port as strings in the config
- Return regular errors from methods
This is easier in many cases and does not make the user parse a multiaddress. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
We had a problem happening when assigning the returned *api.Error to default 'error' type. Things like "if err != nil" would not work even when *api.Error is nil I'm not sure why this happens, but this is very confusing for the user integrating on top. It is better that we just return plain go errors. License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
ghost
added
the
| resolveCtx, cancel := context.WithTimeout(ctx, cfg.Timeout) | ||
| defer cancel() | ||
| resolved, err := madns.Resolve(resolveCtx, cfg.APIAddr) | ||
| cfg.APIAddr = resolved[0] |
There was a problem hiding this comment.
Just double checking that the first element of resolved is what we want here. Looking at madns I found that the first element in the case of an ipv4 address is the ip4maddr from: ip4maddr, err := ma.NewMultiaddr("/ip4/" + ip4.String()). I had trouble tracking down what ip4 exactly is from the code but was suspicious this might just be an ip and not include a transport. Maybe that is fine too? Just bringing up because that wouldn't match with the default multiaddr (ip + tcp addrs).
There was a problem hiding this comment.
I guess you saw:
ip4maddr, err := ma.NewMultiaddr("/ip4/" + ip4.String())
if err != nil {
return result, err
}
parts := append([]ma.Multiaddr{ip4maddr}, encap...)
result = append(result, ma.Join(parts...))
See it does ma.Join on the result, so the resulting multiaddress has all the parts after the "/ip4/ip", including the /tcp/port etc..
There was a problem hiding this comment.
Ah I was thrown off thinking result already has something else in it, thanks this makes sense
api/rest/client/client_test.go
Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if c.urlPrefix != "http://127.0.0.1:1234" { | ||
| t.Error("bad resolved address") | ||
| } | ||
| } | ||
|
|
||
| func TestVersion(t *testing.T) { |
There was a problem hiding this comment.
Why did these tests make codeclimate unhappy? Are we planning to replace them?
There was a problem hiding this comment.
oh, i forgot to add the file, i moved them
|
Code climate complained about function length, complexity, fake duplicates... I'm adjusting the settings, somethings are better fixed, others are not so realistic. |
License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
License: MIT Signed-off-by: Hector Sanjuan <code@hector.link>
hsanjuan
force-pushed
the
feat/client_features
branch
from
4f69606 to
d6bb5bb
Compare
|
@ZenGround0 added the missing file |
