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

Maintain Default Data Source Name from Original Config after Hot Reloading #2069

Merged
merged 23 commits into from
Mar 13, 2024

Conversation

aaronburtle
Copy link
Contributor

Why make this change?

Closes #1967

We maintain the same default data source name after hot reloading.

What is this change?

When hot reloading, pass along the original default data source name, and after creating the runtimeconfig object, update that name and the dependent data structures to use the original data source name from before the hot reload.

How was this tested?

  • Unit Tests
    Added validation that we maintain the same default data source name to the unit tests.

Sample Request(s)

  • Example REST and/or GraphQL request to demonstrate modifications
  • Example of CLI usage to demonstrate modifications

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Needs better integration into existing code.

src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Core/Configurations/ConfigFileWatcher.cs Outdated Show resolved Hide resolved
src/Core/Configurations/RuntimeConfigProvider.cs Outdated Show resolved Hide resolved
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.

Some suggestions and questions. Also pending Ani's feedback asking about state and availability of DefaultDataSourceName value.

src/Config/FileSystemRuntimeConfigLoader.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/FileSystemRuntimeConfigLoader.cs Show resolved Hide resolved
src/Config/RuntimeConfigLoader.cs Show resolved Hide resolved
src/Core/Configurations/ConfigFileWatcher.cs Outdated Show resolved Hide resolved
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.

Some questions about approach in latest changeset

src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Core/Configurations/RuntimeConfigProvider.cs Outdated Show resolved Hide resolved
src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
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.

pending fixes to unit tests

src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
@aaronburtle
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@aaronburtle
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for addressing all comments

src/Config/ObjectModel/RuntimeConfig.cs Outdated Show resolved Hide resolved
src/Service.Tests/Unittests/ConfigFileWatcherUnitTests.cs Outdated Show resolved Hide resolved
@aaronburtle
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@aaronburtle
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@aaronburtle aaronburtle enabled auto-merge (squash) March 13, 2024 18:04
@aaronburtle aaronburtle merged commit 40142af into main Mar 13, 2024
9 checks passed
@aaronburtle aaronburtle deleted the dev/aaronburtle/SaveDataSourceNameForHotReload branch March 13, 2024 18:38
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.

Change default data source name for Hot Reload
3 participants