-
Notifications
You must be signed in to change notification settings - Fork 375
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(pubsub): do not overwrite ConnectionOptions #7406
Conversation
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7406 +/- ##
=======================================
Coverage 93.65% 93.65%
=======================================
Files 1351 1351
Lines 117334 117381 +47
=======================================
+ Hits 109886 109935 +49
+ Misses 7448 7446 -2
Continue to review full report at Codecov.
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
When
PublisherOptions
is constructed, we default all options, including ones covered byConnectionOptions
, for example,GrpcCredentialOption
.google-cloud-cpp/google/cloud/pubsub/publisher_options.cc
Line 25 in c1a44cd
google-cloud-cpp/google/cloud/pubsub/internal/defaults.cc
Line 113 in c1a44cd
This means that when we merge options here ....
google-cloud-cpp/google/cloud/pubsub/publisher_connection.cc
Lines 94 to 100 in c1a44cd
.... we try to merge user-set options contained in
ConnectionOptions
but ignore them because those options all have default values fromPublisherOptions
.(I am saying we, but this is solely @dbolduc's fault)
This is a disaster if users try to authenticate with custom credentials passed to
ConnectionOptions
. Also, the same is true for the Subscriber version of things.To fix the problem, I could have just switched the order of the merge so that the
PublisherOptions
are merged into theConnectionOptions
. But I decided it would be better (and easier to test) thatPublisherOptions
only defaults options from thePublisherOptionList
.This change is