-
Notifications
You must be signed in to change notification settings - Fork 119
Merge feature flags from different configuration source #536
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
src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs
Outdated
Show resolved
Hide resolved
…-Dotnet into zhiyuanliang/merge-ff-source
…-Dotnet into zhiyuanliang/merge-ff-source
| // IFeatureDefinitionProviderCacheable interface is only used to mark this provider as cacheable. This allows our test suite's | ||
| // provider to be marked for caching as well. | ||
|
|
||
| private static readonly PropertyInfo _propertyInfo = typeof(ConfigurationProvider).GetProperty("Data", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); |
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.
Can you put a comment that compile time dependency ensures presence of this property via OnDemandConfigurationProvider.Data
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 wish we could really take advantage of the compile time dependency. Like if we could have a static method inside of OnDemandConfigurationProvider that uses nameof(data) to enforce the dependency.
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.
internal class OnDemandConfigurationProvider : ConfigurationProvider
{
private static readonly PropertyInfo _propertyInfo = typeof(ConfigurationProvider).GetProperty(nameof(Data), BindingFlags.NonPublic | BindingFlags.Instance);
public OnDemandConfigurationProvider(ConfigurationProvider configurationProvider)
{
var data = _propertyInfo.GetValue(configurationProvider) as IDictionary<string, string>;
Data = data;
}
}Is this what you want? @jimmyca15
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
This PR implements merging of feature flags from multiple configuration sources by adding new JSON configuration files, updating dependency versions, and enhancing the ConfigurationFeatureDefinitionProvider to handle on‑demand flag extraction.
- Added three new appsettings JSON files with varying feature flag values.
- Updated test projects and added a new integration test to validate feature flag merging behavior.
- Enhanced the provider logic in the main library to aggregate feature definitions from chained configuration sources.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Tests.FeatureManagement/appsettings1.json, appsettings2.json, appsettings3.json | New JSON configuration files with feature flag definitions |
| tests/Tests.FeatureManagement/Tests.FeatureManagement.csproj, tests/Tests.FeatureManagement.AspNetCore/Tests.FeatureManagement.AspNetCore.csproj | Upgraded dependency versions and ensured configuration files copy to output |
| tests/Tests.FeatureManagement/FeatureManagementTest.cs | Added integration test verifying the merged configuration behavior |
| src/Microsoft.FeatureManagement/OnDemandConfigurationProvider.cs | Introduced a provider to enable on‑demand configuration extraction |
| src/Microsoft.FeatureManagement/Microsoft.FeatureManagement.csproj | Upgraded dependency versions to align with new features and fixes |
| src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs | Updated logic to cache and refresh feature definition sections from various configuration sources |
Why this PR?
#507