-
Notifications
You must be signed in to change notification settings - Fork 81
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
Cli change for file based request source #563
Cli change for file based request source #563
Conversation
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
a global request generator. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Adding comments. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
…TestForRequestSourceFactory Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
request source. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
🔨 rebuilding |
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
/retest |
🔨 rebuilding |
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, thanks!
Leaving one last suggestion with respect to a test that now uses a textual yaml diff comparison as the sole means to test for equivalence, I suspect that doubling down with a true equivalence tests helps catch edge cases. (We really need #433)
test/options_test.cc
Outdated
std::string s1 = Envoy::MessageUtil::getYamlStringFromMessage( | ||
*(options_from_proto.toCommandLineOptions()), true, true); | ||
std::string s2 = Envoy::MessageUtil::getYamlStringFromMessage(*command, true, true); | ||
EXPECT_EQ(s1, s2); |
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.
Consider also adding the following comparison, as I suspect the yaml/text based comparison may miss edge cases.
EXPECT_TRUE(util(*(options_from_proto.toCommandLineOptions()), *command));
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.
Ok. Understood. That makes sense.
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.
@dubious90 Wanted to let you know that I was adding this back in since this reasoning makes sense to me.
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
/retest |
🔨 rebuilding |
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 modulo a few commenting comments. Will continue tracking the two conversations there, but otherwise reassigning to mumak, who will approve.
test/options_test.cc
Outdated
@@ -249,7 +250,7 @@ TEST_F(OptionsImplTest, AlmostAll) { | |||
EXPECT_TRUE(util(cmd->stats_sinks(0), options->statsSinks()[0])); | |||
EXPECT_TRUE(util(cmd->stats_sinks(1), options->statsSinks()[1])); | |||
EXPECT_EQ(cmd->latency_response_header_name().value(), options->responseHeaderWithLatencyInput()); | |||
|
|||
// TODO(#433) |
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.
Two nit asks on this set of TODOs:
- Can we add a oneliner, explaining what the action to do here specifically is
- Rather than repeating it on every test here, we could have in the comment something like "Here and below". Then we only need to include the TODO once per relevant file
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.
- sounds good to me. I feel like tagging the comparisons which need to be replaced is helpful, but I can remove the other ones if you insist.
test/options_test.cc
Outdated
CommandLineOptionsPtr cmd = options->toCommandLineOptions(); | ||
EXPECT_TRUE( | ||
util(cmd->request_source_plugin_config(), options->requestSourcePluginConfig().value())); | ||
// Now we construct a new options from the proto we created above. This should result in an |
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.
Sorry -- my previous code comment wasn't particularly helpful/directive. I agree that comments here would be helpful, but I'm not finding this comment to clarify the code for me, so let me re-explain better:
In general, comments are most helpful when we explain the why, rather than the what. This comment is actually doing a little bit of both. One of the reasons why I think you're finding the need to explain "what" is that the comments aren't actually positionally with the code that they're explaining.
Rather than explaining a whole series of actions in one comment, can we split up the comments to be directly tied to their code sections?
test/options_test.cc
Outdated
OptionsImpl options_from_proto(*command); | ||
std::string s1 = Envoy::MessageUtil::getYamlStringFromMessage( | ||
*(options_from_proto.toCommandLineOptions()), true, true); | ||
std::string s2 = Envoy::MessageUtil::getYamlStringFromMessage(*command, true, true); |
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.
One thing that might make this easier to parse is if we use more descriptive names, rather than s1 and s2. Maybe expected_options_yaml and actual_options_yaml? Or whatever names you think would be accurate/helpful.
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.
For sure.
test/options_test.cc
Outdated
INSTANTIATE_TEST_SUITE_P( | ||
HappyPathRequestSourceConfigJsonSuccessfullyTranslatesIntoOptions, | ||
RequestSourcePluginTestFixture, | ||
::testing::ValuesIn(std::vector<std::string>{ |
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.
I'm having a really hard time parsing this. There are two things that might help here:
- C++ has a formatted string option that will make this much easier to read, rather than using " so much. You can do R"( string that contains " with no problems)". https://abseil.io/tips/64
- (optional) I think this is just one string, but it's difficult to tell. Can we abstract the actual textprotos into variables?
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.
I'm not sure that 1 works for me here. I might be missing something, but it seems like there are issues with the newlines. At least it seems like using R"( makes you keep the newlines if you space them into separate lines. I can make each line a separate raw string though, I'm not sure that this makes it more readable, but maybe you'll find this preferable?
2. I've kind of tried to do this. Hope that this approach looks ok.
Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>
/retest |
🔨 rebuilding |
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.
Looks good, no actionable comments from my end.
@dubious90 this is good to go. |
Commit to add RequestSourcePlugin option to the CLI. base PR: #560.
This is necessary to make use of the previous PRs adding the requestsourcepluginfactory via the CLI.