removing most v2 references from source/ by alyssawilk · Pull Request #17415 · envoyproxy/envoy

alyssawilk · 2021-07-20T02:21:20Z

Risk Level: Low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

daixiang0 · 2021-07-20T05:15:16Z

source/extensions/tracers/zipkin/zipkin_tracer_impl.h


  // The version of the collector. This is related to endpoint's supported payload specification and
-  // transport. Currently it defaults to envoy::config::trace::v2::ZipkinConfig::HTTP_JSON_V1. In
+  // transport. Currently it defaults to envoy::config::trace::32::ZipkinConfig::HTTP_JSON_V1. In


nice catch, fixed!

daixiang0 · 2021-07-20T05:15:35Z

test/common/config/delta_subscription_test_harness.h


 #include <queue>

+#include "envoy/api/v2/discovery.pb.h"


still use v2?

yeah this PR only fixes src/ :-)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp

Thanks!

alyssawilk · 2021-07-20T14:22:47Z

unfortunately windows CI failure is real, and due to https://github.com/envoyproxy/envoy/blob/main/source/server/proto_descriptors.cc#L13
@mattklein123 do you know if I can just remove the validate function? I'm not clear on the v3 equivalents here.

mattklein123 · 2021-07-20T16:00:59Z

@mattklein123 do you know if I can just remove the validate function? I'm not clear on the v3 equivalents here.

Hmm. I don't remember the history here. I think this is to make linking issues obvious? We should probably swap this to v3 equivalents and leave it? I guess it would be OK to just remove it all for now and see if it bites us again. I'm fine either way.

alyssawilk · 2021-07-20T19:11:13Z

yeah removing them breaks things and I'm not sure if they're real or test breakages. I'll err on the side of caution and just update to v3 equivalents.
/wait

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only · 2021-07-20T19:36:17Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17415 was synchronize by alyssawilk.

see: more, trace.

alyssawilk · 2021-07-20T21:01:29Z

/retest

repokitteh-read-only · 2021-07-20T21:01:34Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17415 (comment) was created by @alyssawilk.

see: more, trace.

htuch

LGTM, thanks!

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

markdroth · 2021-07-21T16:47:26Z

/lgtm api

alyssawilk · 2021-07-21T19:04:43Z

ugh this windows build is not having it :-(
/wait

alyssawilk · 2021-07-22T14:38:46Z

@wrowe do you have any ideas why this PR is failing on windows only? @davinci26 had suggested linking the variou removed files into the failing tests and I believe I've done that to no avail :-/

davinci26 · 2021-07-23T19:59:58Z

@alyssawilk I think that the issue is the test that are currently failing are including protobuf_link_hacks.h so you will need to add/replace with v2_link_hack.h include to those tests files as well. I am not sure if just adding a reference to the package without including is good enough. I have verified it locally as well

I think that the reason this is happening is because the compiler\linker thinks that this dependency is not used and it does not link it with the test. However, I do not fully understand why this is only happening on Windows. I have seen it a couple of times so I can recognize it as a pattern. @wrowe might be able to give more details.

Patch applied:

PS C:\envoy> git diff .\test\common\config\grpc_mux_impl_test.cc
diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc
index 3eecdbb4f..f760020dc 100644
--- a/test/common/config/grpc_mux_impl_test.cc
+++ b/test/common/config/grpc_mux_impl_test.cc
@@ -8,13 +8,13 @@
 #include "source/common/common/empty_string.h"
 #include "source/common/config/api_version.h"
 #include "source/common/config/grpc_mux_impl.h"
-#include "source/common/config/protobuf_link_hacks.h"
 #include "source/common/config/utility.h"
 #include "source/common/config/version_converter.h"
 #include "source/common/protobuf/protobuf.h"
 #include "source/common/stats/isolated_store_impl.h"

 #include "test/common/stats/stat_test_utility.h"
+#include "test/config/v2_link_hacks.h"
 #include "test/mocks/common.h"
 #include "test/mocks/config/mocks.h"
 #include "test/mocks/event/mocks.h"

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-07-26T15:06:33Z

/wait

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2021-08-02T17:31:12Z

Man, that took longer than expected. Finally got windows happy. Thanks @davinci26 for the pointer to force-includes, that seems to have done the trick!

snowp

Thanks!

davinci26

happy to help, sorry if this was frustrating

…bridge-stream * upstream/main: (32 commits) tls: move ssl connection info into SocketAddressProvider (envoyproxy#17334) conn pool: default enable runtime feature `conn_pool_delete_when_idle` (envoyproxy#17577) api: LEDS api introduction (envoyproxy#17419) kafka: add support for api versions request in mesh-filter (envoyproxy#17475) ext_proc: Implement BUFFERED_PARTIAL processing mode (envoyproxy#17531) tooling: Async/pathlib/mypy cleanups and utils (envoyproxy#17505) xds: restructure CertificateProvider fields (envoyproxy#17201) Refactor OverloadIntegrationTest breaking out a test base, and the fake resource monitors. (envoyproxy#17530) listener: move active connection collection out of active tcp listener (envoyproxy#16947) tools: format checks for backticks (envoyproxy#17566) coverage: set lower limit for common/quic and common (envoyproxy#17573) v2: final source removal (envoyproxy#17565) test: bumping coverage (envoyproxy#17564) quic: enforcing header size and contents (envoyproxy#17520) Support for canonicalizing URI properly for AWS SigV4 signer (envoyproxy#17137) listener: add a stat for transport socket connect timeout (envoyproxy#17458) listener: add listen() error handling (envoyproxy#17427) http: return per route config when direct response is set (envoyproxy#17449) removing most v2 references from source/ (envoyproxy#17415) bug fix: return bootstrap when validating config (envoyproxy#17499) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

Risk Level: Low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

removing v2 references from source/

ec61e39

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

daixiang0 reviewed Jul 20, 2021

View reviewed changes

typo

e9141cb

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

snowp previously approved these changes Jul 20, 2021

View reviewed changes

alyssawilk assigned snowp Jul 20, 2021

alyssawilk added the waiting:any label Jul 20, 2021

repokitteh-read-only bot removed the waiting:any label Jul 20, 2021

alyssawilk added the waiting:any label Jul 20, 2021

repokitteh-read-only bot added waiting and removed waiting:any labels Jul 20, 2021

CI

5f35a01

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed snowp’s stale review via 5f35a01 July 20, 2021 19:36

repokitteh-read-only bot added api and removed waiting labels Jul 20, 2021

repokitteh-read-only bot assigned markdroth Jul 20, 2021

alyssawilk added the waiting label Jul 21, 2021

repokitteh-read-only bot removed the waiting label Jul 21, 2021

alyssawilk added the waiting label Jul 21, 2021

alyssawilk force-pushed the v2_src branch from 7580252 to 5f35a01 Compare July 21, 2021 01:28

repokitteh-read-only bot removed the waiting label Jul 21, 2021

htuch previously approved these changes Jul 21, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 21, 2021

alyssawilk dismissed htuch’s stale review via a9816f0 July 21, 2021 13:53

apparently not removing bootstrap

5b5fab1

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the api label Jul 21, 2021

repokitteh-read-only bot added the waiting label Jul 21, 2021

alwayslink?

af09f9e

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jul 26, 2021

repokitteh-read-only bot added the waiting label Jul 26, 2021

manual fix

c792a86

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jul 28, 2021

alyssawilk added the waiting label Jul 28, 2021

explicit includes

eac6f79

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jul 28, 2021

alyssawilk added the waiting label Jul 28, 2021

build

d1023ce

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jul 28, 2021

alyssawilk added the waiting label Aug 2, 2021

alyssawilk added 2 commits August 2, 2021 09:24

Merge branch 'main' into v2_src

38a81a4

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

more force includes

1d2603d

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Aug 2, 2021

snowp approved these changes Aug 2, 2021

View reviewed changes

davinci26 approved these changes Aug 2, 2021

View reviewed changes

alyssawilk merged commit be63e34 into envoyproxy:main Aug 2, 2021

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

removing most v2 references from source/ (envoyproxy#17415)

02ae01b

Risk Level: Low Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk deleted the v2_src branch August 4, 2022 00:57

Conversation

alyssawilk commented Jul 20, 2021

Uh oh!

daixiang0 Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

daixiang0 Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

alyssawilk Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jul 20, 2021

Uh oh!

mattklein123 commented Jul 20, 2021

Uh oh!

alyssawilk commented Jul 20, 2021

Uh oh!

repokitteh-read-only bot commented Jul 20, 2021

Uh oh!

alyssawilk commented Jul 20, 2021

Uh oh!

repokitteh-read-only bot commented Jul 20, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

markdroth commented Jul 21, 2021

Uh oh!

alyssawilk commented Jul 21, 2021

Uh oh!

alyssawilk commented Jul 22, 2021

Uh oh!

davinci26 commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk commented Jul 26, 2021

Uh oh!

alyssawilk commented Aug 2, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

davinci26 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

davinci26 commented Jul 23, 2021 •

edited

Loading