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

Prevent addition of redundant protobuf imports #15589

Open
phlax opened this issue Mar 20, 2021 · 2 comments
Open

Prevent addition of redundant protobuf imports #15589

phlax opened this issue Mar 20, 2021 · 2 comments
Assignees
Labels

Comments

@phlax
Copy link
Member

phlax commented Mar 20, 2021

description

we have a lot of redundant import warnings when generating protos

i have raised a pr to fix this upstream in cncf/xds (cncf/xds#3)

i have previously removed them from the envoy codebase (#15329 ) and have just opened another PR to remove more recently added ones (#15588 )

it would be great if we prevented these from being added

the obvious way would be to turn warnings into errors (see protocolbuffers/protobuf#3980)

the problem with this is that there are a lot of warnings about missing dir paths which i think is harder to fix (bazelbuild/bazel#7157)

@phlax phlax added the triage Issue requires triage label Mar 20, 2021
@mattklein123 mattklein123 added area/build help wanted Needs help! and removed triage Issue requires triage labels Mar 23, 2021
@phlax
Copy link
Member Author

phlax commented Apr 28, 2021

im wondering if one of the protobuf linters would pick this up (and perhaps improve proto formatting in general)

im also seeing that the problem is in part because of the guidance here - https://github.com/envoyproxy/envoy/blob/main/api/STYLE.md#adding-an-extension-configuration-to-the-api

cc @htuch

@phlax phlax self-assigned this Apr 28, 2021
@htuch
Copy link
Member

htuch commented Apr 29, 2021

It should be lintable by something that understands proto descriptors. I think we do something like this in the tooling to add/remove imports, but it would be better to rely on standard tooling if possible; updating the STYLE.md would help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants