-
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
Add NATS JetStream scaler #3468
Conversation
I tested this scaler using kind and could only test with |
@goku321 any update on this please? |
@zroubalik I will work on the changes today/tomorrow and make it ready to be merged :) |
@goku321 any updates please? we would need to merge it today in order to get it into the next release, if not that we will target 2.9. |
Hi @zroubalik Going to push the changes very soon. Converting tests to go took some time and running tests locally in kind is a bit slower 😅 |
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, just a couple of minor nits.
@goku321 also please fix the static checks: https://github.com/kedacore/keda/runs/7720862248?check_suite_focus=true |
/run-e2e nats_jetstream_test* |
@goku321 could you please rebase your branch agains main? There has been some updates recently (we haver removed npm) that are failing here. |
@goku321 also linter is failing, you can check it locally by running: |
@zroubalik linter is complaining for duplicate code b/w stan and nats_jetstream scaler. Some part is overlapping |
In that case please add exception for your scaler to this file: Line 67 in acb03ab
Also, plase squash the commits, so there are only relevant commits in this PR, otherwise the DCO will complain. |
Signed-off-by: Deepak <sah.sslpu@gmail.com>
Signed-off-by: Deepak <sah.sslpu@gmail.com>
@zroubalik I merged from main into this branch a few times instead of rebasing so I'm not sure if it is possible to squash now. Is it possible to do "squash and merge" from the drop down while merging? |
@goku321 yeah, I can squash those commits. I just don't want to have any unrelated commits here. |
@zroubalik Also, I updated |
I am not sure, but given the error it seems like that you'd have to do that :( Try that locally and you will see. Also please fix the package name in the e2e test, see the other linter error. Then we are hopefully good to test this scaler here in the PR and the only outstanding issue will be docs PR 🤞 |
Signed-off-by: Deepak <sah.sslpu@gmail.com>
/run-e2e nats_jetstream_test* |
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
@goku321 just the Documentation is missing. |
@zroubalik Working on the documentation 🙂 |
@goku321 what is the status? If we don't merge it today it won't get into the release unfortunately |
@zroubalik Added the documentation. Thanks for your patience 😅 |
@goku321 I have just realized that the PR is missing a Unit test. I completely missed this somehow. Could you please add one, it's quite an easy task, if you look at other scalers. |
@zroubalik I had that in back of my mind but forgot about it. Will add for sure |
Add NATS JetStream scaler
Checklist
Fixes #2391
Documentation kedacore/keda-docs#881