Skip to content
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

feat: Add redis streams length e2e test images (#4277) #117

Merged
merged 2 commits into from
Mar 21, 2023

Conversation

jkbmdk
Copy link
Contributor

@jkbmdk jkbmdk commented Mar 20, 2023

Images added for testing issue kedacore/keda#4277

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)
  • A PR is opened to update the documentation on our docs repo

Fixes #

@JorTurFer
Copy link
Member

Hi,
What has changed from already existing images? The only change I can see is that these images don't consume the messages (or at least, I can't see how)

@jkbmdk
Copy link
Contributor Author

jkbmdk commented Mar 20, 2023

Hey, current images (for cluster and sentinel) reads messages and acks on them, but do not removes messages from the stream, it works as an append only stream. Provided images pops out readen messages, so the stream length decreases

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
I am curious wouldn't it make a sense to modify existing images and sources? ie. add some parameter to enable/disable this feature?
This might be easier to maintain for us.

@jkbmdk
Copy link
Contributor Author

jkbmdk commented Mar 20, 2023

I think it's possible to extend redis cluster and sentinel images, but standalone is outside of the repository. It might be easier to rewrite standalone image/test, shall I do this?

@JorTurFer
Copy link
Member

I think that if this is something we can parametrize, we could parametrize it and also adding the code for standalone redis. I mean, you have already added the code with the new feature, you could just replace the already existing external image using your code with the flag

@jkbmdk
Copy link
Contributor Author

jkbmdk commented Mar 21, 2023

I've made one image handling all redis-streams test cases, "one to rule them all"

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Thanks for the improvement

@JorTurFer JorTurFer merged commit 6944498 into kedacore:main Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants