feat(mssql): add driverName support for azuresql driver#7567
feat(mssql): add driverName support for azuresql driver#7567akarki2005 wants to merge 8 commits intokedacore:mainfrom
Conversation
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
e8a507f to
5d73b28
Compare
Signed-off-by: Aayush Karki <akarki2005@gmail.com>
5d73b28 to
5fde208
Compare
Signed-off-by: Aayush Karki <akarki2005@gmail.com>
|
/run-e2e mssql |
dttung2905
left a comment
There was a problem hiding this comment.
lgtm . Thanks @akarki2005 for the contribution. Can we make some changes to the integration test too ? https://github.com/kedacore/keda/blob/main/tests/scalers/mssql/mssql_test.go
Signed-off-by: Aayush Karki <akarki2005@gmail.com>
Signed-off-by: Aayush Karki <akarki2005@gmail.com>
Signed-off-by: Aayush Karki <akarki2005@gmail.com>
|
Hi, I've parameterized the integration tests by driver so |
|
/run-e2e mssql |
Signed-off-by: Aayush Karki <akarki2005@gmail.com>
|
Hi @rickbrouwer, I've pushed a patch for the test teardown issue causing the previous e2e failure (the logs showed a duplicated cleanup of the test namespace). It should be ready for another run whenever convenient. |
|
/run-e2e mssql |
|
Hi @rickbrouwer - just following up on this PR. I’ve addressed the earlier feedback (including enum validation and the integration tests), and the latest e2e run is passing. I noticed the previous change request is now marked as outdated, but the PR is still blocked by that review. Could you take another look when you have a chance? Happy to make any additional changes if needed. |
|
which PR do you mean that is outdated? |
|
Hi @rickbrouwer, sorry for the confusion - I meant to say the earlier review comments on this PR are now resolved. I was hoping for another review on this PR to check if everything looks good or if there's anything else I should address. Thanks! |
|
I will review your PR soon. Sometimes that takes a bit longer due to other work and other reviews. The maintainers and reviewers will see that all checks are green and will surely do a review soon as well. |
rickbrouwer
left a comment
There was a problem hiding this comment.
The unit tests are incorrect regarding expectedConnectionString. However, the test passed. I suspect there is an error somewhere in the test causing the check to be incorrect. Can you check that?
|
@dttung2905 asked for an E2E test. The drivers are currently being tested in the E2E, so that is great. However, I think it would be valuable if we specifically included Azure AD Auth (managed identity, workload identity, etc.) in the E2E test via the Azure SQL driver. |
|
I totally agree with @rickbrouwer on this |
Signed-off-by: Aayush Karki <akarki2005@gmail.com>
|
Hi, thanks so much for the detailed feedback and suggestions - I've made some changes to fix the identified gaps.
Happy to make additional adjustments to the test structure or coverage if needed. |
…r-driver-name-param
b1f4084 to
90ce961
Compare
| spec: | ||
| secretTargetRef: | ||
| - parameter: password | ||
| name: {{.SecretName}} | ||
| key: mssql-sa-password |
There was a problem hiding this comment.
| spec: | |
| secretTargetRef: | |
| - parameter: password | |
| name: {{.SecretName}} | |
| key: mssql-sa-password | |
| spec: | |
| secretTargetRef: | |
| - parameter: password | |
| name: {{.SecretName}} | |
| key: mssql-sa-password |
| "replica count should be %d after 3 minutes", data.MaxReplicaCount) | ||
| } | ||
|
|
||
| func testScaleIn(t *testing.T, kc *kubernetes.Clientset, data templateData) { |
There was a problem hiding this comment.
I'm taking a quick look at how this is going to work. I see an activation of 10 and another 10 in the scaleout. But shouldn't those rows be cleared to ScaleIn?
| } | ||
|
|
||
| type mssqlMetadata struct { | ||
| DriverName string `keda:"name=driverName, order=authParams;triggerMetadata, enum=sqlserver;azuresql, default=sqlserver"` |
There was a problem hiding this comment.
There are some tabs and spaces between driverName and the order. Can you make them spaces only?
|
Any status @akarki2005 ? |
This PR adds optional
driverNamesupport to the MSSQL scaler, allowing users to use theazuresqldriver for Azure AD–based authentication.If not specified, it defaults to
sqlserver, so existing configurations remain unchanged.Checklist
make generate-scalers-schemahas been run to update any outdated generated filesFixes #7412
Related Docs: kedacore/keda-docs#1722