[configgrpc] Deprecate GRPCServerSettings, use ServerConfig instead #9403
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #9403 +/- ##
=======================================
Coverage 90.46% 90.46%
=======================================
Files 344 344
Lines 18024 18024
=======================================
Hits 16306 16306
Misses 1390 1390
Partials 328 328 ☔ View full report in Codecov by Sentry. |
config/configgrpc/configgrpc.go
Outdated
| type GRPCServerSettings = GRPCServerConfig | ||
|
|
||
| // GRPCServerConfig defines common settings for a gRPC server configuration. | ||
| type GRPCServerConfig struct { |
There was a problem hiding this comment.
qq: Should we also make this ServerConfig since it will be used as configgrpc.ServerConfig? I personally like the shorter name.
There was a problem hiding this comment.
Even configgrpc.Server seems clear enough to me, since config is also redundant.
There was a problem hiding this comment.
I like configgrpc.ServerConfig. configgrpc.Server sounds a bit unclear.
@open-telemetry/collector-approvers WDYT?
We already did this change to confighttp. If we think that it's a good option we should drop HTTP from HTTPServerConfig and HTTPServerConfig before the next release
There was a problem hiding this comment.
ServerConfig makes more sense to me, it's weird to have a ToServer method on a struct called Server
There was a problem hiding this comment.
I also like configgrcp.ServerConfig
There was a problem hiding this comment.
That an implementation detail for that receiver not for this thread.
There was a problem hiding this comment.
It's actually part of this PR so we need to decide
There was a problem hiding this comment.
Pushing GRPCServerConfig -> ServerConfig now.
There was a problem hiding this comment.
Oh, I was wrong this doesn't introduce the breaking chance since ServerConfig isn't embedded in this receiver. So no more changes are needed
There was a problem hiding this comment.
I pushed a PR to update confighttp accordingly #9453
dmitryax
added
the
release:blocker
|
Putting the blocker label to avoid breaking the merged HTTP configs again after the discussion is resolved |
atoulme
changed the title
atoulme
force-pushed
the
grpcserversettings_grpcserverconfig
branch
from
29b4926
to
98f9962
Compare
Following discussion #9403 (comment)
Description:
Deprecate GRPCServerSettings, use
GRPCServerConfig insteadLink to tracking Issue:
#6767