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

Migrate all v2 resources to v3. #177

Closed
wants to merge 6 commits into from
Closed

Migrate all v2 resources to v3. #177

wants to merge 6 commits into from

Conversation

slonka
Copy link
Contributor

@slonka slonka commented Sep 24, 2020

Fixes #97

@slonka slonka temporarily deployed to envoy-control-pr-177 September 24, 2020 11:32 Inactive
lukidzi
lukidzi previously approved these changes Sep 24, 2020
@@ -519,8 +521,13 @@ protected static void assertThatWatchReceivesSnapshot(WatchAndTracker watchAndTr

assertThat(response).isNotNull();
assertThat(response.version()).isEqualTo(snapshot.version(watchAndTracker.watch.request().getTypeUrl()));
assertThat(response.resources().toArray(new Message[0]))
.containsExactlyElementsOf(snapshot.resources(watchAndTracker.watch.request().getTypeUrl()).values());
Message[] responseValues = response.resources().toArray(new Message[0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not know why but the containsExactlyElementsOf stopped working :/ even though the values underneath are the same. I converted this to an equivalent assertion but please pay attention here.

Copy link
Contributor

@MarcinFalkowski MarcinFalkowski Sep 24, 2020

Choose a reason for hiding this comment

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

I think debugging using a breakpoint, line by line, will tell us where where the problem is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the lack of toArray for snapshot.resources is the problem?

@@ -45,13 +45,13 @@ fun RouteAction.hasCustomRequestTimeout(requestTimeout: Duration): RouteAction {
return this
}

fun RouteAction.autoHostRewriteHeaderIsEmpty(): RouteAction {
assertThat(this.autoHostRewriteHeader).isEmpty()
fun RouteAction.hostRewriteHeaderIsEmpty(): RouteAction {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -95,7 +95,7 @@ class SnapshotDebugController(controlPlane: ControlPlane) {
.add(Struct.getDescriptor())
.add(Value.getDescriptor())
.add(RBAC.getDescriptor())
.add(FilterRBAC.getDescriptor())
.add(RBACFilter.getDescriptor())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed for consistency in RBAC filter factory.

@@ -21,22 +21,6 @@ class EnvoyCurrentVersionHttpsDependencyTest : EnvoyHttpsDependencyTest() {
}
}

// TODO(https://github.com/allegro/envoy-control/issues/97) - remove when envoy < 1.14.0-dev will be not supported
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We no longer have any Envoy-s with this version on production.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also a code that needs to be removed in EnvoyClustersFactory.kt

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also add "Fixes #97" to the description

@slonka slonka temporarily deployed to envoy-control-pr-177 September 24, 2020 12:36 Inactive
@slonka slonka temporarily deployed to envoy-control-pr-177 September 25, 2020 10:13 Inactive
@slonka slonka temporarily deployed to envoy-control-pr-177 September 25, 2020 12:08 Inactive
@slonka slonka temporarily deployed to envoy-control-pr-177 September 25, 2020 12:31 Inactive
@slonka slonka temporarily deployed to envoy-control-pr-177 September 25, 2020 12:49 Inactive
@slonka slonka temporarily deployed to envoy-control-pr-177 September 25, 2020 13:45 Inactive
@pzmi
Copy link
Contributor

pzmi commented Oct 20, 2020

Looks like this PR got quite outdated. How about we close this one and get back to it with a fresh one when we're ready?

@pzmi pzmi closed this Oct 26, 2020
@pzmi pzmi deleted the update-to-v3-step-2 branch October 30, 2020 09:10
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.

Remove support for envoy < 1.14.0-dev
4 participants