Skip to content
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

Fixes #50. #51

Merged
merged 2 commits into from
Dec 17, 2015
Merged

Fixes #50. #51

merged 2 commits into from
Dec 17, 2015

Conversation

dekobon
Copy link
Contributor

@dekobon dekobon commented Dec 17, 2015

I've been trying to execute the unit tests on my local workstation outside of the docker build environment. The tests were failing on IP parsing. Since this would be a quick fix, I went ahead and made the changes. Please tell me what you think.

@tgross
Copy link
Contributor

tgross commented Dec 17, 2015

LGTM once you add some tests, @dekobon. But...

I've been trying to execute the unit tests on my local workstation outside of the docker build environment.

...I feel like that shouldn't necessarily be supported. The reason we use Docker as the build platform is to avoid having to support every local configuration in existence; Golang developers in particular have a bad habit of making all kinds of assumptions about GOPATH and dependency management that we really don't want to get into.

@justenwalker
Copy link
Contributor

Interesting, so it groups IPV4 and IPV6 addressed together.

@tgross - Maybe I'm over thinking this, but; Golang docs don't document this behavior. I hope we aren't relying something undocumented about the order in which these IPs are returned.

Should we instead split & parse these addresses and explicitly pick the one which is IPV4? Should we have a flag to prefer IPV4 over IPV6?

@misterbisson
Copy link
Contributor

Should we have a flag to prefer IPV4 over IPV6?

@justenwalker and @tgross: do we need to ticket an issue to add IPv6 support to Containerbuddy (and for users to specify their preference)?

@dekobon
Copy link
Contributor Author

dekobon commented Dec 17, 2015

You can peek at what golang is doing here: https://golang.org/src/syscall/netlink_linux.go

Also, it looks like we are in good company about IPV6 confusion: hashicorp/consul#529

Maybe we want to let the user specify what what IP to advertise.

@tgross
Copy link
Contributor

tgross commented Dec 17, 2015

Ensuring we have IPv6 support is likely to be a bit of extra effort to make sure we're not making IPv4 assumptions elsewhere in the code. Here's what I recommend:

  • For this issue let's force the case of a IPv4/6 interface to select the IPv4 IP address by parsing the addresses as @justenwalker has suggested.
  • And then let's open a new ticket per @misterbisson's suggestion to provide IPv6 support so that we can review the whole code base in that light.

@tgross tgross mentioned this pull request Dec 17, 2015
@tgross
Copy link
Contributor

tgross commented Dec 17, 2015

This LGTM. I've opened #52 for further discussion of the IPv6 issue.

tgross added a commit that referenced this pull request Dec 17, 2015
@tgross tgross merged commit 69ad0af into TritonDataCenter:master Dec 17, 2015
@dekobon dekobon deleted the fix_50 branch December 17, 2015 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants