-
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
Improve Redis support #2181
Improve Redis support #2181
Conversation
Signed-off-by: Jeroen Bobbeldijk <[email protected]>
Signed-off-by: Jeroen Bobbeldijk <[email protected]>
…ange if statements to switch statements Signed-off-by: Jeroen Bobbeldijk <[email protected]>
Thanks for the PR, can you also create an issue please? Typically we discuss things on issues first but it's OK in this case. |
Done #2182 |
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.
Looking good, I have a few questions:
- does the updated Redis Library still support the old versions? (ie. we are backwards compatible)
- I am not Redis expert, but if I am not mistaken Redis Sentinel is basically HA solution for Redis, do you think that it makes sense to create e2e test for Redis Sentinel Scalers similar to Redis existing ones? https://github.com/kedacore/keda/tree/main/tests
Yes, you can check here: https://github.com/go-redis/redis/blob/master/CHANGELOG.md
Yes Sentinel is a HA solution for Redis, ik keeps track of which server is the master, and if there is a failure it knows how to recover and select a new master. When you connect using Sentinel, the client asks which server is the master at that point in time and then connects to that master. I was not aware of the e2e tests, I only noticed the go tests, but yes it would be a good idea to add e2e tests, will look into that! |
OK, makes sense, thanks! |
@zroubalik
|
The sources should be here: https://github.com/kedacore/test-tools/tree/master/e2e/images/redis But the reference to https://github.com/orgs/kedacore/packages?repo_name=test-tools registry hasnt been changed yet in the current redis e2e tests. Would you mind changing it in your PR? I hope that all sources are there. |
Thanks! |
@abhirockzz do you have the sources for |
Please note that the following images are not in there at all: |
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.
Overall this looks excellent.
My one, fairly large all request is to remove the global var ctx = context.Background()
and add it as a parameter to whatever function(s) requires a context.Context
. I noted a few (bot not all) places where that would be appropriate
I have updated some of the code to use the context that Keda provides (in |
bbcce79
to
a6ee491
Compare
Signed-off-by: Jeroen Bobbeldijk <[email protected]>
Signed-off-by: Jeroen Bobbeldijk <[email protected]>
a6ee491
to
b5f1b40
Compare
Signed-off-by: Jeroen Bobbeldijk <[email protected]>
I agree, we should tackle the context propagation in a separate task. @jerbob92 would you mind opening an issue for this? Thanks! |
Done: #2190 Just FYI: |
@jerbob92 currently we are not running e2e tests on PRs, they run after the merge on |
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.
Hi @jerbob92 sorry for the late reply. The mentioned images are available. See the screenshot below Are you still having problems accessing them or is it resolved? Thanks for your patience. |
@goku321 we are looking for sources of those images, we would like to host the sources here: https://github.com/kedacore/test-tools @jerbob92 would like to extend your images for testing the feature from this PR. Thanks! |
Hello @zroubalik, this makes sense. Now I realize I had this thing in mind before but I got caught up with other stuff. I'll put the sources in the required place. |
@zroubalik @jerbob92 Here's the source for above mentioned images: kedacore/test-tools#21 Please do let me know if I can help with anything else 🙂 |
@jerbob92 e2e tests sources are up, feel free to update them as it is needed to. Then please use images hosted in ghcr.io: https://github.com/orgs/kedacore/packages?tab=packages&q=redis Thanks! |
Signed-off-by: Jeroen Bobbeldijk <[email protected]>
d2b8abe
to
599fd09
Compare
@zroubalik What do I have to do to get the images from test-tools? I'd like to run the e2e tests. The MR in test-tools has been merged but the new packages don't show up. |
@jerbob92 I can see the images here: https://github.com/orgs/kedacore/packages?tab=packages&q=sent Do you need anything else? |
@jerbob92 the e2e tests are running here: https://github.com/kedacore/keda/runs/4068547564?check_suite_focus=true |
They don't show up for me. Are they private by default? |
@jerbob92 you are right, sorry I missed that. Fixing now, thanks! |
|
This pull request:
Checklist