-
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 SchemaAdmin #7436
feat(pubsub): implement GUAC for SchemaAdmin #7436
Conversation
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7436 +/- ##
==========================================
+ Coverage 93.63% 93.64% +0.01%
==========================================
Files 1359 1362 +3
Lines 118082 118232 +150
==========================================
+ Hits 110568 110721 +153
+ Misses 7514 7511 -3
Continue to review full report at Codecov.
|
|
||
} // namespace | ||
|
||
std::shared_ptr<pubsub::SchemaAdminConnection> MakeSchemaAdminConnection( |
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.
(Sorry this comment comes a little late (it applies to previous PR's). Also, feel free to ignore it. I'm just throwing an idea out there.)
There is something weird about this method, which is only used for testing. It is used in conjunction with:
google-cloud-cpp/google/cloud/pubsub/schema_admin_connection_test.cc
Lines 42 to 48 in d6ad1b6
std::shared_ptr<SchemaAdminConnection> MakeTestSchemaAdminConnection( | |
std::shared_ptr<pubsub_internal::SchemaStub> mock, Options opts = {}) { | |
opts = pubsub_internal::DefaultCommonOptions( | |
pubsub_testing::MakeTestOptions(std::move(opts))); | |
return pubsub_internal::MakeSchemaAdminConnection(std::move(opts), | |
std::move(mock)); | |
} |
We need it in order to make the
SchemaAdminConnectionImpl
, which is defined in an anonymous namespace. Part of me wants to combine the two functions, but we probably stay more flexible by leaving them separate.
I did get confused just now about where I was in the code. We could name it something like MakeTestSchemaAdminConnection
and s/stub
/mock
/. Or potentially add a comment that says it is only used for testing, and that is why we use the insecure credentials.
Also, I am wondering if we should even be setting the unified credentials here. I actually think it makes more sense to set them in schema_admin_connection_test.cc
. The thing that is testing decides that we are using test credentials. What do you think?
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.
See #7438, let's continue the discussion there.
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.
I am approving. If we think there is value in changing the test-only Make*Connection
methods, we can do so in a future PR that updates them all at once.
Part of the work for #6463
This change is