-
Notifications
You must be signed in to change notification settings - Fork 374
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
fix: restore credential logic #12690
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #12690 +/- ##
=======================================
Coverage 93.58% 93.58%
=======================================
Files 2069 2069
Lines 183018 183044 +26
=======================================
+ Hits 171276 171304 +28
+ Misses 11742 11740 -2
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some obvious refactoring needed. This predates your PR though, so approved as-is.
@@ -91,7 +92,8 @@ std::shared_ptr<$stub_rest_class_name$> | |||
CreateDefault$stub_rest_class_name$(Options const& options) { | |||
Options opts = options; | |||
if (!opts.has<UnifiedCredentialsOption>()) { | |||
opts.set<UnifiedCredentialsOption>(MakeGoogleDefaultCredentials(options)); | |||
opts.set<UnifiedCredentialsOption>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like all the code generated here could go into a common function in google::cloud::internal
or google::cloud::rest_internal
. There is nothing specific to this stub from lines 93 to 107.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack
Modifies #12671 and #12672 to only forward the OTel tracing option in cases where the library is responsible for creating the default credentials.
Passing extra options was breaking at least authorized user credential authentication, and possibly more.
This change is