Skip to content

protos: add support for go_option package#17460

Merged
mattklein123 merged 15 commits intoenvoyproxy:mainfrom
remyleone:specify_go_package
Dec 3, 2021
Merged

protos: add support for go_option package#17460
mattklein123 merged 15 commits intoenvoyproxy:mainfrom
remyleone:specify_go_package

Conversation

@remyleone
Copy link
Contributor

@remyleone remyleone commented Jul 23, 2021

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.

@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:

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
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

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

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

@yanavlasov
Copy link
Contributor

/wait

@remyleone
Copy link
Contributor Author

@keith done :) It modified a lot of files in generated_api_shadow and in API. Is this command documented somewhere?

@keith
Copy link
Member

keith commented Aug 14, 2021

Hrm looks like formatting failed just for 3 files this time, maybe for unrelated changes. I guess it might be ideal if you could only update the format of the files you intentionally changed, but I'm not sure how folks normally do this if others did need updates. Regardless maybe you can fix those 3 files and be good here?

I'm not sure if it's documented or what the normal workflow is here I just found that in the script you tried to run and am hoping that should be all we need

@keith
Copy link
Member

keith commented Aug 17, 2021

So this looks like an actually related failure?

@phlax
Copy link
Member

phlax commented Aug 24, 2021

hi @remyleone looks like this will need a rebase

@repokitteh-read-only
Copy link

🙀 Error while processing event:

evaluation error
error: context deadline exceeded
🐱

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

see: more, trace.

remyleone and others added 15 commits December 2, 2021 10:52
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>
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>
@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.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.