Skip to content
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

Remove option java_generic_services from proto files #25298

Merged
merged 1 commit into from
Feb 9, 2023

Conversation

scrocquesel
Copy link
Contributor

Commit Message: Remove option java_generic_services from proto files
Additional Description: Generic services are deprecated since protoc version 2.4.0 (2010). Protoc plugins that generates code may require that generic services are disabled, so that they can generate their own classes of the same name.
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
Optional Fixes #25172

@repokitteh-read-only
Copy link

Hi @scrocquesel, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #25298 was opened by scrocquesel.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #25298 was opened by scrocquesel.

see: more, trace.

@scrocquesel scrocquesel force-pushed the remove_java_generic_services branch from e06e71e to d1b439b Compare February 1, 2023 21:32
@htuch
Copy link
Member

htuch commented Feb 6, 2023

Will need to update https://github.com/envoyproxy/envoy/blob/main/tools/protoprint/protoprint.py. @phlax since you responded to the original issue, do you want to take this PR?

@phlax
Copy link
Member

phlax commented Feb 6, 2023

@scrocquesel thanks for raising this

i think these 2 lines

if file_proto.service:
options.java_generic_services = True
need to be removed

it might also need to be removed from tools/testdata/protoxform/envoy/v2/discovery_service.proto.active_or_frozen.gold

Signed-off-by: Sébastien CROCQUESEL <88554524+scrocquesel@users.noreply.github.com>
@scrocquesel scrocquesel force-pushed the remove_java_generic_services branch from d1b439b to 23daea4 Compare February 6, 2023 10:48
@phlax
Copy link
Member

phlax commented Feb 6, 2023

(as a side point - its generally better not to force push unless there is a reason - ie use merge over rebase and separate commits etc - it doesnt matter so much at this point - but its generally frowned upon)

@scrocquesel
Copy link
Contributor Author

scrocquesel commented Feb 6, 2023

@phlax Yes, I have only searched the directive in proto files.

@phlax
Copy link
Member

phlax commented Feb 6, 2023

/retest

1 similar comment
@phlax
Copy link
Member

phlax commented Feb 7, 2023

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:

🐱

Caused by: a #25298 (comment) was created by @phlax.

see: more, trace.

Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks @scrocquesel

cc @htuch @adisuissa for final api stamp

@repokitteh-read-only repokitteh-read-only bot removed the api label Feb 9, 2023
@htuch htuch merged commit baec129 into envoyproxy:main Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove java_generic_services option from service proto files
4 participants