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.

Sign up for GitHub

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

Jump to bottom

Get Public IPs (IPv6) #189

Merged
merged 2 commits into from
May 27, 2020
Merged

Get Public IPs (IPv6) #189

merged 2 commits into from
May 27, 2020

Conversation

ghost
Copy link

@ghost ghost commented May 21, 2020

Exposed IPsV6 and AllIPs APIs. Added tests.

philippschulte
philippschulte reviewed May 21, 2020
View reviewed changes
Copy link
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

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

@wbautistaEXPEDIA thank you for your contribution. This PR introduces a breaking change. Why not leaving the IPs function as it is and just introducing a new function for retrieving the IPv6 addresses instead? @phamann what are your thoughts on this?

@ghost
Copy link
Author

ghost commented May 21, 2020

Why not leaving the IPs function as it is and just introducing a new function for retrieving the IPv6 addresses instead?

That's what I did. I think the diff in the PR may be adding to confusion, but it essentially leaves the original API as is, and exposes two new APIs (one for retrieving both sets, and one for the ipv6 set). See https://github.com/fastly/go-fastly/pull/189/files#diff-063a6e698147141c9ccad8e9632e0e13R22

philippschulte
philippschulte approved these changes May 21, 2020
View reviewed changes
Copy link
Member

@philippschulte philippschulte left a comment

Choose a reason for hiding this comment

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

Apologies, @wbautistaEXPEDIA! Sometimes the diff is confusing. I'd like to get @phamann approval before we merge and cut a new release. @phamann this looks good to me, can you please review?

phamann
phamann approved these changes May 26, 2020
View reviewed changes
Copy link
Member

@phamann phamann left a comment

Choose a reason for hiding this comment

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

👍 LGTM thank you for the contribution, @wbautistaEXPEDIA!

@ghost
Copy link
Author

ghost commented May 26, 2020

@philippschulte
Is there an ETA on merges and releases?

@philippschulte philippschulte merged commit 1f66b72 into fastly:master May 27, 2020
@philippschulte
Copy link
Member

@wbautistaEXPEDIA v1.14.0 has been released. Thanks again for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@philippschulte @phamann @wbautistajr