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

envoy: move listener manager code back under source #31129

Merged
merged 6 commits into from
Dec 6, 2023

Conversation

alyssawilk
Copy link
Contributor

@alyssawilk alyssawilk commented Nov 30, 2023

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Part of #31113

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: #31129 was opened by alyssawilk.

see: more, trace.

@alyssawilk alyssawilk force-pushed the listener_move branch 7 times, most recently from 19001e0 to 6f96797 Compare December 4, 2023 15:23
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

🐱

Caused by: #31129 was synchronize by alyssawilk.

see: more, trace.

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.

The new location source/common/tcp will now also contain the udp listener code; that's confusing. Maybe it should go into source/common/listener_manager instead?

/wait-any

@@ -59,6 +59,7 @@ declare -a KNOWN_LOW_COVERAGE=(
"source/extensions/wasm_runtime/wasmtime:0.0" # Not enabled in coverage build
"source/extensions/wasm_runtime/wavm:0.0" # Not enabled in coverage build
"source/extensions/watchdog:83.3" # Death tests within extensions
"source/extensions/listener_managers:70.5"
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, this is quite low. It looks like the only change in that directory is a path in a #include. Do you understand why the test coverage is so low?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this directory covered the highly tested TCP code and the not highly tested validation manager code.

Copy link
Contributor

Choose a reason for hiding this comment

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

facepalm Thanks!
Coverage LGTM

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
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.

I think the files previously in source/common/tcp were accidentally duplicated into listener_manager: async_tcp_client_impl.h/cc and conn_pool.h/cc

/wait

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk merged commit 5c4b0d6 into envoyproxy:main Dec 6, 2023
105 checks passed
@alyssawilk alyssawilk deleted the listener_move branch March 19, 2024 19:47
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.

3 participants