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

extensions: Consolidate configuration #16496

Merged
merged 1 commit into from
May 28, 2021
Merged

Conversation

phlax
Copy link
Member

@phlax phlax commented May 14, 2021

Signed-off-by: Ryan Northey [email protected]

Commit Message: extensions: Consolidate configuration
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax changed the title extensions: Consolidate configuration [WIP] extensions: Consolidate configuration May 14, 2021
@phlax phlax force-pushed the extensions-config branch from 120395f to 87cfcc2 Compare May 14, 2021 12:13
@phlax
Copy link
Member Author

phlax commented May 15, 2021

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #16496 (comment) was created by @phlax.

see: more, trace.

@phlax phlax force-pushed the extensions-config branch from d1bc1d8 to 40be3ec Compare May 15, 2021 09:48
bazel/envoy_library.bzl Outdated Show resolved Hide resolved
@phlax phlax force-pushed the extensions-config branch 4 times, most recently from 1c85b30 to bade3aa Compare May 16, 2021 10:02
tools/extensions/BUILD Outdated Show resolved Hide resolved
@phlax phlax force-pushed the extensions-config branch from bade3aa to 686ad21 Compare May 16, 2021 10:12
@phlax
Copy link
Member Author

phlax commented May 16, 2021

@htuch - quick summary

there is a remaining issue on what to do with the validation checks that previously happened in the bazel macro - i can either readd by adding extensions names to source bazel build files - or we can validate these in generate_extension_db

i have added a regression test for generate_extension_db this should be removed before landing i think

there is an override extensions_build_config.bzl file for mac osx that is still using the old format - nothing seems to break - but im still wondering whether it should be updated to use the newer format

related to this im noticing that other repos also use or override the extensions_build_config.bzl file - eg

not sure how these will be affected by changing the format of the config in this way - im happy to open downstream PRs to fix if necessary

im also noticing that there are some dev docs that will need to be updated...

@phlax phlax force-pushed the extensions-config branch from 686ad21 to df06bbf Compare May 16, 2021 11:04
@phlax
Copy link
Member Author

phlax commented May 16, 2021

re breaking https://github.com/envoyproxy/envoy-mobile - i have opened a ticket here envoyproxy/envoy-mobile#1475 to resolve

re breaking https://github.com/GoogleCloudPlatform/esp-v2 - this seems to be quite a way behind envoy HEAD so its unlikely to break soon - i can open a ticket there warning of impending extensions config change

docs/build.sh Outdated Show resolved Hide resolved
@phlax
Copy link
Member Author

phlax commented May 16, 2021

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #16496 (comment) was created by @phlax.

see: more, trace.

@phlax phlax force-pushed the extensions-config branch 4 times, most recently from e7838aa to fe150f0 Compare May 17, 2021 14:37
@phlax phlax force-pushed the extensions-config branch from 197935d to 37252f9 Compare May 27, 2021 11:32
@phlax phlax force-pushed the extensions-config branch from 37252f9 to ea00ebf Compare May 27, 2021 11:43
@phlax phlax changed the title [WIP] extensions: Consolidate configuration extensions: Consolidate configuration May 27, 2021
@phlax phlax assigned htuch and unassigned phlax May 27, 2021
@phlax phlax marked this pull request as ready for review May 27, 2021 11:44
@phlax phlax removed the deps Approval required for changes to Envoy's external dependencies label May 27, 2021
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Like seeing the wall of red.

@htuch htuch merged commit 57976d1 into envoyproxy:main May 28, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
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.

2 participants