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

Enclose IPv6 addresses in "[]" #3477

Merged
merged 4 commits into from
Jun 13, 2018
Merged

Enclose IPv6 addresses in "[]" #3477

merged 4 commits into from
Jun 13, 2018

Conversation

herver
Copy link
Contributor

@herver herver commented Jun 12, 2018

What does this PR do?

Add square brackets [ and ] when dealing with IPv6 backends.

Motivation

I was just bitten by #714 which I can confirm, and I'm running a version with this PR in production just fine.

Fixes #714

@ldez ldez changed the title Enclose IPv6 addresses in "[]" (Fixes #714) Enclose IPv6 addresses in "[]" Jun 12, 2018
@ldez ldez added this to the 1.6 milestone Jun 12, 2018
@herver
Copy link
Contributor Author

herver commented Jun 12, 2018

Do you want me to add a test with a config that contains an IPv6 backend to ensure the configuration is consistent ?

@herver herver requested a review from a team as a code owner June 12, 2018 13:10
@ldez ldez changed the base branch from master to v1.6 June 12, 2018 13:10
@ldez
Copy link
Contributor

ldez commented Jun 12, 2018

Thanks for the contribution 👍

I rebased your PR on 1.6 branch.

Could you add tests on getServer method?

@ldez
Copy link
Contributor

ldez commented Jun 12, 2018

@herver you don't need to merge with v1.6 (unless you have conflicts), we have a bot to do it at the end of the review process

The test generates a catalog update containing a dummy service that has two backends:
 - one IPv4
 - one IPv6

We ensure that both of the backend URLs are correctly formatted (as generated by getServer())
@herver
Copy link
Contributor Author

herver commented Jun 12, 2018

@ldez Oh ok :) well I added the test (which might be a bit overkill) but it makes sure that both v4 and v6 backend URLs are generated properly

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

LGTM
:shipit:

Copy link
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

@herver herver deleted the patch-1 branch June 13, 2018 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants