istio / old_pilot_repo

Deprecated home of Istio's Pilot, now in istio/istio's pilot dir
Apache License 2.0
137 stars 91 forks source link

let getDefaultNamespace() returns "default" if nothing is found. #1666

Closed jmuk closed 6 years ago

jmuk commented 6 years ago

https://github.com/istio/issues/issues/110 reminds me that istioctl can set empty namespace for mixer configs, which will be delivered directly and therefore it can cause same error of "empty namespace".

When the obtained namespace happens to be empty, I think it should return the default value.

(cherry picked from commit c67bb1450974fa8916f74862dee5c3f8f37f0edf)

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

none
ldemailly commented 6 years ago

/lgtm

istio-testing commented 6 years ago

@ldemailly: changing LGTM is restricted to assignees, and assigning you to the PR failed.

In response to [this](https://github.com/istio/pilot/pull/1666#issuecomment-340927253): >/lgtm Instructions for interacting with me using PR comments are available [here](https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
ldemailly commented 6 years ago

/lgtm

andraxylia commented 6 years ago

/lgtm

istio-testing commented 6 years ago

@andraxylia: changing LGTM is restricted to assignees, and assigning you to the PR failed.

In response to [this](https://github.com/istio/pilot/pull/1666#issuecomment-340927769): >/lgtm > Instructions for interacting with me using PR comments are available [here](https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
jmuk commented 6 years ago

/assign @andraxylia

ldemailly commented 6 years ago

/ok-to-test

yutongz commented 6 years ago

/retest

codecov[bot] commented 6 years ago

Codecov Report

Merging #1666 into release-0.2 will increase coverage by 0.04%. The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           release-0.2    #1666      +/-   ##
===============================================
+ Coverage        82.69%   82.74%   +0.04%     
===============================================
  Files               52       52              
  Lines             6426     6426              
===============================================
+ Hits              5314     5317       +3     
+ Misses             909      907       -2     
+ Partials           203      202       -1
Impacted Files Coverage Δ
platform/consul/monitor.go 83.33% <0%> (+3.57%) :arrow_up:

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 25d7796...1d20f1c. Read the comment docs.

sebastienvas commented 6 years ago

/retest

sebastienvas commented 6 years ago

/retest

yutongz commented 6 years ago

I triggered it with mkpj, but not sure why it was not being triggered at first place.