Skip to content

add assert in immutable config provider constructor to make sure…#6878

Merged
htuch merged 5 commits intoenvoyproxy:masterfrom
stevenzzzz:hcm
May 23, 2019
Merged

add assert in immutable config provider constructor to make sure…#6878
htuch merged 5 commits intoenvoyproxy:masterfrom
stevenzzzz:hcm

Conversation

@stevenzzzz
Copy link
Copy Markdown
Contributor

… correct instance type gets set

Signed-off-by: Xin Zhuang stevenzzz@google.com

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

Description: add asserts in ImmutableConfigProviderBase too make sure correct instance type gets passed in.
Risk Level: LOW
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

…rect instance type gets set

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
@stevenzzzz
Copy link
Copy Markdown
Contributor Author

@AndresGuedez PTAL.

Copy link
Copy Markdown
Contributor

@AndresGuedez AndresGuedez left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup. A test would be great :- )

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
@stevenzzzz
Copy link
Copy Markdown
Contributor Author

Added a test.

Copy link
Copy Markdown
Contributor

@AndresGuedez AndresGuedez left a comment

Choose a reason for hiding this comment

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

LGTM modulo outstanding comment regarding test suite name. Thanks!

const Protobuf::Message* getConfigProto() const override { return nullptr; }
};

TEST_F(ConfigProviderImplTest, AssertionFailureOnIncorrectInstanceType) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

PTAL.

Copy link
Copy Markdown
Contributor

@AndresGuedez AndresGuedez left a comment

Choose a reason for hiding this comment

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

Undo LGTM. Please check test failure.

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
ConfigProviderInstanceType::Inline);
InlineDummyConfigProvider bar(factory_context_, *provider_manager_,
ConfigProviderInstanceType::Static);
EXPECT_DEATH(InlineDummyConfigProvider(factory_context_, *provider_manager_,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs to use EXPECT_DEBUG_DEATH().

Copy link
Copy Markdown
Contributor Author

@stevenzzzz stevenzzzz May 16, 2019

Choose a reason for hiding this comment

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

right

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
@stale
Copy link
Copy Markdown

stale bot commented May 23, 2019

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label May 23, 2019
Copy link
Copy Markdown
Contributor

@AndresGuedez AndresGuedez left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label May 23, 2019
@stevenzzzz
Copy link
Copy Markdown
Contributor Author

@htuch could you please give it a stamp.

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

@htuch htuch merged commit 668725e into envoyproxy:master May 23, 2019
@stevenzzzz stevenzzzz deleted the hcm branch July 25, 2019 14:16
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