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

fix: improve warnings for Direct Path xDS set via env #3019

Merged
merged 15 commits into from
Jul 29, 2024
Merged

fix: improve warnings for Direct Path xDS set via env #3019

merged 15 commits into from
Jul 29, 2024

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Jul 8, 2024 •
edited
Loading

Fixes #2427

Context

If GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS is set in the environment aiming to enable Direct Path xDS in another client, we want to just warn the user that if this is intended, it can be a misconfiguration and that the gRPCLB Direct Path setting should be enabled as well.

Approach

As said in the context, we will warn the user that it could be a misconfiguration if the env var setting was meant for the client in question. Other warn cases remain the same.

Coverage

Due to the fact that part of the method in question is tested via a special env var test (not detected by SonarCloud), it will show as uncovered. See this screenshot for coverage of all tests combined
image

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Jul 8, 2024
@diegomarquezp diegomarquezp marked this pull request as ready for review July 8, 2024 21:11
@diegomarquezp diegomarquezp requested a review from a team as a code owner July 8, 2024 21:12
@diegomarquezp diegomarquezp requested a review from mohanli-ml July 10, 2024 13:48
@diegomarquezp diegomarquezp mentioned this pull request Jul 10, 2024
Closed
@diegomarquezp diegomarquezp requested a review from mohanli-ml July 12, 2024 20:07
@diegomarquezp
Copy link
Contributor Author

image
This is because the uncovered case is when it's set via env var, which is tested in a separate test profile.

@diegomarquezp diegomarquezp requested review from blakeli0 and lqiu96 July 18, 2024 15:10
diegomarquezp and others added 3 commits July 25, 2024 10:13
@diegomarquezp @lqiu96
Update gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/Instan…
c71d229 
…tiatingGrpcChannelProvider.java

Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
@diegomarquezp @lqiu96
Update gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/Instan…
4fede21 
…tiatingGrpcChannelProvider.java

Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
@diegomarquezp
improve comments and warning content
ca723e8
@diegomarquezp diegomarquezp requested a review from lqiu96 July 25, 2024 15:06
lqiu96
lqiu96 approved these changes Jul 25, 2024
View reviewed changes
Copy link
Contributor

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

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

LGTM, if you could address the last bit of comments. Thanks!

diegomarquezp and others added 5 commits July 26, 2024 08:29
@diegomarquezp @lqiu96
Update gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/Instan…
cb5c240 
…tiatingGrpcChannelProvider.java

Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
@diegomarquezp @lqiu96
Update gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/Instan…
c7840f8 
…tiatingGrpcChannelProvider.java

Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
@diegomarquezp
reformat
b33009f
@diegomarquezp
add comment to isDirectPathXdsEnabled()
d14e50c
@diegomarquezp
fix test
26b81e1
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed for 'gapic-generator-java-root'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed for 'java_showcase_integration_tests'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
80.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@diegomarquezp diegomarquezp merged commit 7a26115 into main Jul 29, 2024
47 of 49 checks passed
@diegomarquezp diegomarquezp deleted the fix-directpathxds-unwanted-warning branch July 29, 2024 14:00
@release-please release-please bot mentioned this pull request Jul 29, 2024
Merged
mpeddada1 pushed a commit that referenced this pull request Aug 16, 2024
@release-please
chore(main): release 2.44.0 (#3074)
5925503 
🤖 I have created a release *beep* *boop*
---


<details><summary>2.44.0</summary>

##
[2.44.0](v2.43.0...v2.44.0)
(2024-08-16)


### Features

* update ErrorDetails to allow unpacking arbitrary messages
([#3073](#3073))
([6913db5](6913db5))


### Bug Fixes

* Generator callable generation is based on method type
([#3075](#3075))
([c21a013](c21a013))
* improve warnings for Direct Path xDS set via env
([#3019](#3019))
([7a26115](7a26115))


### Dependencies

* update dependency argcomplete to v3.5.0
([#3099](#3099))
([0654a28](0654a28))
* update dependency black to v24.8.0
([#3082](#3082))
([a864f62](a864f62))
* update dependency com.google.crypto.tink:tink to v1.14.1
([#3083](#3083))
([c13b63e](c13b63e))
* update dependency com.google.errorprone:error_prone_annotations to
v2.30.0
([#3100](#3100))
([a10ef54](a10ef54))
* update dependency com.google.errorprone:error_prone_annotations to
v2.30.0
([#3101](#3101))
([9bff64f](9bff64f))
* update dependency lxml to v5.3.0
([#3102](#3102))
([4e145b1](4e145b1))
* update dependency org.apache.commons:commons-lang3 to v3.16.0
([#3103](#3103))
([95c9508](95c9508))
* update dependency org.checkerframework:checker-qual to v3.46.0
([#3081](#3081))
([2431920](2431920))
* update dependency org.easymock:easymock to v5.4.0
([#3079](#3079))
([182ae50](182ae50))
* update dependency pyyaml to v6.0.2
([#3086](#3086))
([f847e45](f847e45))
* update dependency watchdog to v4.0.2
([#3094](#3094))
([f1c75a1](f1c75a1))
* update google api dependencies
([#3071](#3071))
([c5abe90](c5abe90))
* update google auth library dependencies to v1.24.1
([#3109](#3109))
([62acdd6](62acdd6))
* update googleapis/java-cloud-bom digest to a98202d
([#3097](#3097))
([bb216ae](bb216ae))
* update googleapis/java-cloud-bom digest to ad905cc
([#3080](#3080))
([250b26c](250b26c))
* update grpc dependencies to v1.66.0
([#3104](#3104))
([b63b643](b63b643))
* update opentelemetry-java monorepo to v1.41.0
([#3105](#3105))
([7364916](7364916))
* update protobuf to 3.25.4
([#3113](#3113))
([2b271fc](2b271fc))
* update slf4j monorepo to v2.0.16
([#3098](#3098))
([c13f932](c13f932))


### Documentation

* Update the Javadoc of ObsoleteApi.java
([#3088](#3088))
([0ef6619](0ef6619))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mohanli-ml mohanli-ml mohanli-ml approved these changes

@lqiu96 lqiu96 lqiu96 approved these changes

@blakeli0 blakeli0 Awaiting requested review from blakeli0

Assignees
No one assigned
Labels
size: m Pull request size is medium.
Projects
None yet
Milestone
No milestone
3 participants
@diegomarquezp @lqiu96 @mohanli-ml