-
Notifications
You must be signed in to change notification settings - Fork 12
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
Suggestions from code review for gRPC-only feature #708
Conversation
@@ -443,7 +443,7 @@ private function getClientDefaults(): PhpClassMember | |||
|
|||
// TODO: Consolidate setting all the known array values together. | |||
// We do this here to maintain the existing sensible ordering. | |||
if ($this->serviceDetails->transportType !== Transport::REST) { | |||
if ($this->serviceDetails->transportType === Transport::GRPC_REST) { |
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.
reverting this change simply because GapicClientGenerator
is considered "deprecated". But this isn't harming anyone how it was.
@@ -220,6 +220,7 @@ php_gapic_library( | |||
srcs = ["@com_google_googleapis//google/cloud/functions/v1:functions_proto_with_info"], | |||
grpc_service_config = "@com_google_googleapis//google/cloud/functions/v1:functions_grpc_service_config.json", | |||
migration_mode = "NEW_SURFACE_ONLY", | |||
transport = "grpc", |
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.
Moved the "grpc-only" client to be Functions instead of Redis, since now it must be a library marked as NEW_SURFACE_ONLY
@@ -71,12 +71,14 @@ def php_gapic_srcjar( | |||
transport = "grpc+rest" | |||
if transport != "grpc+rest" and transport != "rest" and transport != "grpc": | |||
fail("Error: Only 'grpc+rest', 'rest' or `grpc` transports are supported") | |||
if transport == "grpc" and migration_mode != "NEW_SURFACE_ONLY": | |||
fail("Error: 'grpc' transport is only supported with 'NEW_SURFACE_ONLY' migration mode") |
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.
this ensures that the previous client surfaces (e.g. GapicClientGenerator
, as opposed to GapicClientV2Generator
) are not able to be generated with "grpc-only" transport, for simplicity.
Code review for #707