Skip to content

config: rename NewGrpcMuxImpl -> GrpcMuxImpl#8919

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
wgallagher:renamenewgrpcmuximpl
Nov 14, 2019
Merged

config: rename NewGrpcMuxImpl -> GrpcMuxImpl#8919
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
wgallagher:renamenewgrpcmuximpl

Conversation

@wgallagher
Copy link
Copy Markdown

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: rename NewGrpcMuxImpl -> GrpcMuxImpl
Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

@mattklein123 mattklein123 self-assigned this Nov 6, 2019
@mattklein123
Copy link
Copy Markdown
Member

Thanks @wgallagher can you fix format and DCO (see contributor guide for setting things up)?

/wait

@mattklein123
Copy link
Copy Markdown
Member

Putting into waiting while we deal with the revert.

/wait

Signed-off-by: Bill Gallagher <bgallagher@lyft.com>
@mattklein123
Copy link
Copy Markdown
Member

@wgallagher please fix format and undraft when ready for review. Thank you.

/wait

@mattklein123
Copy link
Copy Markdown
Member

Please fix DCO.

/wait

Signed-off-by: Bill Gallagher <bgallagher@lyft.com>
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.

LGTM with small comment. Thank you!

/wait

namespace Envoy {
namespace Config {

NewGrpcMuxImpl::NewGrpcMuxImpl(std::unique_ptr<SubscriptionStateFactory> subscription_state_factory,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you rename the files also to get rid of new? (Here and in all the other files).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

+1

Signed-off-by: Bill Gallagher <bgallagher@lyft.com>
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.

Thanks, LGTM with one question.

/wait-any

],
)

envoy_cc_test(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did we lose coverage here somehow?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

this class contained a single test which seemed to be duplicated in grpc_mux_impl_test.cc

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Specifically, TEST_F(GrpcMuxImplTest, DiscoveryResponseNonexistentSub) which does everything this test does and adds a few additional checks.

@wgallagher wgallagher marked this pull request as ready for review November 14, 2019 18:18
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.

Thanks!

@mattklein123 mattklein123 merged commit 6d50553 into envoyproxy:master Nov 14, 2019
mattklein123 added a commit that referenced this pull request Nov 15, 2019
…e_names_ (#8918)"

This reverts commit 80aedc1.

Revert "config: rename NewGrpcMuxImpl -> GrpcMuxImpl (#8919)"
This reverts commit 6d50553.

Revert "config: reinstate #8478 (unification of delta and SotW xDS), reverted by #8939 (#8974)"
This reverts commit a37522c.

Signed-off-by: Matt Klein <mklein@lyft.com>
mattklein123 added a commit that referenced this pull request Nov 16, 2019
…e_names_ (#8918)" (#9044)

This reverts commit 80aedc1.

Revert "config: rename NewGrpcMuxImpl -> GrpcMuxImpl (#8919)"
This reverts commit 6d50553.

Revert "config: reinstate #8478 (unification of delta and SotW xDS), reverted by #8939 (#8974)"
This reverts commit a37522c.

Signed-off-by: Matt Klein <mklein@lyft.com>
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