-
Notifications
You must be signed in to change notification settings - Fork 69
Add a static config accessor that can help with migration #618
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
Conversation
This enables switching from `ConfigurationManager.[Appsettings|ConnectionStrings]` to `AppConfiguration.[GetSetting|GetConnectionString]` so that there is a static configuration source on all platforms while adapting to more modern practices of dependency injection for configuration.
| /// </summary> | ||
| /// <param name="key">The key of the settings.</param> | ||
| /// <returns>The setting if available, otherwise <c>null</c>.</returns> | ||
| public static string? GetSetting(string key) => Configuration?.GetSetting(key); |
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.
Do we need to make a remark on the requirements of the string parameter? Pretty sure there are keys that are not supported so I wonder if we should either do some sanitization or document we assume it is a valid key.
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 don't know of any IConfiguration[index] constraints. If there are, we can document it but it's nothing that doesn't already exist there as this is just a pass through since we don't want to add the dependency to IConfiguration for consumers of the Microsoft.AspNetCore.SystemWebAdapters package
joperezr
left a comment
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.
Left a few minor comments but looks good otherwise
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.
Pull Request Overview
Adds a static AppConfiguration API and supporting infrastructure to enable migration from ConfigurationManager to IConfiguration across Core and Framework targets. Introduces an internal IConfigurationAccessor service and registers it during system web adapter setup; provides samples and end-to-end tests validating behavior; adjusts host startup to ensure services are available before returning.
- Introduces AppConfiguration static helper and IConfigurationAccessor interface.
- Registers configuration accessor in Core and Framework paths and adds samples (Core, Framework, AppHost) plus E2E tests.
- Modifies host builder to wait for application start before returning.
Reviewed Changes
Copilot reviewed 32 out of 35 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/Microsoft.AspNetCore.SystemWebAdapters.E2E.Tests/Microsoft.AspNetCore.SystemWebAdapters.E2E.Tests.csproj | Adds project reference to new AppConfig sample for E2E testing. |
| test/Microsoft.AspNetCore.SystemWebAdapters.E2E.Tests/AppConfigTests.cs | New tests validating static configuration accessor across Core and Framework apps. |
| src/Services/ConfigurationAccessorExtensions.cs | Adds internal extension to register IConfigurationAccessor implementation. |
| src/Microsoft.AspNetCore.SystemWebAdapters/Microsoft.AspNetCore.SystemWebAdapters.csproj | Includes new source files for AppConfiguration and IConfigurationAccessor; adds InternalsVisibleTo for FrameworkServices. |
| src/Microsoft.AspNetCore.SystemWebAdapters/AppConfiguration.cs | Adds static AppConfiguration API with XML docs. |
| src/Microsoft.AspNetCore.SystemWebAdapters/Adapters/IConfigurationAccessor.cs | Defines internal IConfigurationAccessor interface. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/SystemWebAdapterConfiguration.cs | Registers configuration accessor during adapter setup. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/Hosting/HttpApplicationHostBuilder.cs | Waits for application start using TaskCompletionSource after queuing host run. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/Hosting/HttpApplicationHost.cs | Registers configuration accessor in host creation. |
| src/Microsoft.AspNetCore.SystemWebAdapters.FrameworkServices/Hosting/ConfigurationManagerConfigExtensions.cs | Refactors enumeration of connection strings and app settings to strongly typed loops. |
| src/Microsoft.AspNetCore.SystemWebAdapters.CoreServices/SystemWebAdaptersExtensions.cs | Registers configuration accessor in Core services setup. |
| samples/AppConfig/... (multiple) | Adds Core and Framework sample apps plus AppHost to demonstrate configuration migration. |
| Microsoft.AspNetCore.SystemWebAdapters.sln | Adds solution folder and projects for new AppConfig samples. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...crosoft.AspNetCore.SystemWebAdapters.FrameworkServices/Hosting/HttpApplicationHostBuilder.cs
Show resolved
Hide resolved
src/Microsoft.AspNetCore.SystemWebAdapters/Adapters/IConfigurationAccessor.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNetCore.SystemWebAdapters/Adapters/IConfigurationAccessor.cs
Show resolved
Hide resolved
src/Microsoft.AspNetCore.SystemWebAdapters/Adapters/IConfigurationAccessor.cs
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…tionAccessor.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This enables switching from
ConfigurationManager.[Appsettings|ConnectionStrings]toAppConfiguration.[GetSetting|GetConnectionString]so that there is a static configuration source on all platforms while adapting to more modern practices of dependency injection for configuration.