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.

Sign up for GitHub

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

Jump to bottom

feat: add an option to enable DirectPath xDS #1643

Merged
merged 9 commits into from
May 1, 2023
Merged

feat: add an option to enable DirectPath xDS #1643

merged 9 commits into from
May 1, 2023

Conversation

mohanli-ml
Copy link
Contributor

@mohanli-ml mohanli-ml commented Apr 20, 2023 •
edited
Loading

@mohanli-ml mohanli-ml requested a review from a team as a code owner April 20, 2023 17:23
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Apr 20, 2023
@mohanli-ml mohanli-ml mentioned this pull request Apr 20, 2023
Closed
@mohanli-ml
Copy link
Contributor Author

@blakeli0 Hi Blake, could you take another look? Thanks!

blakeli0
blakeli0 reviewed Apr 25, 2023
View reviewed changes
...ava/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java Outdated
.withHeaders(Collections.<String, String>emptyMap())
.withEndpoint("localhost:8080");

assertThat(provider.needsCredentials()).isTrue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand this test case, does setAttemptDirectPathXds make needsCredentials() to true indirectly?

...ava/gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java Outdated
} else {
provider = provider.withCredentials(CloudShellCredentials.create(3000));
}
assertThat(provider.needsCredentials()).isFalse();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why provider.needsCredentials() is false now?

@blakeli0
Copy link
Collaborator

@blakeli0 Hi Blake, could you take another look? Thanks!

Thanks for reopening the PR in this repo! Looking good other than you may want to make the unit tests clearer.

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Apr 26, 2023
@mohanli-ml
Copy link
Contributor Author

@blakeli0 I have updated the test. PTAL. Thanks!

blakeli0
blakeli0 reviewed Apr 27, 2023
View reviewed changes
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
if (isDirectPathXdsEnv) {
return true;
}
return false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. PTAL. Thanks!

blakeli0
blakeli0 reviewed Apr 27, 2023
View reviewed changes
Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

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.

@blakeli0 blakeli0 added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 27, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 27, 2023
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Apr 27, 2023
@blakeli0 blakeli0 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented May 1, 2023

[gapic-generator-java-root] SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

69.6% 69.6% Coverage
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 1, 2023

[java_showcase_integration_tests] SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

21.7% 21.7% Coverage
0.0% 0.0% Duplication

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 1, 2023

[java_showcase_unit_tests] SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

69.6% 69.6% Coverage
0.0% 0.0% Duplication

@blakeli0 blakeli0 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2023
@blakeli0 blakeli0 added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. labels May 1, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 1, 2023
@blakeli0 blakeli0 merged commit 4054bc6 into googleapis:main May 1, 2023
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label May 1, 2023
@release-please release-please bot mentioned this pull request May 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blakeli0 blakeli0 blakeli0 approved these changes

Assignees
No one assigned
Labels
size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mohanli-ml @blakeli0