-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refactor redis scaler config #5997
Conversation
Signed-off-by: SpiritZhou <[email protected]>
/run-e2e redis |
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@wozniakjan @dttung2905 mind reviewing? |
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.
Thank you for the PR. I just have a few minor comments, mostly on the support of multiple key name
in our util function
pkg/scalers/redis_scaler.go
Outdated
Ports []string `keda:"name=port;ports, order=triggerMetadata;resolvedEnv;authParams"` | ||
EnableTLS bool `keda:"name=enableTLS;tls, order=triggerMetadata;authParams, optional, default=false"` | ||
UnsafeSsl bool `keda:"name=unsafeSsl, order=triggerMetadata, optional, default=false"` | ||
Cert string `keda:"name=Cert;cert, order=authParams"` |
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.
Similar question there ? Are you trying to cover all cases where user can mistakenly input a value into the manifest or such cases have been supported in the current code ? 🤔
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 multi-name keys is a good feature, although I wasn't able to find Cert
(with capital C
) anywhere in the original code. Maybe this is a typo?
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, hopefully multi-named keys won't complicate the API spec generator but we will cross that bridge once we have some tooling foundation.
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
@wozniakjan what about the |
in this specific PR it should continue to work with keda/pkg/scalers/redis_scaler.go Line 50 in 3afc165
this contains keda/pkg/scalers/scalersconfig/typed_config.go Lines 403 to 405 in 3afc165
It might be a bit more understandable in this unit test keda/pkg/scalers/scalersconfig/typed_config_test.go Lines 27 to 63 in 36cc226
|
@wozniakjan Thanks for the explanation! Then it should work fine for the rest :) |
/run-e2e redis |
/run-e2e redis |
Signed-off-by: SpiritZhou <[email protected]>
@zroubalik Could you help to review it again? |
/run-e2e redis |
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Signed-off-by: SpiritZhou <[email protected]> Signed-off-by: Jorge Turrado <[email protected]>
Signed-off-by: SpiritZhou <[email protected]>
Provide a description of what has been changed
Checklist
Relates to # #5797