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

build: take two on upstream visibility rules #12692

Merged
merged 6 commits into from
Aug 27, 2020

Conversation

alyssawilk
Copy link
Contributor

Setting things up so downstream folks can just override with visibility:public but upstream avoids core code depending on extensions.

Risk Level: medium (of breaking builds, instructions of how to unbreak included)
Testing: manually swapped to public, all works.
Docs Changes: included
Release Notes: overkill
Fixes #12444

@lizan
Copy link
Member

lizan commented Aug 17, 2020

I just merged envoyproxy/envoy-filter-example#128, can we bump filter-example SHA in CI to see if it works?

@alyssawilk
Copy link
Contributor Author

pass-through filter is manually tagged public (not using the default rules) so I think it's orthogonal from this change.

More interesting would be if it were private, and envoy filter example had an override to explicitly depend on it, but I agree that one should be public (and possibly moved to core but organization tbd)

@lizan
Copy link
Member

lizan commented Aug 17, 2020

@alyssawilk it is manually tagged public after the previous take. There were a couple PRs.

@alyssawilk
Copy link
Contributor Author

Right, but this PR doesn't revert that being public, so I guess I'm not sure what bumping proves?
I can do it either way, I would just expect this change and that change to not interact.

Signed-off-by: Alyssa Wilk <[email protected]>
# These can be changed to ["//visibility:public"], for downstream builds which
# need to directly reference Envoy extensions.
EXTENSION_CONFIG_VISIBILITY = ["//:extension_config"]
EXTENSION_EXTENSION_PACKAGE_VISIBILITY = ["//:extension_library"]
Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable. I can imagine a finer grained system, but this is fine as a starting point to limit the visibility in Envoy.

Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Thanks this looks good. just one nit

# These can be changed to ["//visibility:public"], for downstream builds which
# need to directly reference Envoy extensions.
EXTENSION_CONFIG_VISIBILITY = ["//:extension_config"]
EXTENSION_EXTENSION_PACKAGE_VISIBILITY = ["//:extension_library"]
Copy link
Member

Choose a reason for hiding this comment

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

Just name this EXTENSION_PACKAGE_VISIBILITY?

@lizan
Copy link
Member

lizan commented Aug 21, 2020

@rgs1 can you test this PR with your BUILD?

@lizan
Copy link
Member

lizan commented Aug 21, 2020

/wait-any

@rgs1
Copy link
Member

rgs1 commented Aug 24, 2020

@rgs1 can you test this PR with your BUILD?

Hey sorry for the late response I was OOO; yes I can give this a try. Mind merging master first tho?

@rgs1
Copy link
Member

rgs1 commented Aug 25, 2020

So with:

diff --git a/bazel/build_config/extensions_build_config.bzl b/bazel/build_config/extensions_build_config.bzl
index ab61ec7..5e1f725 100644
--- a/bazel/build_config/extensions_build_config.bzl
+++ b/bazel/build_config/extensions_build_config.bzl
@@ -101,3 +101,6 @@ WINDOWS_EXTENSIONS = {
 }

 ADDITIONAL_VISIBILITY = []
+
+EXTENSION_CONFIG_VISIBILITY = ["//visibility:public"]
+EXTENSION_PACKAGE_VISIBILITY = ["//visibility:public"]

this works for us. Thanks!

[ we do reference a lot of Envoy extensions... the gzip stuff, the crypto stuff, etc. ]

Copy link
Member

@rgs1 rgs1 left a comment

Choose a reason for hiding this comment

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

Works for us -- thanks!

@alyssawilk
Copy link
Contributor Author

@lizan any other thoughts?

@alyssawilk
Copy link
Contributor Author

@lizan ping?

@alyssawilk alyssawilk merged commit d245c02 into envoyproxy:master Aug 27, 2020
@alyssawilk
Copy link
Contributor Author

cc @yanavlasov @auni53

@alyssawilk alyssawilk deleted the vis_fix branch December 10, 2020 19:16
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.

visibility issues after #12337
5 participants