-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
protos: add support for go_option package #17460
Conversation
Hi @remyleone, 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. |
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
49cfef3
to
8877df2
Compare
I think we need this now. |
@kyessenov I'm ok to do a pass, how can the linter rule be implemented? |
8877df2
to
294d700
Compare
@kyessenov pass done. How can we check it up with a linter? |
f709ee2
to
c71515b
Compare
Thanks you for doing this forklift change. I think the linter needs to be fixed here: envoy/tools/protoxform/protoprint.py Line 206 in 79ade4a
to add go_package option. You can see how it is done for java. |
/wait |
c71515b
to
b9319bb
Compare
@yanavlasov done in envoy/tools/protoxform/protoprint.py :) |
f76e86b
to
41cf6c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the massive cleanup. There's a syntax linter in python script that needs a fix, but otherwise, everything LGTM.
/lgtm api |
@kyessenov fixed :) |
I think the script |
Can you try running just the proto formatter locally? |
/wait |
Add a linter pass for go_package Fix with ./tools/proto_format/proto_format.sh fix Signed-off-by: Rémy Léone <rleone@scaleway.com>
These updates are required for things to work nicely when building on Apple Silicon machines targeting darwin_arm64. Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
This reverts commit b21ec23906d82ac19be63436e33d74b2f3a83a3d. Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
cf46ad8
to
a16e88c
Compare
/retest |
Retrying Azure Pipelines: |
Thanks for slogging through all the protos. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yay!
Thanks a lot for the reviews and patience :) |
Thanks for all the hard work everyone! |
Hello, I would like first to see if this change is desired before specifying the go_package option in all the files. If so, I will gladly specify it everywhere. :)
For reference when I use envoy in buf I get the following warnings:
#17082