Skip to content

Parameterize echo listener address#1

Merged
htuch merged 1 commit into
envoyproxy:envoy-cifrom
hennna:ipv6-echo2-filter-test
May 7, 2017
Merged

Parameterize echo listener address#1
htuch merged 1 commit into
envoyproxy:envoy-cifrom
hennna:ipv6-echo2-filter-test

Conversation

@hennna
Copy link
Copy Markdown

@hennna hennna commented May 5, 2017

This change corresponds to changes in envoyproxy/envoy#883 to allow for Ipv6 parameterized integration testing. In envoyproxy/envoy#883, we SetUp the test_server for each test rather than once in SetUpTestCase.

Copy link
Copy Markdown
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, but we should also ideally update the submodule hash as well. Some rambling about CI below for further discussion..

I think we have CI coverage of this repository via the main envoy Travis (when we bump its hash for this repo). But, we might break stuff for external folks who rely on this repo as an example without CI and use the submodule build path.

Of course, if we add this, we get into a chicken-and-egg problem, as we can't update either envoy.git or envoy-filter-example.git atomically. This makes my head hurt.

I think the best thing to do is accept temporary brokeness in this repo and ship the PR as is.

@htuch
Copy link
Copy Markdown
Member

htuch commented May 7, 2017

Actually, after thinking about this, I think we should merge this into a branch called envoy-ci. That way, envoy.git can depend on this for CI, without having the example we provide to developers on the master branch breaking.

@htuch htuch changed the base branch from master to envoy-ci May 7, 2017 15:39
@htuch htuch merged commit 03f5353 into envoyproxy:envoy-ci May 7, 2017
@hennna hennna deleted the ipv6-echo2-filter-test branch June 2, 2017 17:17
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