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 AWS DynamoDB Scaler #2486

Merged
merged 34 commits into from
Mar 22, 2022
Merged

Add AWS DynamoDB Scaler #2486

merged 34 commits into from
Mar 22, 2022

Conversation

samueleresca
Copy link
Contributor

@samueleresca samueleresca commented Jan 16, 2022

Signed-off-by: Samuele Resca [email protected]

Provide a description of what has been changed

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • 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 Adding dynamodb scaler documentation keda-docs#680
  • Changelog has been updated

Fixes #2482

Signed-off-by: Samuele Resca <[email protected]>
@samueleresca samueleresca marked this pull request as ready for review February 21, 2022 13:12
@samueleresca samueleresca requested a review from a team as a code owner February 21, 2022 13:12
@tomkerkhove
Copy link
Member

Thanks for your PR! Would you mind opening and linking a PR for the docs please?

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 few minor nits on formatting.

Also a big ask is e2e testing. Do you think you can implement e2e tests for this scaler, similar to this https://github.com/kedacore/keda/tree/main/tests? We should have KEDA related AWS account soon CC @tomkerkhove

pkg/scalers/aws_dynamodb_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/aws_dynamodb_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/aws_dynamodb_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/aws_dynamodb_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/aws_dynamodb_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/aws_dynamodb_scaler.go Outdated Show resolved Hide resolved
samueleresca and others added 3 commits February 28, 2022 14:46
Co-authored-by: Zbynek Roubalik <[email protected]>
Signed-off-by: Samuele Resca <[email protected]>
Co-authored-by: Zbynek Roubalik <[email protected]>
Signed-off-by: Samuele Resca <[email protected]>
Co-authored-by: Zbynek Roubalik <[email protected]>
Signed-off-by: Samuele Resca <[email protected]>
@samueleresca
Copy link
Contributor Author

Looking good, just a few minor nits on formatting.

Also a big ask is e2e testing. Do you think you can implement e2e tests for this scaler, similar to this https://github.com/kedacore/keda/tree/main/tests? We should have KEDA related AWS account soon CC @tomkerkhove

I'm happy to implement the e2e tests for this. Should I do it in this PR?

@zroubalik
Copy link
Member

Looking good, just a few minor nits on formatting.
Also a big ask is e2e testing. Do you think you can implement e2e tests for this scaler, similar to this https://github.com/kedacore/keda/tree/main/tests? We should have KEDA related AWS account soon CC @tomkerkhove

I'm happy to implement the e2e tests for this. Should I do it in this PR?

Yes please :)

@samueleresca
Copy link
Contributor Author

Looking good, just a few minor nits on formatting.
Also a big ask is e2e testing. Do you think you can implement e2e tests for this scaler, similar to this https://github.com/kedacore/keda/tree/main/tests? We should have KEDA related AWS account soon CC @tomkerkhove

I'm happy to implement the e2e tests for this. Should I do it in this PR?

Yes please :)

Regarding the e2e testing. I'm considering spanning up an instance using the following image for testing purposes: https://hub.docker.com/r/amazon/dynamodb-local. Until we don't have an AWS account. It should mimic the dynamodb on AWS, except for the fact that multiple regions are not supported

@samueleresca
Copy link
Contributor Author

samueleresca commented Mar 7, 2022

@zroubalik I updated the PR with the tests. I executed the E2E test locally with my personal subscription and looks good to me. Let me know if I can do something else.

@zroubalik
Copy link
Member

@zroubalik I updated the PR with the tests. I executed the E2E test locally with my personal subscription and looks good to me. Let me know if I can do something else.

@JorTurFer @tomkerkhove how should we approach this? Do we know if there's any progress on the AWS account from CNCF?

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.

Nit

tests/scalers/aws-dynamodb.test.ts Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

@zroubalik I updated the PR with the tests. I executed the E2E test locally with my personal subscription and looks good to me. Let me know if I can do something else.

@JorTurFer @tomkerkhove how should we approach this? Do we know if there's any progress on the AWS account from CNCF?

If the e2e test is running, I'd keep it commented and I'd enable it when the account is ready. We already have other scalers without any e2e test, so I wouldn't block this PR.
Other option is using DynamoDB Local but I'm not sure if that development version could be enough for our e2e test case. Even if the local version is enough, I'd prefer using the real version, but this is an option

@zroubalik
Copy link
Member

@zroubalik I updated the PR with the tests. I executed the E2E test locally with my personal subscription and looks good to me. Let me know if I can do something else.

@JorTurFer @tomkerkhove how should we approach this? Do we know if there's any progress on the AWS account from CNCF?

If the e2e test is running, I'd keep it commented and I'd enable it when the account is ready. We already have other scalers without any e2e test, so I wouldn't block this PR. Other option is using DynamoDB Local but I'm not sure if that development version could be enough for our e2e test case. Even if the local version is enough, I'd prefer using the real version, but this is an option

Yeah, let's disable e2e tests for now and enable once we have the account.

@tomkerkhove
Copy link
Member

No AWS account yet indeed. I will check

CHANGELOG.md Outdated Show resolved Hide resolved
tests/scalers/aws-dynamodb.test.ts Show resolved Hide resolved
@zroubalik zroubalik changed the title Add DynamoDB Scaler Add AWS DynamoDB Scaler Mar 16, 2022
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.

You have done an awesome job! 🤟🤟🤟
Thanks for this contribution and all the effort, LGTM
Only 1 small nit inline (and please, rebase main because there is a conflict with one file)

pkg/scalers/aws_dynamodb_test.go Outdated Show resolved Hide resolved
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 huge effort!

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, thanks for the contribution! Great job.

CHANGELOG.md Outdated Show resolved Hide resolved
@zroubalik zroubalik enabled auto-merge (squash) March 22, 2022 09:02
@zroubalik zroubalik disabled auto-merge March 22, 2022 09:03
@zroubalik zroubalik merged commit 6cc380a into kedacore:main Mar 22, 2022
RamCohen pushed a commit to RamCohen/keda that referenced this pull request Mar 23, 2022
Signed-off-by: Samuele Resca <[email protected]>

Co-authored-by: Zbynek Roubalik <[email protected]>
Signed-off-by: Ram Cohen <[email protected]>
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.

AWS DynamoDb scaler
4 participants