-
Notifications
You must be signed in to change notification settings - Fork 46
Custom gateway controller names #1240
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
Conversation
9eaa61a
to
07c5858
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1240 +/- ##
==========================================
- Coverage 77.46% 77.12% -0.34%
==========================================
Files 82 82
Lines 7388 7406 +18
==========================================
- Hits 5723 5712 -11
- Misses 1477 1497 +20
- Partials 188 197 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
e7e68ec
to
95a1ad6
Compare
|
||
func getGatewayControllerNames(envName string, defaultGatewayControllerName string) []gatewayapiv1.GatewayController { | ||
envValue := env.GetString(envName, defaultGatewayControllerName) | ||
gatewayControllers := utils.Map(strings.Split(envValue, ","), func(c string) gatewayapiv1.GatewayController { |
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.
Can we please use github.com/samber/lo
here and avoid importing another package?
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.
is there any equivalent to strings.Split
in lo
?
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.
There is nothing wrong with utils.Map
. As long as it is there, we are free to use it.
@guicassolato if you think github.com/samber/lo
is a better library than 4 lines of code on our own repo, be my guest and open a PR deleting the utils.Map
function from the repo.
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 think he means to avoid including strings
and slices
and use the already imported github.com/samber/lo
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.
strings
and slices
packages come from the standard library, which is always more stable and performant than any other third party lib.
@guicassolato can you clarify your request?
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.
Maybe we could decide if it makes sense to remove the slice utils in favor of lo
(not as part of this PR tho)
95a1ad6
to
d63d1ac
Compare
d63d1ac
to
3c48a8e
Compare
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
🥇
Signed-off-by: dd di cesare <[email protected]>
implementation Signed-off-by: dd di cesare <[email protected]>
3c48a8e
to
c97a5fc
Compare
@didierofrivia are we good to merge here? This will allow @trepel etc to start testing with 4.19 hopefully |
Closes #1200
This PR introduces a way to configure multiple controller names, as in GatewayClass.ControllerName exposing an environment variable per Gateway implementation,
ISTIO_GATEWAY_CONTROLLER_NAMES
andENVOY_GATEWAY_GATEWAY_CONTROLLER_NAMES
respectively. Which configures the operator to also reconcile with multiple custom names, i.e:ISTIO_GATEWAY_CONTROLLER_NAMES= istio.io/gateway-controller-alpha1, istio.io/gateway-controller-rc2
Kuadrant operator will reconcile not only the controllers defined in the env var, but also the default one
istio.io/gateway-controller
Notes