api: rename connection pool proto messages #13378
Conversation
The java_outer_classname must not collide with a type defined within a proto file in order to compile protos to Java. Additionally, this commit introduces a format check to prevent this from happening again. Signed-off-by: Spencer Lewis <[email protected]>
| @@ -14,5 +14,5 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |||
| // A connection pool which forwards downstream HTTP as TCP or HTTP to upstream, | |||
| // based on CONNECT configuration. | |||
| // [#extension: envoy.upstreams.http.generic] | |||
| message GenericConnectionPoolProto { | |||
| message GenericConnectionPool { | |||
There was a problem hiding this comment.
This is a breaking API change, see https://github.com/envoyproxy/envoy/blob/master/api/API_VERSIONING.md#backwards-compatibility. Can you change the Java outer classname?
There was a problem hiding this comment.
Oh no. Yeah I'll do that instead.
There was a problem hiding this comment.
@spenceral you'll run into the issue that these are auto-generated by the protoformat tool based on the filename. You'll need to maybe update that with a special case.
Signed-off-by: Spencer Lewis <[email protected]>
| # This is a workaround for Java outer class names that would otherwise | ||
| # conflict with types defined within the same proto file, see | ||
| # https://github.com/envoyproxy/envoy/pull/13378. | ||
| options.java_outer_classname += "OuterClass" |
There was a problem hiding this comment.
I chose OuterClass because I think it is how protoc handles this case if you don't set the java_outer_classname option https://developers.google.com/protocol-buffers/docs/reference/java-generated
With this protoprint change, we don't actually need the check_format error. WDYT @htuch ?
There was a problem hiding this comment.
Yeah, I agree, the check format isn't needed. I kind of don't like the special case, but my working assumption is if we change every Java outer classname, this will force a lot of churn in folks who are consuming the APIs. Does that sound right? If so, I think this is the best we can do for now. Maybe leave a comment saying "TODO: In next major version, make this consistent.".
There was a problem hiding this comment.
Does that sound right?
That's my understanding as well. All import statements and/or fully qualified class names would need to updated. I'll leave the comment.
Signed-off-by: Spencer Lewis <[email protected]>
The java_outer_classname must not collide with a type defined within a proto
file in order to compile protos to Java. Additionally, this commit
introduces a format check to prevent this from happening again.
Signed-off-by: Spencer Lewis [email protected]
Risk Level: low
Testing: none
Docs Changes:none
Release Notes: none
Fixes #13368