-
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
/run-e2e redis |
@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.
@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 |
@zroubalik Could you help to review it again? |
/run-e2e redis |
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com> Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Provide a description of what has been changed
Checklist
Relates to # #5797