-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add integration tests for static secrets #3910
Conversation
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@PiotrSikora @lizan @mattklein123 Could you help to review this? |
@alyssawilk do you think it's worthwhile to have some dev instructions on how to check integration tests for flakes? You have done a lot of awesome work here and it would be great to be able to point folks at a doc on "how to check my new integration test for flakes". WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few nits
auto* common_tls_context = filter_chain->mutable_tls_context()->mutable_common_tls_context(); | ||
common_tls_context->add_alpn_protocols("h2"); | ||
common_tls_context->add_alpn_protocols("http/1.1"); | ||
common_tls_context->mutable_deprecated_v1()->set_alt_alpn_protocols("http/1.1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do you need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
void initialize() override { | ||
config_helper_.addConfigModifier([](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { | ||
auto* static_resources = bootstrap.mutable_static_resources(); | ||
auto* cluster = static_resources->mutable_clusters(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you don't need this line, can be combined with the line below without variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
void initialize() override { | ||
config_helper_.addConfigModifier([](envoy::config::bootstrap::v2::Bootstrap& bootstrap) { | ||
auto* filter_chain = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you don't need filter_chain, can be combined with the line below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks this LGTM. Great to see this getting added. I would still like @alyssawilk, integration test fixer extraordinaire, so help us with flake verification instructions that can potentially be added to docs somewhere.
@mattklein123 just clarification, do you want me or @alyssawilk to help to add "flake verification instructions"? This test is similar to ssl integration tests, maybe I can copy the instructions from it. |
We should totally have a doc on how to repro and diagnose test flakes. I'm afraid for all the work I've done lately I don't have a ton to say on best practices but I'll try to get at least something written this week and add to it as I keep deflaking :-P |
@alyssawilk OK cool. Sorry I recall you saying somewhere to "run this bazel command" to uncover flakes and simulate slow CI servers and even that seems useful. We can merge this for now and come back with docs. |
Signed-off-by: Wayne Zhang qiwzhang@google.com
Description:
This integration test is for already merged PR: #3465
To integrate test secrets statically configured in the static resources.
Risk Level: None
Testing: add two integration tests.
Docs Changes: None
Release Notes: None