Allow setting multiple path in KUBECONFIG by qiujian16 · Pull Request #4138 · istio/istio

qiujian16 · 2018-03-09T09:26:23Z

Change to user default loading rules that will automatically load config in KUBECONFIG environment variable.

fix #4081

istio-merge-robot · 2018-03-09T09:27:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: costinm

Assign the PR to them by writing /assign @costinm in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

pilot/OWNERS

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

baodongli · 2018-03-09T21:43:21Z

pilot/pkg/serviceregistry/kube/client.go

This is used by both the pilot server and istioctl. I believe that the issue you are trying to fix is from istioctl. I don't think pilot is ready to support this yet.

@baodongli I do not think there is difference between pilot server and istioctl...if pilot server does not specify kubeconfig, in cluster config will be used. This is to ensure that the kubeconfig loaded by istioctl is the same as what is loaded by kubernetes client.

gyliu513 · 2018-03-12T03:33:06Z

pilot/pkg/serviceregistry/kube/client.go

Can you put this to a helper function name as ResolveMultiPathConfig or some others

gyliu513 · 2018-03-12T03:33:49Z

pilot/pkg/serviceregistry/kube/client.go

Add a new test for this in controller_test.go？

codecov · 2018-03-12T07:08:30Z

Codecov Report

Merging #4138 into master will decrease coverage by 1%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #4138    +/-   ##
=======================================
- Coverage      71%     71%   -<1%     
=======================================
  Files         324     317     -7     
  Lines       29547   28882   -665     
=======================================
- Hits        20763   20218   -545     
+ Misses       7531    7443    -88     
+ Partials     1253    1221    -32

Impacted Files	Coverage Δ
mixer/adapter/solarwinds/log_handler.go	`50% <0%> (-7%)`	⬇️
pilot/pkg/kube/inject/webhook.go	`77% <0%> (-4%)`	⬇️
mixer/adapter/redisquota/redisquota.go	`86% <0%> (-3%)`	⬇️
mixer/adapter/opa/opa.go	`79% <0%> (-3%)`	⬇️
mixer/adapter/list/list.go	`100% <0%> (ø)`	⬇️
mixer/adapter/list/ipList.go	`100% <0%> (ø)`	⬆️
mixer/adapter/memquota/keys.go	`100% <0%> (ø)`	⬆️
mixer/adapter/memquota/dedup.go	`100% <0%> (ø)`	⬆️
mixer/adapter/solarwinds/solarwinds.go	`0% <0%> (ø)`	⬆️
mixer/adapter/dogstatsd/dogstatsd.go	`100% <0%> (ø)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8eb6288...08b784c. Read the comment docs.

gyliu513 · 2018-03-12T08:20:32Z

pilot/pkg/serviceregistry/kube/client.go

Why not put the in-cluster logic to LoadConfigFromMultiPath?

gyliu513 · 2018-03-12T08:21:09Z

pilot/pkg/serviceregistry/kube/client_test.go

+1 to create a new test file.

gyliu513 · 2018-03-12T14:18:33Z

/ok-to-test

qiujian16 · 2018-03-14T05:04:28Z

/retest

istio-merge-robot · 2018-04-12T08:01:42Z

@qiujian16 PR needs rebase

stale · 2018-06-23T08:34:15Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2018-07-07T08:56:21Z

This pull request has been automatically closed because it has not had activity in the last month. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

qiujian16 requested a review from a team March 9, 2018 09:26

googlebot added the cla: yes label Mar 9, 2018

istio-testing added the needs-ok-to-test label Mar 9, 2018

baodongli reviewed Mar 9, 2018

View reviewed changes

gyliu513 reviewed Mar 12, 2018

View reviewed changes

pilot/pkg/serviceregistry/kube/client.go Outdated

Copy link
Copy Markdown

Member

gyliu513 Mar 12, 2018

Choose a reason for hiding this comment

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

Why not put the in-cluster logic to LoadConfigFromMultiPath?

gyliu513 reviewed Mar 12, 2018

View reviewed changes

pilot/pkg/serviceregistry/kube/client_test.go Outdated

Copy link
Copy Markdown

Member

gyliu513 Mar 12, 2018

Choose a reason for hiding this comment

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

+1 to create a new test file.

istio-testing removed the needs-ok-to-test label Mar 12, 2018

qiujian16 added 2 commits March 19, 2018 10:12

Allow setting multiple path in KUBECONFIG

f1ebc26

Add test cases

08b784c

istio-merge-robot added the needs-rebase Indicates a PR needs to be rebased before being merged label Apr 12, 2018

ymesika mentioned this pull request May 29, 2018

Fixed istioctl to be able to deal with multiple paths in KUBECONFIG #5881

Merged

stale bot added the stale label Jun 23, 2018

stale bot closed this Jul 7, 2018

Conversation

qiujian16 commented Mar 9, 2018

Uh oh!

istio-merge-robot commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyliu513 commented Mar 12, 2018

Uh oh!

qiujian16 commented Mar 14, 2018

Uh oh!

istio-merge-robot commented Apr 12, 2018

Uh oh!

stale bot commented Jun 23, 2018

Uh oh!

stale bot commented Jul 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Mar 12, 2018 •

edited

Loading