-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
3932277
to
4ebe5a8
Compare
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -92,7 +93,37 @@ func TestSidecar(t *testing.T) { | |||
ID: "008", | |||
Name: "api", | |||
Hostname: "another-aws-host", | |||
Status: 0, | |||
Ports: []service.Port{ | |||
service.Port{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gofmt -s
on this file and it will probably remove these inner service.Port
names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's one annoyance of goimports. 💯 I'll take a look at the fork.
provider/sidecar_test.go
Outdated
@@ -101,13 +132,21 @@ func TestSidecar(t *testing.T) { | |||
|
|||
backs := prov.makeBackends(states) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
|
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: