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

Microsoft SQL Server (MSSQL) scaler implementation #1591

Merged
merged 6 commits into from
Feb 23, 2021

Conversation

cgillum
Copy link
Contributor

@cgillum cgillum commented Feb 10, 2021

This PR adds a built-in scaler that uses a Microsoft SQL Server (MSSQL) database as the event source. It's very similar to both the existing mysql and postgresql scalers and is intended to work with both self-hosted SQL Server databases and SQL Server databases hosted in managed clouds.

Documentation PR: kedacore/keda-docs#367

There is still more work to do on this PR, which is tracked in the checklist below. This also includes end-to-end testing. However, I wanted to open it early to hopefully get some early feedback since this is both my first contribution to KEDA and my first time writing code in Go (besides hello world). Feedback is greatly appreciated. 🙏🏽

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Unit tests have been added
  • End-to-end tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified) (doesn't seem like this is necessary)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

Fixes #674

@cgillum cgillum marked this pull request as draft February 10, 2021 22:05
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Thanks for bringing this upstream @cgillum!

Can you please:

Thanks!

@tomkerkhove
Copy link
Member

tomkerkhove commented Feb 15, 2021

@cgillum Can we mark the PR as ready for review or is there any open work?

@cgillum
Copy link
Contributor Author

cgillum commented Feb 16, 2021

@tomkerkhove still working on putting together end-to-end tests for this one. I'm learning as I go (first time deploying a real k8s cluster), so apologies for the delay.

@tomkerkhove
Copy link
Member

Oh no worries at all! I was just checking if you were waiting on us or not!

@lgmorand
Copy link

@cgillum can't help you but you have my moral support to finish your e2e test :) this scaler would be great to have

@cgillum cgillum marked this pull request as ready for review February 19, 2021 18:50
@cgillum
Copy link
Contributor Author

cgillum commented Feb 19, 2021

@tomkerkhove and @ahmelsayed, I was able to complete my e2e testing successfully and I think this PR is ready to go. Would it be okay to defer the authoring of the e2e test for another PR or should that also be part of this PR?

@ahmelsayed
Copy link
Contributor

@cgillum yes that's fine, e2e test can come in a separate PR

Signed-off-by: Chris Gillum <[email protected]>
@cgillum
Copy link
Contributor Author

cgillum commented Feb 20, 2021

I ended up getting an end-to-end test working sooner than I thought, so I went ahead and added it to this PR in my latest iteration. It's basically the same as the mysql test with a few tweaks and simplifications. Please let me know if the way that it is written is acceptable, especially regarding the dependency on my docker hub test container image.

Signed-off-by: Chris Gillum <[email protected]>
@tomkerkhove
Copy link
Member

If you have a test container, we could host it on our test container repo if you want?

@cgillum
Copy link
Contributor Author

cgillum commented Feb 22, 2021

It probably makes more sense to eventually host it in a KEDA-specific container repo so that I don't accidentally break anything in the future. That said, this doesn't have to happen right away. We could move it as part of a separate PR once we know that the nightly tests are stable.

@tomkerkhove
Copy link
Member

Sure thing, no worries! Will wait for @ahmelsayed his review.

Thanks!

@ahmelsayed ahmelsayed merged commit 8f27289 into kedacore:main Feb 23, 2021
ycabrer pushed a commit to ycabrer/keda that referenced this pull request Mar 1, 2021
* Initial implementation and tests for mssql scaler

Signed-off-by: Chris Gillum <[email protected]>

* PR feedback and minor doc updates

Signed-off-by: Chris Gillum <[email protected]>

* Fixed mssql driver loading issue

Signed-off-by: Chris Gillum <[email protected]>

* Added end-to-end test

Signed-off-by: Chris Gillum <[email protected]>

* Removing trailing whitespace

Signed-off-by: Chris Gillum <[email protected]>
Rodolfodc pushed a commit to sidilabs/keda that referenced this pull request Mar 11, 2021
* Initial implementation and tests for mssql scaler

Signed-off-by: Chris Gillum <[email protected]>

* PR feedback and minor doc updates

Signed-off-by: Chris Gillum <[email protected]>

* Fixed mssql driver loading issue

Signed-off-by: Chris Gillum <[email protected]>

* Added end-to-end test

Signed-off-by: Chris Gillum <[email protected]>

* Removing trailing whitespace

Signed-off-by: Chris Gillum <[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.

Provide SQL Server scaler
4 participants