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.

Sign up for GitHub

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

Jump to bottom

protos: add support for go_option package #17460

Merged
merged 15 commits into from
Dec 3, 2021
Merged

protos: add support for go_option package #17460

merged 15 commits into from
Dec 3, 2021

Conversation

remyleone
Copy link
Contributor

@remyleone remyleone commented Jul 23, 2021 •
edited
Loading

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:

2021/07/23 09:46:05 WARNING: Missing 'go_package' option in "envoy/api/v2/core/socket_option.proto",
please specify it with the full Go package path as
a future release of protoc-gen-go will require this be specified.
See https://developers.google.com/protocol-buffers/docs/reference/go-generated#package for more information.

#17082

@repokitteh-read-only
Copy link

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.

🐱

Caused by: #17460 was opened by remyleone.

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/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17460 was opened by remyleone.

see: more, trace.

@remyleone remyleone force-pushed the specify_go_package branch 2 times, most recently from 49cfef3 to 8877df2 Compare July 23, 2021 07:42
@kyessenov
Copy link
Contributor

I think we need this now. go_package appears to be mandatory from now on (talk about v1 stability!). I think this can be a mechanical sed operation to do a global pass, and then we can institute a linter rule requiring go_package. Are you OK with doing the pass? If not, I might have to find some time to do that.

@remyleone
Copy link
Contributor Author

@kyessenov I'm ok to do a pass, how can the linter rule be implemented?

@remyleone
Copy link
Contributor Author

@kyessenov pass done. How can we check it up with a linter?

@yanavlasov
Copy link
Contributor

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

options = descriptor_pb2.FileOptions()

to add go_package option. You can see how it is done for java.

@yanavlasov
Copy link
Contributor

/wait

@remyleone
Copy link
Contributor Author

@yanavlasov done in envoy/tools/protoxform/protoprint.py :)

kyessenov
kyessenov previously approved these changes Aug 6, 2021
View reviewed changes
Copy link
Contributor

@kyessenov kyessenov left a 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.

@markdroth
Copy link
Contributor

/lgtm api
/lgtm v2-freeze

@remyleone
Copy link
Contributor Author

@kyessenov fixed :)

@remyleone
Copy link
Contributor Author

remyleone commented Aug 10, 2021 •
edited
Loading

I think the script ./ci/format_pre.sh cannot be launch from local. I've tried ENVOY_SRCDIR=. ./ci/format_pre.sh but it does not seem to work. I've just run yapf in place and it seems to pass in local

@keith
Copy link
Member

keith commented Aug 11, 2021 •
edited
Loading

Can you try running just the proto formatter locally? ./tools/proto_format/proto_format.sh fix

@yanavlasov
Copy link
Contributor

/wait

remyleone and others added 15 commits December 2, 2021 10:52
@remyleone @keith
protos: add support for go_option package
d8c5d5b 
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>
@keith
bazel: update golang and rules_go
54e663d 
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>
@keith
Fix date
c2d05db 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Apply format
43b10ee 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Bump go version
7bd7659 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
api/envoy/annotations
2c4ed76 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Fix some v3 protos
3afd2ad 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
New protos
05e9a68 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Revert "Fix some v3 protos"
a5bec16 
This reverts commit b21ec23906d82ac19be63436e33d74b2f3a83a3d.

Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Add explicit go package name
601759b 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Force golang packages to be less ambiguous
1c04c4c 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Update tests
cff2ab3 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Format python
3e8dda4 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Remove potential accidental addition
0056fae 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Add another for a new proto
a16e88c 
Signed-off-by: Keith Smiley <keithbsmiley@gmail.com>
@keith
Copy link
Member

keith commented Dec 2, 2021

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17460 (comment) was created by @keith.

see: more, trace.

@kyessenov
Copy link
Contributor

Thanks for slogging through all the protos.

mattklein123
mattklein123 approved these changes Dec 3, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Yay!

@repokitteh-read-only repokitteh-read-only bot removed api deps Approval required for changes to Envoy's external dependencies labels Dec 3, 2021
@mattklein123 mattklein123 merged commit 42f82c2 into envoyproxy:main Dec 3, 2021
@remyleone
Copy link
Contributor Author

Thanks a lot for the reviews and patience :)

@keith keith deleted the specify_go_package branch December 3, 2021 18:01
@keith
Copy link
Member

keith commented Dec 3, 2021

Thanks for all the hard work everyone!

@keith keith mentioned this pull request Dec 3, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moderation moderation moderation left review comments

@keith keith keith left review comments

@markdroth markdroth markdroth left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

@yanavlasov yanavlasov yanavlasov approved these changes

@kyessenov kyessenov kyessenov approved these changes

@dio dio Awaiting requested review from dio

Assignees

@yanavlasov yanavlasov

@kyessenov kyessenov

@markdroth markdroth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.