-
Notifications
You must be signed in to change notification settings - Fork 37
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.
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
Adding explicit tls roots configuration to stackdriver #96
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
=======================================
+ Coverage 52.3% 56.4% +4.1%
=======================================
Files 38 39 +1
Lines 4967 5446 +479
=======================================
+ Hits 2598 3074 +476
- Misses 2369 2372 +3 ☔ View full report in Codecov by Sentry. |
So the only point of discussion I see is that with this fix: If both webpki-roots and native-roots features are enabled then here I default to webpki roots. I don't know if this is different from the previous tonic implicit behavior. Another option would be to error out if both are enabled. |
I see a generated_code_is_fresh test is failing, what should I do to fix this? |
Are there any tests that need to be written for this? I have been using this in my own project for the past month and it has resolved the errors for me. |
I think the changes are trivial, and we can add the tests afterwards if required. |
Could we get a release for this? |
There is otel-rust release planned during this week. We can have release from contrib repo subsequently. |
Fixes #95
Changes
When upgrading to tonic 0.12, tonic stopped implicitly setting up tls roots even when the features are enabled for it. This PR makes this setup explicit in stackdriver.
It doesn't change user-facing API and the change is relatively trivial. I manually tested this in my repo and the error went away. I don't think there is a good way to unit test this unless we want the unit tests to verify connectivity to gcp servers.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes