Factory plugin implementation by wjuan-AFK · Pull Request #566 · envoyproxy/nighthawk

wjuan-AFK · 2020-10-28T14:47:28Z

Adding implementation in the factory_impl class for loading request source plugins. This also introduces python tests for the previous PR and undoes the temporary hack to reduce test_integration_coverage threshold in issue #564.

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>

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>

…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>

…format and check format. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

wjuan-AFK · 2020-11-09T19:21:43Z

/retest

repokitteh-read-only · 2020-11-09T19:21:47Z

🔨 rebuilding ci/circleci: clang_tidy (failed build)
🔨 rebuilding ci/circleci: test_gcc (failed build)

🐱

Caused by: a #566 (comment) was created by @wjuan-AFK.

see: more, trace.

wjuan-AFK · 2020-11-09T22:35:52Z

/retest

repokitteh-read-only · 2020-11-09T22:35:57Z

🔨 rebuilding ci/circleci: clang_tidy (failed build)

🐱

Caused by: a #566 (comment) was created by @wjuan-AFK.

see: more, trace.

wjuan-AFK · 2020-11-10T16:28:17Z

/retest

repokitteh-read-only · 2020-11-10T16:28:20Z

🔨 rebuilding ci/circleci: clang_tidy (failed build)

🐱

Caused by: a #566 (comment) was created by @wjuan-AFK.

see: more, trace.

oschaaf

One last nit about removing // GCOVR_EXCL_START and // GCOVR_EXCL_END. It seems some of the changes outdated that comment, and these annotations no longer have value. Sorry about the back and forth on that. Other then that, this looks great to me.

oschaaf · 2020-11-10T21:15:31Z

source/client/factories_impl.cc

+  } else if (options_.requestSourcePluginConfig().has_value()) {
+    absl::StatusOr<RequestSourcePtr> plugin_or = LoadRequestSourcePlugin(
+        options_.requestSourcePluginConfig().value(), api_, std::move(header));
+    // GCOVR_EXCL_START


I didn't expect the GCOVR exclusion here, but rather at/around the RELEASE_ASSERT line above -- so, sorry could you remove it here? Looking at the coverage report everything is fine now, including the RELEASE_ASSERT line, so it seems we don't need it there either now (probably because there's no conditional argument being passed).

Sounds good.

source/client/factories_impl.cc

mum4k

Ready for merging, blocked on the clang_tidy flake which we are planning to remove temporarily in the near future (#570).

mum4k · 2020-11-11T03:05:44Z

@oschaaf, this is still marked as "change requested" from your last review, can you please indicate if there is anything outstanding or approve if otherwise?

oschaaf · 2020-11-11T09:26:48Z

@mum4k Sorry, I forgot to update the waiting-for-... label. There's one last (small) change I requested.

mum4k · 2020-11-11T15:30:58Z

Please merge from master to remove the hard block on clang_tidy.

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

oschaaf

Looks great, thanks

wjuan-AFK added 30 commits July 15, 2020 20:04

Adding a multiple path test to confirm that expected paths are hit

99f45d1

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

fixing asan problems

075a4cf

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix Format

ecbf407

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Removeing unused comments

8fb11b6

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Removing unnecessary invoke calls for cleanliness

032dbba

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath

953e953

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Refactoring test to take in request generator parameter instead of using

6185edb

a global request generator. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Changing to pass by reference.

cded6ad

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath

fbfe523

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'master' into RequestSourceUnitTestBenchmarkMultiplePath

0d70179

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Refactoring per comments in PR. Moving helpers, renaming functions.

cdb7efe

Adding comments. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding comments and renaming testbasicfunctionality for clarity

f15bb47

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Consistent unit test naming convention using should. Clarifying comments

59359a2

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format

f63df91

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding a clarifying comment for helper function.

406bd6b

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fixing typo and other nits

f0b697b

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Renaming testBenchmarkClient for clarity

8425e48

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Replacing Flat hash map with flat hash set for simplicity.

fb4f372

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Comment clarification.

c6eae6c

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Small refactor for factory method for more dependency injection.

96ae38b

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format.

b17240f

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Refactor to inject the requestsourceconstructor.

5c5a146

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Moving helper functions.

29591ff

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'RequestSourceUnitTestBenchmarkMultiplePath' into Adding…

7d928a9

…TestForRequestSourceFactory Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format.

ff82d95

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding test using the new mock requestsourceconstructor to get remote

98381a4

request source. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format.

83ac556

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding missing request source constructor files.

38c5084

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Changing helper functions to anonymous namespace.

fa4e285

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Changing to use a nullable pointer instead of reference for clarity.

8551c76

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

wjuan-AFK added 4 commits November 9, 2020 16:54

changing to exception over assert, and adding tests.

d04aa5e

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Removing new line since check format was complaining.

3be39ae

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Fix format

932de71

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Adding python format exclusion because of weird conflict between fix …

e9572a8

…format and check format. Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

wjuan-AFK added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Nov 9, 2020

wjuan-AFK requested review from mum4k and oschaaf November 9, 2020 19:21

oschaaf requested changes Nov 10, 2020

View reviewed changes

mum4k previously approved these changes Nov 11, 2020

View reviewed changes

oschaaf added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Nov 11, 2020

wjuan-AFK added 2 commits November 11, 2020 16:07

Update comment.

b6b5b64

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Merge branch 'master' into FactoryPluginImplementation

dc5d9c8

Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

wjuan-AFK dismissed mum4k’s stale review via dc5d9c8 November 11, 2020 16:10

wjuan-AFK added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Nov 11, 2020

oschaaf approved these changes Nov 11, 2020

View reviewed changes

mum4k approved these changes Nov 11, 2020

View reviewed changes

mum4k merged commit d63ade7 into envoyproxy:master Nov 11, 2020

wjuan-AFK deleted the FactoryPluginImplementation branch November 11, 2020 18:59

Conversation

wjuan-AFK commented Oct 28, 2020

Uh oh!

wjuan-AFK commented Nov 9, 2020

Uh oh!

repokitteh-read-only bot commented Nov 9, 2020

Uh oh!

wjuan-AFK commented Nov 9, 2020

Uh oh!

repokitteh-read-only bot commented Nov 9, 2020

Uh oh!

wjuan-AFK commented Nov 10, 2020

Uh oh!

repokitteh-read-only bot commented Nov 10, 2020

Uh oh!

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

oschaaf Nov 10, 2020

Choose a reason for hiding this comment

Uh oh!

wjuan-AFK Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

mum4k commented Nov 11, 2020

Uh oh!

oschaaf commented Nov 11, 2020

Uh oh!

mum4k commented Nov 11, 2020

Uh oh!

oschaaf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants