Closed dbolduc closed 10 months ago
PopulateCommonOptions
should really be setting theUnifiedCredentialOption
Note to self: We can't do this. We need to maintain backwards compatibility with GrpcCredentialOption
, which does not exist in common
. In PopulateGrpcOptions
we cannot know whether the UnifiedCredentialOption
was set by the application or by the library as a default. So we cannot know whether we should use that or GrpcCredentialOption
when both are set.
It might be a good idea to compose the gRPC / REST default functions. Such as:
Options PopulateGrpcOptions(Options o, ...) {
if (!o.has<GrpcOption>()) o.set<GrpcOption>("default");
o = PopulateCommonOptions(std::move(o), ...);
return o;
}
I am closing this. Defaulting REST options is still weird, but I have tasks to do that have deadlines. This is not one of them.
Some weirdness here
PopulateGrpcOptions
is wrong. It talks about common options, not gRPC options.rest_internal::LongrunningEndpointOption
is an internal option? Should customers be able to set this? It is only ever used for gRPC transcoding in the spanner admin rest stubs. (Most REST services use their own endpoint for their own custom LROs).PopulateCommonOptions
should really be setting theUnifiedCredentialOption
. But it is handled differently in gRPC vs. REST. In gRPC, we default theGrpcCredentialOption
. REST defaults theUnifiedCredentialOption
and usesMakeAuthOptions(...)
. I think we can use one code path for both.GOOGLE_CLOUD_CPP_TRACING_OPTIONS
forRestTracingOptionsOption
.RestTracingOptionsOption
andGrpcTracingOptionsOption
are the same thing. There should only be one type incommon
. There is a separate proposal on how to resolve this. It is out of scope for this issue.