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

Add NATS JetStream scaler #3468

Merged
merged 3 commits into from
Aug 8, 2022
Merged

Add NATS JetStream scaler #3468

merged 3 commits into from
Aug 8, 2022

Conversation

goku321
Copy link
Contributor

@goku321 goku321 commented Aug 2, 2022

Add NATS JetStream scaler

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • [N/A] A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #2391

Documentation kedacore/keda-docs#881

@goku321 goku321 requested a review from a team as a code owner August 2, 2022 08:16
@goku321
Copy link
Contributor Author

goku321 commented Aug 2, 2022

I tested this scaler using kind and could only test with maxReplicaCount: 2 due to limited hardware resources. Will create a docs PR soon. Also, I realized that I've missed unit tests. Will add them as well.

@goku321 goku321 changed the title Nats jetstream Add NATS JetStream scaler Aug 2, 2022
pkg/scalers/nats_jetstream_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/nats_jetstream_scaler.go Show resolved Hide resolved
tests/scalers/nats-jetstream-helpers.ts Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

@goku321 any update on this please?

@goku321
Copy link
Contributor Author

goku321 commented Aug 4, 2022

@zroubalik I will work on the changes today/tomorrow and make it ready to be merged :)

@zroubalik
Copy link
Member

@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.

@goku321
Copy link
Contributor Author

goku321 commented Aug 8, 2022

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 😅

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.

looking good, just a couple of minor nits.

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/scalers/nats_jetstream_scaler.go Outdated Show resolved Hide resolved
tests/scalers/nats/nats_jetstream/nats_jetstream_test.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

@zroubalik
Copy link
Member

zroubalik commented Aug 8, 2022

/run-e2e nats_jetstream_test*
Update: You can check the progress here

@zroubalik
Copy link
Member

@goku321 could you please rebase your branch agains main? There has been some updates recently (we haver removed npm) that are failing here.

@zroubalik
Copy link
Member

@goku321 also linter is failing, you can check it locally by running: make golangci

@goku321
Copy link
Contributor Author

goku321 commented Aug 8, 2022

@zroubalik linter is complaining for duplicate code b/w stan and nats_jetstream scaler. Some part is overlapping

@zroubalik
Copy link
Member

@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:

- dupl

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>
@goku321
Copy link
Contributor Author

goku321 commented Aug 8, 2022

@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?

@zroubalik
Copy link
Member

@goku321 yeah, I can squash those commits. I just don't want to have any unrelated commits here.

@goku321
Copy link
Contributor Author

goku321 commented Aug 8, 2022

@zroubalik Also, I updated .golangci.yml for "nats_jetstream_scaler". Do I need to add a path for "stan_scaler" as well?

@zroubalik
Copy link
Member

zroubalik commented Aug 8, 2022

@zroubalik Also, I updated .golangci.yml for "nats_jetstream_scaler". Do I need to add a path for "stan_scaler" as well?

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>
@zroubalik
Copy link
Member

zroubalik commented Aug 8, 2022

/run-e2e nats_jetstream_test*
Update: You can check the progress here

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.

LGTM

@zroubalik
Copy link
Member

@goku321 just the Documentation is missing.

@goku321
Copy link
Contributor Author

goku321 commented Aug 8, 2022

@zroubalik Working on the documentation 🙂

@zroubalik
Copy link
Member

@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

@goku321
Copy link
Contributor Author

goku321 commented Aug 8, 2022

@zroubalik Added the documentation. Thanks for your patience 😅

@zroubalik zroubalik merged commit b9ec02e into kedacore:main Aug 8, 2022
@zroubalik
Copy link
Member

@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.

@goku321
Copy link
Contributor Author

goku321 commented Aug 8, 2022

@zroubalik I had that in back of my mind but forgot about it. Will add for sure

@goku321 goku321 deleted the nats-jetstream branch August 9, 2022 05:52
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.

Provide scaler for NATS Jetstream
2 participants