Skip to content

api: add proto options for java#5369

Merged
ggreenway merged 8 commits intoenvoyproxy:masterfrom
dapengzhang0:add-java-options
Jan 2, 2019
Merged

api: add proto options for java#5369
ggreenway merged 8 commits intoenvoyproxy:masterfrom
dapengzhang0:add-java-options

Conversation

@dapengzhang0
Copy link
Copy Markdown
Contributor

@dapengzhang0 dapengzhang0 commented Dec 20, 2018 •
edited
Loading

Description:
Without the proto option java_package, the proto java codegen will generate classes with a "default package". Take the cds.proto file for example, in envoyproxy/java-control-plane, it will generate a java class envoy.api.v2.Cds.Cluster. But java packages normally starts with a domain name. io.envoyproxy.envoy.api.v2.Cds.Cluster would be better for java convention. Another proto option java_multiple_files = true is also recommended for most cases.

The PR is generated by running:

cd api
for f in $(find . -name "*.proto")
do
  sed -i -e '/^option\sjava_multiple_files\s=/d' $f
  sed -i -e "/^package\s/a option java_multiple_files = true;" $f
  sed -i -e '/^option\sjava_package\s=/d' $f
  cmd='grep -Po "^package \K.*(?=;$)" '"${f}"
  sed -i -e "/^package\s/a option java_package = \"io.envoyproxy.$(eval $cmd)\";" $f
done

Risk Level: Medium
Note that e.g. envoyproxy/java-control-plane will have to do a refactoring when they sync the proto files next time.
Testing: N/A
Docs Changes: N/A
Release Notes: added proto options for java

@dapengzhang0
api: add proto options for java
1e82546 
Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>
@ggreenway
Copy link
Copy Markdown
Member

ggreenway commented Dec 20, 2018

@snowp Since you have most of the recent commits in java-control-plane, can you review this?

@ggreenway ggreenway requested a review from snowp December 20, 2018 21:51
snowp
snowp suggested changes Dec 20, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Adding these options seems fine to me. Would it be possible to add something to CI that ensures that we do this for all future protos so that we keep this consistent? Maybe just a simple script we can run in the bazel.api CI task?

@dapengzhang0
add ci for checking proto options
b74551d 
Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>
ggreenway
ggreenway requested changes Dec 21, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Also, please don't force-push to the PR branch; it makes it more difficult to review the incremental diffs. We'll squash-merge the PR when it's ready, so it's easier for the maintainers if you leave all the intermediate commits. Thanks!

@dapengzhang0
Copy link
Copy Markdown
Contributor Author

dapengzhang0 commented Dec 21, 2018

@ggreenway Thank you for your suggestions. The force push was because I forgot --signoff for the commit. I do have incremental diffs.

@ggreenway
Copy link
Copy Markdown
Member

ggreenway commented Dec 21, 2018

The force push was because I forgot --signoff for the commit.

Ok, that makes sense. Carry on :)

@dapengzhang0
Revert "add ci for checking proto options"
c12feff 
This reverts commit b74551d.

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>
@dapengzhang0
check_format for java proto options
5000e4c 
Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>
@dapengzhang0
fix python format for check_format.py
59b795a 
Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>
@snowp
Copy link
Copy Markdown
Contributor

snowp commented Dec 22, 2018

You will need to merge master to pick up a fix for mac build failure

snowp
snowp suggested changes Dec 22, 2018
View reviewed changes
tools/check_format.py Outdated
java_package_correct = True
if java_multiple_files and java_package_correct:
return []
return ["Java proto options are not set correctly for file: %s" % file_path]
Copy link
Copy Markdown
Contributor

@snowp snowp Dec 22, 2018

Choose a reason for hiding this comment

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

Can we output what options are missing? Would save people from having to grep around to figure out what's wrong

Copy link
Copy Markdown
Contributor Author

@dapengzhang0 dapengzhang0 Jan 2, 2019

Choose a reason for hiding this comment

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

Added more detailed error messages.

htuch
htuch reviewed Dec 24, 2018
View reviewed changes
api/envoy/api/v2/cds.proto
syntax = "proto3";

package envoy.api.v2;
option java_package = "io.envoyproxy.envoy.api.v2";
Copy link
Copy Markdown
Member

@htuch htuch Dec 24, 2018

Choose a reason for hiding this comment

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

Out of curiosity, is there no way to have the Java protoc plugin take the organizational package prefix (in this case 'io.envoyproxy) as a CLI arguement and prefix it automatically? It seems this would be a common case thing to want to do.

@stale
Copy link
Copy Markdown

stale bot commented Dec 31, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 31, 2018
@dapengzhang0
Merge branch 'master' of https://github.com/envoyproxy/envoy into add…
5ca513c 
…-java-options

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>
@dapengzhang0
more detailed error messages
5f7a826 
Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 2, 2019
@dapengzhang0
reformat check_format.py
88bc414 
Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>
snowp
snowp approved these changes Jan 2, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

If you have a chance to update java-control-plane when this lands that'd be awesome, if not no worries

@ggreenway ggreenway merged commit 02659d4 into envoyproxy:master Jan 2, 2019
@dapengzhang0
Copy link
Copy Markdown
Contributor Author

dapengzhang0 commented Jan 3, 2019

Thank you all for the review. @snowp, I sent out a PR envoyproxy/java-control-plane#86 to update java-control-plane.

If you have a chance to update java-control-plane when this lands that'd be awesome, if not no worries

@snowp snowp mentioned this pull request Jan 4, 2019
Closed
@dapengzhang0 dapengzhang0 deleted the add-java-options branch February 8, 2019 18:29
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
@dapengzhang0 @fredlas
api: add proto options for java (envoyproxy#5369)
99f9604 
* api: add proto options for java
* add ci for checking proto options

Signed-off-by: Penn (Dapeng) Zhang <zdapeng@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggreenway ggreenway ggreenway approved these changes

+2 more reviewers

@htuch htuch htuch left review comments

@snowp snowp snowp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@snowp snowp

@ggreenway ggreenway

@htuch htuch

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dapengzhang0 @ggreenway @snowp @htuch