Skip to content

Conversation

@mbhaskar
Copy link
Member

@mbhaskar mbhaskar commented Nov 29, 2022

Why make this change?

Fixes #959

Reading comsos options from data-source/options instead of "cosmos" object.
Follow up of PR #993

What is this change?

This PR initalizes comsos metadata provider with cosmos database, container and schema options read from data-source/options path

How was this tested?

Existing test should be good.

  • Integration Tests
  • Unit Tests

Sample Request(s)

@seantleonard
Copy link
Contributor

Do any of the existing CosmosDB config files need to be changed to be compatible with this change? Doesn't this modify how the config file should be parsed, which maybe I missed config file changes in a different PR?

@mbhaskar
Copy link
Member Author

Do any of the existing CosmosDB config files need to be changed to be compatible with this change? Doesn't this modify how the config file should be parsed, which maybe I missed config file changes in a different PR?

The dab-config file is the one that changes. Ayush had a PR that changes CLI to generate options inside the data-source

Copy link
Contributor

@ayush3797 ayush3797 left a comment

Choose a reason for hiding this comment

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

LGTM!

@mbhaskar mbhaskar merged commit 42c65e2 into main Dec 1, 2022
@mbhaskar mbhaskar deleted the mbhaskar/cosmos-options branch December 1, 2022 17:23
_graphQLSingularTypeToEntityNameMap = _runtimeConfig.GraphQLSingularTypeToEntityNameMap;

CosmosDbOptions? cosmosDb = _runtimeConfig.CosmosDb;
CosmosDbOptions? cosmosDb = _runtimeConfig.DataSource.CosmosDbNoSql;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this section looks like it could be simplified. the conditional could just check _runtimeConfig.DataSource.CosmosDbNoSql for null and assign that value to _cosmosDb if not null.

Copy link
Contributor

@seantleonard seantleonard left a comment

Choose a reason for hiding this comment

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

one nit

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.

Modify and consume the new consolidated config file format with updated data-source section

5 participants