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

Adding external cosmos db scaler #1

Merged
merged 10 commits into from
Aug 19, 2021
Merged

Conversation

divyagandhisethi
Copy link
Contributor

@divyagandhisethi divyagandhisethi commented Mar 5, 2021

Provide a description of what has been changed

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)
  • A PR is opened to update the documentation on our docs repo

Fixes #

@divyagandhisethi
Copy link
Contributor Author

@pragnagopa

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.

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!

Keda.Cosmosdb.Scaler/src/ConsoleLogger.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDbTrigger.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDbTrigger.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/ExternalScalerService.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Startup.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDbTrigger.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDbTrigger.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/ExternalScalerService.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/ExternalScalerService.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/ExternalScalerService.cs Outdated Show resolved Hide resolved
Copy link

@pragnagopa pragnagopa 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 the PR! Added few comments. Please add unit test project as well.

Keda.Cosmosdb.Scaler/Dockerfile Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/ConsoleLogger.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Program.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Program.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Protos/externalscaler.proto Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDbTrigger.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDbTrigger.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDbTrigger.cs Outdated Show resolved Hide resolved
public string AccountName { get; internal set; }
}

internal class CosmosDbTriggerMetadata

Choose a reason for hiding this comment

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

Keda.Cosmosdb.Scaler/src/Services/ExternalScalerService.cs Outdated Show resolved Hide resolved
@ahmelsayed
Copy link
Contributor

regarding @tomkerkhove note

Please add a CI on GitHub Actions

@divyagandhisethi I can help set this up if you're not familiar with github actions.

@divyagandhisethi
Copy link
Contributor Author

regarding @tomkerkhove note

Please add a CI on GitHub Actions

@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

@divyagandhisethi
Copy link
Contributor Author

@tomkerkhove Can you please take a look?

Copy link

@pragnagopa pragnagopa left a 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!

Keda.Cosmosdb.Scaler/Dockerfile Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Protos/externalscaler.proto Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDBLeaseMetadata.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDbTrigger.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Services/CosmosDbTrigger.cs Outdated Show resolved Hide resolved
@tomkerkhove
Copy link
Member

Created an issue for CI - #2

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.

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
Copy link
Member

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)
Copy link
Member

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

Copy link

@pragnagopa pragnagopa left a 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/Extensions/StringExtensions.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Keda.Cosmosdb.Scaler.csproj Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Program.cs Outdated Show resolved Hide resolved
Copy link

@pragnagopa pragnagopa left a 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/Extensions/StringExtensions.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Keda.Cosmosdb.Scaler.csproj Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/src/Constants.cs Outdated Show resolved Hide resolved
Keda.Cosmosdb.Scaler/Dockerfile Outdated Show resolved Hide resolved
Copy link

@pragnagopa pragnagopa left a 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

Keda.CosmosDB.Scaler/src/Services/CosmosDBTrigger.cs Outdated Show resolved Hide resolved
Copy link

@pragnagopa pragnagopa left a 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

@divyagandhisethi
Copy link
Contributor Author

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
Issue for integration tests - #4
For CI artifacts - we can generate the preview image manually until we have the CI setup

@divyagandhisethi
Copy link
Contributor Author

@tomkerkhove This PR is blocked on your review. Please do take a look when possible.

@tomkerkhove
Copy link
Member

For CI artifacts - we can generate the preview image manually until we have the CI setup

Let's have a GitHub action to build & push an image to GHCR similar to our core. We use a different experimental tag AFAIK

@tomkerkhove
Copy link
Member

@divyagandhisethi Would you mind fixing DCO please?

@tomkerkhove
Copy link
Member

What is the status on this PR?

@pragnagopa
Copy link

@divyagandhisethi - Please merge the PR once you address DCO

@pragnagopa
Copy link

cc @JatinSanghvi and @SushmithaVReddy

@eskaufel
Copy link

You are so close... you can do this! 👍🏻

@borqosky
Copy link

@divyagandhisethi could you just address DCO?
THIS PR is really interesting and as soon you merge it, I'm going to try it 😺

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]>
@divyagandhisethi
Copy link
Contributor Author

You are so close... you can do this! 👍🏻

Thanks @eskaufel .

@divyagandhisethi
Copy link
Contributor Author

@divyagandhisethi could you just address DCO?
THIS PR is really interesting and as soon you merge it, I'm going to try it 😺

Thanks @borqosky . The commits are now signed. I don't have write permissions on the repo, so can't merge this PR in.

@divyagandhisethi
Copy link
Contributor Author

divyagandhisethi commented Aug 19, 2021

For CI artifacts - we can generate the preview image manually until we have the CI setup

Let's have a GitHub action to build & push an image to GHCR similar to our core. We use a different experimental tag AFAIK

@pragnagopa @JatinSanghvi @SushmithaVReddy - please follow up on this and publish this image as an experimental scaler.

@divyagandhisethi
Copy link
Contributor Author

@pragnagopa @JatinSanghvi @SushmithaVReddy A PR needs to be open against the docs repo

@tomkerkhove
Copy link
Member

@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!

@tomkerkhove
Copy link
Member

We are planning to surface external scalers on KEDA.sh based on Artifact Hub

@tomkerkhove
Copy link
Member

@divyagandhisethi could you just address DCO?
THIS PR is really interesting and as soon you merge it, I'm going to try it 😺

Thanks @borqosky . The commits are now signed. I don't have write permissions on the repo, so can't merge this PR in.

Happy to merge if you want.

@divyagandhisethi
Copy link
Contributor Author

@divyagandhisethi could you just address DCO?
THIS PR is really interesting and as soon you merge it, I'm going to try it 😺

Thanks @borqosky . The commits are now signed. I don't have write permissions on the repo, so can't merge this PR in.

Happy to merge if you want.

Thanks @tomkerkhove . Yes, please go ahead and merge it in.

@ahmelsayed ahmelsayed merged commit bcade15 into kedacore:main Aug 19, 2021
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.

8 participants