Conversation
mihaitodor
force-pushed
the
use-sidecar-host-ips
branch
from
3932277 to
4ebe5a8
Compare
mihaitodor
force-pushed
the
use-sidecar-host-ips
branch
from
4ebe5a8 to
1062fec
Compare
I'd gone back and forth on this before, but lean towards not adding it now. I think you'd argued this previously too @mihaitodor; if we can't resolve the address at this stage, the rest of the program likely won't be able to either. |
relistan
left a comment
relistan
left a comment
There was a problem hiding this comment.
Code looks good, some comments below.
provider/sidecar.go
Outdated
| URL: "http://" + ipAddr[0].String() + ":" + strconv.FormatInt(svc.Ports[i].Port, 10), | ||
| for _, port := range svc.Ports { | ||
| name := svc.Hostname | ||
| if len(svc.Ports) > 1 { |
There was a problem hiding this comment.
Let's just make them consistent and always use the port.
provider/sidecar.go
Outdated
| } | ||
|
|
||
| host := port.IP | ||
| if host == "" { |
There was a problem hiding this comment.
I don't do this in Sidecar and should, but let's be careful here to make sure we got a properly formed IP address by doing an net.ParseIP here to make sure it's a valid IP as well. We will just use the string, but the parse will tell us it's legit.
provider/sidecar_test.go
Outdated
| Hostname: "another-aws-host", | ||
| Status: 0, | ||
| Ports: []service.Port{ | ||
| service.Port{ |
There was a problem hiding this comment.
gofmt -s on this file and it will probably remove these inner service.Port names.
There was a problem hiding this comment.
Nice catch @relistan! So, it turns out that the standard goimports doesn't support the -s flag that gofmt provides. I ended up installing this 3rd party goimports to have this flag and keep using the goimports hook on save.
There was a problem hiding this comment.
Yep, that's one annoyance of goimports. 💯 I'll take a look at the fork.
| @@ -101,13 +132,21 @@ func TestSidecar(t *testing.T) { | |||
|
|
|||
| backs := prov.makeBackends(states) | |||
|
|
|||
There was a problem hiding this comment.
This seems like way too much stuff to be checking in a single test. The comments explaining all the checks are the giveaway. Even if you have to repeat the same loading of the data multiple times in separate tests, this should be broken into tests that are validating the same functionality. Just share the setup between the Convey blocks and give the So clauses a wrapper that explains what they're doing.
There was a problem hiding this comment.
Good point! I'm starting to like goconvey :)
|
@relistan I think I covered everything. Please let me know if it's good to go now. |
|
|
!3 participants
This is a follow-up to the conversation from here.
We want to fetch the IP addresses from Sidecar, if possible.
Also, we want to support having multiple ports exposed for the same service on the same host. This changeset appends
_<port>to the service name when multiple ports are provided instead of exposing only the first port.TODO: