Skip to content

api: remove generated_api_shadow#18091

Merged
mattklein123 merged 5 commits intoenvoyproxy:mainfrom
htuch:remove-api-shadow
Sep 13, 2021
Merged

api: remove generated_api_shadow#18091
mattklein123 merged 5 commits intoenvoyproxy:mainfrom
htuch:remove-api-shadow

Conversation

@htuch
Copy link
Copy Markdown
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
Copy Markdown

repokitteh-read-only bot commented Sep 12, 2021

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
Copy Markdown

repokitteh-read-only bot commented Sep 12, 2021

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
Copy Markdown
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
Copy Markdown
Contributor

moderation commented Sep 13, 2021

/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
Copy Markdown
Member

@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
Copy Markdown
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
Copy Markdown
Member

mattklein123 commented Sep 13, 2021

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
Copy Markdown
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
Closed
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