Add ipv6 support.#381
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
drubin
left a comment
drubin
left a comment
There was a problem hiding this comment.
Tests don't seem to pass so not going to approve. But in general besides the improvements the code looks good.
src/config.ts
Outdated
|
|
||
| // Wrap raw IPv6 addresses in brackets. | ||
| let serverHost = host; | ||
| if (host && host.indexOf(':') != -1) { |
There was a problem hiding this comment.
There are methods to check if its an ipv6 Adress.
| name: clusterName, | ||
| caFile: `${pathPrefix}${Config.SERVICEACCOUNT_CA_PATH}`, | ||
| server: `${scheme}://${host}:${port}`, | ||
| server: `${scheme}://${serverHost}:${port}`, |
There was a problem hiding this comment.
Maybe we should also be building up the address with Uri helpers? Maybe they can also handle this natively?
There was a problem hiding this comment.
I couldn't tell from docs if they handled this or not, so I left it as is.
| if (!cluster) { | ||
| return; | ||
| } | ||
| expect(cluster.server).to.equal('http://[::1234:5678]:80'); |
|
Comments addressed, style fixed, please take another look. Thanks! |
k8s-ci-robot
added
the
lgtm
3 participants
Fixes #380
cc @liggett @thockin