-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adding external cosmos db scaler #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start but think we need some more work.
Main topics are:
- Please add a CI on GitHub Actions
- Removing the deprecated approach and use new instead
- Use connection string parser from official SDK, if available
- Create a file per class instead
Thanks for the PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! Added few comments. Please add unit test project as well.
public string AccountName { get; internal set; } | ||
} | ||
|
||
internal class CosmosDbTriggerMetadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to expose these through config via Options - similar to https://github.com/Azure/azure-webjobs-sdk-extensions/blob/dev/src/WebJobs.Extensions.CosmosDB/Config/CosmosDBOptions.cs
regarding @tomkerkhove note
@divyagandhisethi I can help set this up if you're not familiar with github actions. |
Thanks. As discussed we will open a tracking issue for adding CI |
@tomkerkhove Can you please take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added few more comments. Thanks!
Created an issue for CI - #2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more things that I'd change to build a solid foundation, but almost there.
Also opened an issue for integration tests: #4
CosmosDBTrigger trigger) | ||
{ | ||
CosmosDBConnectionString triggerConnection = new CosmosDBConnectionString(trigger.CosmosDBConnectionString); | ||
DocumentCollectionInfo documentCollectionLocation = new DocumentCollectionInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We definately should do that
return builder; | ||
} | ||
|
||
private static string NormalizeString(string inputString) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move it to Extensions/StringExtensions
instead, otherwise Utilities will become a dump :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting partial feedback.
Main feedback in this iteration - move to https://github.com/Azure/azure-cosmos-dotnet-v3
Keda.Cosmosdb.Scaler/src/Repository/ChangeFeedProcessorBuilderFactory.cs
Outdated
Show resolved
Hide resolved
Keda.Cosmosdb.Scaler/src/Repository/ChangeFeedEstimatorFactory.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more feedback. Please ping once review is addressed
Keda.Cosmosdb.Scaler/src/Repository/ChangeFeedEstimatorFactory.cs
Outdated
Show resolved
Hide resolved
Keda.Cosmosdb.Scaler/src/Repository/ChangeFeedEstimatorFactory.cs
Outdated
Show resolved
Hide resolved
Keda.Cosmosdb.Scaler/src/Repository/ChangeFeedEstimatorFactory.cs
Outdated
Show resolved
Hide resolved
Keda.CosmosDB.Scaler/src/Repository/ChangeFeedEstimatorFactory.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added few more comments. Please open an issue here to add E2E test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good. Please open following issues:
- Add E2E test in K4Apps
- Add readme in this repo - brief description and getting started instructions
- If you do not have already, please follow up setting up CI to produce the right artifacts needed - I recommend publishing artifacts with -beta or -preview in the version / name
Issue for readme - #3 |
@tomkerkhove This PR is blocked on your review. Please do take a look when possible. |
Let's have a GitHub action to build & push an image to GHCR similar to our core. We use a different |
@divyagandhisethi Would you mind fixing DCO please? |
What is the status on this PR? |
@divyagandhisethi - Please merge the PR once you address DCO |
cc @JatinSanghvi and @SushmithaVReddy |
You are so close... you can do this! 👍🏻 |
@divyagandhisethi could you just address DCO? |
Signed-off-by: Divya Gandhi <[email protected]>
Signed-off-by: Divya Gandhi <[email protected]>
Signed-off-by: Divya Gandhi <[email protected]>
Signed-off-by: Divya Gandhi <[email protected]>
Signed-off-by: Divya Gandhi <[email protected]>
Signed-off-by: Divya Gandhi <[email protected]>
Signed-off-by: Divya Gandhi <[email protected]>
Signed-off-by: Divya Gandhi <[email protected]>
Signed-off-by: Divya Gandhi <[email protected]>
Signed-off-by: Divya Gandhi <[email protected]>
Thanks @eskaufel . |
Thanks @borqosky . The commits are now signed. I don't have write permissions on the repo, so can't merge this PR in. |
@pragnagopa @JatinSanghvi @SushmithaVReddy - please follow up on this and publish this image as an experimental scaler. |
@pragnagopa @JatinSanghvi @SushmithaVReddy A PR needs to be open against the docs repo |
This is only for built-in scalers but feel free to tackle #7 and use documentation in the README! |
We are planning to surface external scalers on KEDA.sh based on Artifact Hub |
Happy to merge if you want. |
Thanks @tomkerkhove . Yes, please go ahead and merge it in. |
Provide a description of what has been changed
Checklist
Fixes #