Skip to content

CosmosDb provider support for a current region option#15009

Merged
ajcvickers merged 6 commits intodotnet:masterfrom
dimangulov:enhancements/cosmosdbcurrentregion
Mar 20, 2019
Merged

CosmosDb provider support for a current region option#15009
ajcvickers merged 6 commits intodotnet:masterfrom
dimangulov:enhancements/cosmosdbcurrentregion

Conversation

@dimangulov
Copy link
Copy Markdown
Contributor

@dimangulov dimangulov commented Mar 13, 2019

CosmosDb provider doesn't support a configuration option for a current region

Fix #15007

Please check if the PR fulfills these requirements

  • The code builds and tests pass (verified by our automated build checks)
  • Commit messages follow this format
    Summary of the changes
    - Detail 1
    - Detail 2

    Fixes #bugnumber

Please review the guidelines for CONTRIBUTING.md for more details.

CosmosDb provider doesn't support a configuration option for a current region

Fix #15007
@dnfclas
Copy link
Copy Markdown

dnfclas commented Mar 13, 2019

CLA assistant check
All CLA requirements met.

@ajcvickers
Copy link
Copy Markdown
Contributor

@dimangulov We looked at this in triage and the general idea seems fine. However, because "region" is optional, could we make setting it be a method on CosmosDbContextOptionsBuilder instead of a parameter that is passed to UseCosmos()? (We generally try to avoid adding too many parameters to UseProvider methods since it can quickly become unwieldy. )

@ajcvickers ajcvickers self-assigned this Mar 15, 2019
@dimangulov
Copy link
Copy Markdown
Contributor Author

dimangulov commented Mar 15, 2019 via email

CosmosDb provider doesn't support a configuration option for a current region

Fix #15007
@dimangulov
Copy link
Copy Markdown
Contributor Author

@ajcvickers Done. Does it look better now?

Copy link
Copy Markdown
Contributor

@ajcvickers ajcvickers left a comment

Choose a reason for hiding this comment

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

Can you add at least one test that makes sure that the region gets set correctly? @AndriySvyryd can likely point you to the appropriate place.

private string _region;

public CosmosDbOptionsExtension()
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to copy _region onto the new object in the copy-constructor below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is done

@AndriySvyryd
Copy link
Copy Markdown
Member

@dimangulov You can add a test in CosmosEndToEndTest that uses some invalid region and asserts that a "Current location is not a valid Azure region." exception is thrown.

dimangulov and others added 3 commits March 20, 2019 09:55
(cosmosdb emulator doesn't work on MacOS)
(cosmosdb emulator doesn't work on MacOS)
Unit and end-to-end tests for the CosmosDbOptionsExtension.WithRegion configuration method

Fix aspnet#15007
@dimangulov
Copy link
Copy Markdown
Contributor Author

Added 2 end to end and 2 unit tests, let me know if there is smth else missed

Copy link
Copy Markdown
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

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

Also rebase and squash the commits

@ajcvickers ajcvickers merged commit 0805798 into dotnet:master Mar 20, 2019
@ajcvickers
Copy link
Copy Markdown
Contributor

Thanks @dimangulov!

@dimangulov
Copy link
Copy Markdown
Contributor Author

dimangulov commented Mar 21, 2019 via email

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.

CosmosDb provider doesn't support a configuration option for a current region

4 participants