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

feat: Provide Azure Data Explorer Scaler (#1488) #2627

Merged
merged 8 commits into from
Mar 14, 2022

Conversation

yardenoff
Copy link
Contributor

@yardenoff yardenoff commented Feb 12, 2022

Signed-off-by: Yarden Siboni [email protected]

This pull request adds an Azure Data Explorer scaler. This is related to this issue: #1488

Here is the keda-docs PR: kedacore/keda-docs#658

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated and is aligned with our changelog requirements

Relates to #1488

@tomkerkhove
Copy link
Member

Thanks a ton, great addition!

tests/scalers/azure-data-explorer.test.ts Outdated Show resolved Hide resolved
pkg/scalers/azure_data_explorer_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/azure/azure_data_explorer.go Outdated Show resolved Hide resolved
pkg/scalers/azure_data_explorer_scaler.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

zroubalik commented Feb 16, 2022

/run-e2e azure-data-explorer*
Update: You can check the progres here

@yardenoff
Copy link
Contributor Author

/run-e2e azure-data-explorer* Update: You can check the progres here

Failed with 403 Forbidden status code - Principal 'aadapp=***;***' is not authorized to access database '***'

This az cli command can help with that.

@zroubalik
Copy link
Member

/run-e2e azure-data-explorer* Update: You can check the progres here

Failed with 403 Forbidden status code - Principal 'aadapp=***;***' is not authorized to access database '***'

This az cli command can help with that.

@tomkerkhove PTAL

@yardenoff
Copy link
Contributor Author

/run-e2e azure-data-explorer* Update: You can check the progres here

Failed with 403 Forbidden status code - Principal 'aadapp=***;***' is not authorized to access database '***'

This az cli command can help with that.

@tomkerkhove PTAL

Any update with the role assignments?

@tomkerkhove
Copy link
Member

tomkerkhove commented Feb 28, 2022

/run-e2e azure-data-explorer*
Update: You can check the progres here

@tomkerkhove
Copy link
Member

Sorry, I had missed this :/

@tomkerkhove
Copy link
Member

It failed again but not sure why, or I am looking over it.

@yardenoff
Copy link
Contributor Author

yardenoff commented Feb 28, 2022

It failed again but not sure why, or I am looking over it.

It failed to create new namespace.
Can you please check that there's no existing ns with the name test-azure-data-explorer-ns?

@yardenoff
Copy link
Contributor Author

It failed again but not sure why, or I am looking over it.

It failed to create new namespace. Can you please check that there's no existing ns with the name test-azure-data-explorer-ns?

@tomkerkhove, I pushed a commit that will create namespace only if it doesn't exist already.
Can you please run the e2e again?

@zroubalik
Copy link
Member

It failed again but not sure why, or I am looking over it.

It failed to create new namespace. Can you please check that there's no existing ns with the name test-azure-data-explorer-ns?

@tomkerkhove, I pushed a commit that will create namespace only if it doesn't exist already. Can you please run the e2e again?

I think it would be better if it could delete (clean) the namespace if it exists and recreate then.

@yardenoff
Copy link
Contributor Author

yardenoff commented Feb 28, 2022

It failed again but not sure why, or I am looking over it.

It failed to create new namespace. Can you please check that there's no existing ns with the name test-azure-data-explorer-ns?

@tomkerkhove, I pushed a commit that will create namespace only if it doesn't exist already. Can you please run the e2e again?

I think it would be better if it could delete (clean) the namespace if it exists and recreate then.

done

Edit: I miss understood you. Working on cleaning ns if it exists

@tomkerkhove
Copy link
Member

It's still creating it if it doesn't exist though - I like Zbynek his suggestion more though.

@yardenoff
Copy link
Contributor Author

This way it will either clean the ns if it exists or create new one if it doesn't. WDYT?

@tomkerkhove
Copy link
Member

tomkerkhove commented Feb 28, 2022

/run-e2e azure-data-explorer*

Update: You can check the progres here

@tomkerkhove
Copy link
Member

tomkerkhove commented Mar 11, 2022

/run-e2e azure-data-explorer*

Update: You can check the progres here

@tomkerkhove
Copy link
Member

@zroubalik Anything we need as well?

@tomkerkhove
Copy link
Member

@yasiboni-max Would you mind rebasing main 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.

LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
@yardenoff
Copy link
Contributor Author

yardenoff commented Mar 12, 2022

Not sure why changing one line in CHANGELOG.md caused snyk's security check to fail, and it seems like I don't have enough permissions to see the full report. @tomkerkhove - can you help me with that please?

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.

LGTM

@tomkerkhove
Copy link
Member

Not sure why changing one line in CHANGELOG.md caused snyk's security check to fail, and it seems like I don't have enough permissions to see the full report. @tomkerkhove Tom Kerkhove FTE - can you help me with that please?

It failed to process it, no worries. Thanks for checking (unfortunately we can't make reports public due to Snyk limitation)

@tomkerkhove tomkerkhove merged commit 2f3aaa4 into kedacore:main Mar 14, 2022
RamCohen pushed a commit to RamCohen/keda that referenced this pull request Mar 23, 2022
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.

4 participants