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

forward fix extensions being depended on by core code/tests #20589

Closed
alyssawilk opened this issue Mar 30, 2022 · 2 comments · Fixed by #20590
Closed

forward fix extensions being depended on by core code/tests #20589

alyssawilk opened this issue Mar 30, 2022 · 2 comments · Fixed by #20590
Assignees
@alyssawilk
Labels
enhancement Feature requests. Not bugs or questions.

Comments

@alyssawilk
Copy link
Contributor

so way back when I fixed the default visibility rules so extensions were in general not visible to core code and non-extension tests.

That said, it relies on maintainers remembering to not let folks fix visibility rules, as individual contributors don't know it's problematic (e.g. #20560 (comment)) and that's clearly not a good plan.

I think as a follow up to #9953 we should have CI out disallow "visibility" in //extension BUILD files, to require admin approval of any violations of the extension policy. cc @envoyproxy/envoy-maintainers for any other ideas / objections and cc @phlax for if you can pick it up or if you want to suffer reviewing my awful python :-P
@alyssawilk alyssawilk added the enhancement Feature requests. Not bugs or questions. label Mar 30, 2022
@alyssawilk alyssawilk self-assigned this Mar 30, 2022
@phlax
Copy link
Member

phlax commented Mar 30, 2022

im happy to pick it up - but i have a bit of a backlog atm so if you want to resolve more quickly

also, from my pov its probably quicker translating existing code into the pytooling libs - so would be happy to review

@alyssawilk alyssawilk mentioned this issue Mar 30, 2022
Merged
@alyssawilk
Copy link
Contributor Author

"happy to review" clearly you have not reviewed much of my python :-P Thanks, PR sent your way.

alyssawilk added a commit that referenced this issue Mar 31, 2022
@alyssawilk
tools: disallowing visibility changes by default (#20590)
3f8c00c 
Testing: ran against #20552 :-)
Leaving those BUILD files in as example exemptions.

Fixes #20589

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
ravenblackx pushed a commit to ravenblackx/envoy that referenced this issue Jun 8, 2022
@alyssawilk @ravenblackx
tools: disallowing visibility changes by default (envoyproxy#20590)
89a0b35 
Testing: ran against envoyproxy#20552 :-)
Leaving those BUILD files in as example exemptions.

Fixes envoyproxy#20589

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alyssawilk alyssawilk

Labels
enhancement Feature requests. Not bugs or questions.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

tools: disallowing visibility changes by default alyssawilk/envoy
2 participants
@phlax @alyssawilk