Refactor internal/envoy/v3 package functions #6871
Conversation
Take two of projectcontour#5523 That introduces a struct `NewEnvoysGen` that allows us to modify the behaviour of Envoy config generation. This will help with projectcontour#6806 On the debate on `singleton` vs `struct` approach. I thought this again and I realized that `struct` is probably worth the big diff here. For the following reason: * It allows us to test the behaviour much easier. With a singleton it would be impossible for higher level tests like tests in `internal/featuretests` to modify the behaviour of the Envoy config generation. With a struct, we can just pass the config options to the struct and test the behaviour. Signed-off-by: Sotiris Nanopoulos <[email protected]>
davinci26
requested review from
skriss and
sunjayBhatia
and removed request for
a team
sunjayBhatia
requested review from
a team,
rajatvig and
wilsonwu
and removed request for
a team
|
(waiting for the CI to pass before I give people the go ahead to review) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6871 +/- ##
==========================================
+ Coverage 81.05% 81.07% +0.02%
==========================================
Files 133 134 +1
Lines 20026 20087 +61
==========================================
+ Hits 16232 16286 +54
- Misses 3500 3507 +7
Partials 294 294
|
Signed-off-by: Sotiris Nanopoulos <[email protected]>
|
@sunjayBhatia this is ready for review. Does this need a release note? |
probably not! i'll add the label |
sunjayBhatia
added
the
release-note/none-required
There was a problem hiding this comment.
AFAICT looks good! tests are all passing+compiling which is a good hint
one other tiny nit would be to see if there are any other exported functions in internal/envoy/v3 that could be unexported to avoid any future code using them and not using the config generator (e.g. GrpcService)
will leave for others to catch anything i missed but 👍🏽 from me
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
Signed-off-by: Sotiris Nanopoulos <[email protected]>
…efault and old are the same Signed-off-by: Sotiris Nanopoulos <[email protected]>
|
@skriss and @sunjayBhatia can I get another round of review 🙏 🙏 🙏 I am afraid that I will be fighting with merge conflicts with this one and this one is blocking to delta XDS which is something that would be fantastic for the project! |
|
LGTM but should get another sanity check from another one of @projectcontour/maintainers, thanks @davinci26! |
Take two of #5523
That introduces a struct
NewEnvoysGenthat allows us to modify the behaviour of Envoy config generation. This will help with#6806
On the debate on
singletonvsstructapproach. I thought this again and I realized thatstructis probably worth the big diff here. For the following reason:internal/featureteststo modify the behaviour of the Envoy config generation. With a struct, we can just pass the config options to the struct and test the behaviour.