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

api: remove generated_api_shadow #18091

Merged
merged 5 commits into from
Sep 13, 2021
Merged

api: remove generated_api_shadow #18091

merged 5 commits into from
Sep 13, 2021

Conversation

htuch
Copy link
Member

@htuch htuch commented Sep 12, 2021

This PR eliminates the generated_api_shadow/ tree and allows direct references to the api/ directory from Envoy source. Along the way:

  • Cleaned up vestigial hidden_envoy_deprecated_ references.
  • Removed stale v1/2 tech debt from TCP proxy
  • Removed active-shadow merging and next version upgrades in protoxform.

There is still some further work to do in retiring v2, major items include (these are deferred to later PRs, this PR is already quite large):

  • Updating BUILD file generation to remove all v3 -> v2 references.
  • Simplify / cleanup / eliminate (?) type whisperer and database
  • Cleanup pass on protoxform/protosync/protoformat to simplify.

Risk level: Medium (it's unlikely but possible that some of the proto changes introduce issues, e.g. with enum defaults)
Testing: CI (also manual inspection of docs / format fixes)

Fixes #9479

Signed-off-by: Harvey Tuch htuch@google.com

@htuch
rm -rf generated_api_shadow
be1a9e2 
Signed-off-by: Harvey Tuch <htuch@google.com>
@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18091 was opened by htuch.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #18091 was opened by htuch.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 12, 2021
@htuch
Copy link
Member Author

htuch commented Sep 12, 2021 •
edited
Loading

Note to reviewers: I split the patch stack so that the bulk rm -rf generated_api_shadow/ is in the first patch, so you can just focus on the following patches.

@ggreenway I did the TCP proxy implementation cleanup in this PR, since it was using DeprecatedV1 protos internally, so it made sense to just get that out of the way.

Once this is passing CI it should be good to go for review.

CC @envoyproxy/api-shepherds @tyxia @ankatare

@htuch
Remove protoxform testdata
759020b 
Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch htuch force-pushed the remove-api-shadow branch 2 times, most recently from 9f0dd92 to 29d6dd3 Compare September 12, 2021 20:35
@htuch
Remove C++ API booster and tests.
33fac6b 
Signed-off-by: Harvey Tuch <htuch@google.com>
@htuch
Everything else
da5f292 
Signed-off-by: Harvey Tuch <htuch@google.com>
@moderation
Copy link
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 13, 2021
@htuch htuch marked this pull request as ready for review September 13, 2021 02:38
@htuch htuch requested a review from lizan as a code owner September 13, 2021 02:38
ggreenway
ggreenway previously approved these changes Sep 13, 2021
View reviewed changes
Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

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

Awesome! Looks good!

mattklein123
mattklein123 previously approved these changes Sep 13, 2021
View reviewed changes
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.

Amazing!!!

@mattklein123
Copy link
Member

Looks like we need a main merge. If you want to freeze main to get this in we can do that.

/wait

@htuch htuch dismissed stale reviews from mattklein123 and ggreenway via ef76b25 September 13, 2021 16:57
mattklein123
mattklein123 approved these changes Sep 13, 2021
View reviewed changes
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! 🎉

@mattklein123 mattklein123 merged commit b0e1526 into envoyproxy:main Sep 13, 2021
@htuch htuch deleted the remove-api-shadow branch September 13, 2021 19:06
@suniltheta suniltheta mentioned this pull request Sep 15, 2021
Merged
@htuch htuch mentioned this pull request Sep 15, 2021
Closed
@adisuissa adisuissa mentioned this pull request Sep 22, 2021
Merged
@mmizutani mmizutani mentioned this pull request Dec 31, 2021
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@ggreenway ggreenway ggreenway left review comments

@lizan lizan Awaiting requested review from lizan

Assignees

@mattklein123 mattklein123

@ggreenway ggreenway

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove shadow artifacts from API tree
4 participants
@htuch @moderation @mattklein123 @ggreenway