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

Add GetIPAddress() #1

Merged
merged 2 commits into from
Jan 10, 2023
Merged

Add GetIPAddress() #1

merged 2 commits into from
Jan 10, 2023

Conversation

amery
Copy link
Contributor

@amery amery commented Jan 8, 2023

Add GetIPAddresses() and a dumb GetIPAddress()

@amery
Copy link
Contributor Author

amery commented Jan 8, 2023

this is mostly a call for discussion. @karasz

  1. should we introduce a new dedicated package to work with addresses here and on davarza?
  2. what's the best way to choose what address are we most likely to use?

@amery amery requested a review from karasz January 8, 2023 16:58
@amery
Copy link
Contributor Author

amery commented Jan 8, 2023

or maybe use an existing third-party dependency? does harshicorp have something already?

Copy link
Collaborator

@karasz karasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM comments on code.

s = v.IP
}

if ip, ok := netip.AddrFromSlice(s); ok {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if I want to be nitpicking (which I don't) I would say that there are 3 possible results that come from AddrFromSlice (IPv4 if the slice length is 4) and either a IPv6 mapped IPv4 or a standard IPv6 if the slice length is 16. The thing is that you cannot actually distinguish between the later two options easily, the question is do we care? at this point I think we do not care so I will approve.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those are supposed to be treated as IPv6

@amery amery merged commit e060c9e into main Jan 10, 2023
@amery amery deleted the pr-amery-GetIPAddress branch January 10, 2023 15:15
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.

None yet

2 participants