-
Notifications
You must be signed in to change notification settings - Fork 382
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
feat(pubsub): implement GUAC for SubscriptionAdmin #7432
feat(pubsub): implement GUAC for SubscriptionAdmin #7432
Conversation
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7432 +/- ##
==========================================
+ Coverage 93.61% 93.63% +0.01%
==========================================
Files 1356 1359 +3
Lines 117762 118117 +355
==========================================
+ Hits 110248 110602 +354
- Misses 7514 7515 +1
Continue to review full report at Codecov.
|
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.
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @coryan)
google/cloud/pubsub/subscription_admin_connection.cc, line 254 at r1 (raw file):
}; // Decorates a TopicAdminStub. This works for both mock and real stubs.
s/Topic/Subscription/
google/cloud/pubsub/subscription_admin_connection.cc, line 314 at r1 (raw file):
Quoted 9 lines of code…
auto auth = [&] { if (opts.has<google::cloud::UnifiedCredentialsOption>()) { return google::cloud::internal::CreateAuthenticationStrategy( opts.get<google::cloud::UnifiedCredentialsOption>(), background->cq(), opts); } return google::cloud::internal::CreateAuthenticationStrategy( opts.get<google::cloud::GrpcCredentialOption>()); }();
Note to selves: this logic appears in a lot of places. There is an opportunity to refactor.
google/cloud/pubsub/integration_tests/subscription_admin_integration_test.cc, line 216 at r1 (raw file):
Quoted 7 lines of code…
if (UsingEmulator()) { options = Options{} .set<UnifiedCredentialsOption>(MakeAccessTokenCredentials( "test-only-invalid", std::chrono::system_clock::now() + std::chrono::minutes(15))) .set<internal::UseInsecureChannelOption>(true); }
Does it make sense to move these emulator-only values into here?
google-cloud-cpp/google/cloud/pubsub/internal/defaults.cc
Lines 42 to 46 in 1a2ddfb
auto emulator = internal::GetEnv("PUBSUB_EMULATOR_HOST"); | |
if (emulator.has_value()) { | |
opts.set<EndpointOption>(*emulator).set<GrpcCredentialOption>( | |
grpc::InsecureChannelCredentials()); | |
} |
It seems weird that we default GrpcCredentialOption
but not UnifiedCredentialsOption
. (Maybe we shouldn't be setting either in defaults.cc
)
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
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.
Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @dbolduc)
google/cloud/pubsub/subscription_admin_connection.cc, line 254 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
s/Topic/Subscription/
Fixed.
google/cloud/pubsub/subscription_admin_connection.cc, line 314 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
auto auth = [&] { if (opts.has<google::cloud::UnifiedCredentialsOption>()) { return google::cloud::internal::CreateAuthenticationStrategy( opts.get<google::cloud::UnifiedCredentialsOption>(), background->cq(), opts); } return google::cloud::internal::CreateAuthenticationStrategy( opts.get<google::cloud::GrpcCredentialOption>()); }();
Note to selves: this logic appears in a lot of places. There is an opportunity to refactor.
google/cloud/pubsub/integration_tests/subscription_admin_integration_test.cc, line 216 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
if (UsingEmulator()) { options = Options{} .set<UnifiedCredentialsOption>(MakeAccessTokenCredentials( "test-only-invalid", std::chrono::system_clock::now() + std::chrono::minutes(15))) .set<internal::UseInsecureChannelOption>(true); }
Does it make sense to move these emulator-only values into here?
google-cloud-cpp/google/cloud/pubsub/internal/defaults.cc
Lines 42 to 46 in 1a2ddfb
auto emulator = internal::GetEnv("PUBSUB_EMULATOR_HOST"); if (emulator.has_value()) { opts.set<EndpointOption>(*emulator).set<GrpcCredentialOption>( grpc::InsecureChannelCredentials()); } It seems weird that we default
GrpcCredentialOption
but notUnifiedCredentialsOption
. (Maybe we shouldn't be setting either indefaults.cc
)
We probably should switch to setting the unified credentials in this case. #7434
Ah, yes. That makes more sense. |
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Part of the work for #6463
This change is