feat: add an option to enable DirectPath xDS#1643
Conversation
|
@blakeli0 Hi Blake, could you take another look? Thanks! |
| .withHeaders(Collections.<String, String>emptyMap()) | ||
| .withEndpoint("localhost:8080"); | ||
|
|
||
| assertThat(provider.needsCredentials()).isTrue(); |
There was a problem hiding this comment.
I'm not sure I understand this test case, does setAttemptDirectPathXds make needsCredentials() to true indirectly?
| } else { | ||
| provider = provider.withCredentials(CloudShellCredentials.create(3000)); | ||
| } | ||
| assertThat(provider.needsCredentials()).isFalse(); |
There was a problem hiding this comment.
Why provider.needsCredentials() is false now?
Thanks for reopening the PR in this repo! Looking good other than you may want to make the unit tests clearer. |
|
@blakeli0 I have updated the test. PTAL. Thanks! |
| if (isDirectPathXdsEnv) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Can we add one more test case for the default value? Our sonar check is failing because the new code is lacking coverage, I know we cannot set environment variable in unit tests, but we should be able to test the default case.
There was a problem hiding this comment.
Added. PTAL. Thanks!
blakeli0
left a comment
There was a problem hiding this comment.
LGTM other than one more test case needed. I'll be off tomorrow so I'll approve it first, feel free to merge it once the test case is added.
|
[gapic-generator-java-root] SonarCloud Quality Gate failed. |
|
[java_showcase_integration_tests] SonarCloud Quality Gate failed. |
|
[java_showcase_unit_tests] SonarCloud Quality Gate failed. |
Original PR: googleapis/gax-java#1968