Added support for custom feature providers. #79
Conversation
New public API public IFeatureDefinitionProvider; public FeatureDefinition; public FeatureFilterConfiguration;
jimmyca15
force-pushed
the
user/jimmyca/featureSettingsProvider
branch
from
58ab929
to
c38856f
Compare
tests/Tests.FeatureManagement/InMemoryFeatureSettingsProvider.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.FeatureManagement/ConfigurationFeatureDefinitionProvider.cs
Outdated
Show resolved
Hide resolved
|
@zhenlan thanks for the review. I have updated it according to your feedback. |
…finitionProvider.
jimmyca15
force-pushed
the
user/jimmyca/featureSettingsProvider
branch
from
80f0b08
to
40ec2f5
Compare
| /// </summary> | ||
| class FeatureFilterSettings | ||
| public class FeatureFilterConfiguration |
There was a problem hiding this comment.
I am wondering if it will be better if we call this FeatureFilterDefinition.
There was a problem hiding this comment.
The reason I left it as Configuration rather than definition is that there is only one definition for a feature. However the same feature filter might appear in multiple features. They would just be configured differently. So to me, this was the configuration of a feature filter rather than a definition of one.
| services.AddSingleton<IFeatureDefinitionProvider, InMemoryFeatureDefinitionProvider>() | ||
| .AddFeatureManagement() | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Do you like the idea that we add these two APIs so we won't always add ConfigurationFeatureDefinitionProvider to DI and make it easier for users? Or is this too much and we can always add in the future?
AddFeatureManagement<IFeatureDefinitionProvider>()
AddFeatureManagement(IFeatureDefinitionProvider fd)
There was a problem hiding this comment.
I'd go with add in the future for that one.
There was a problem hiding this comment.
Especially if in the future we get a request to provide the capability to pull feature settings from an external source. Then it will point out that maybe the API that you proposed could help with the discoverability that we already offer that option.
|
I updated the settings variables to use the new definition name. Thanks for that. I still couldn't come up with a better name for |
| } | ||
|
|
||
| #pragma warning disable CS1998 // Async method lacks 'await' operators and will run synchronously | ||
| public async IAsyncEnumerable<FeatureDefinition> GetAllFeatureDefinitionsAsync() |
There was a problem hiding this comment.
We should add a test for this API too.
There was a problem hiding this comment.
I agree. Having a test for our ConfigurationFeatureDefinitionProvider.GetAllFeatureDefinitionsAsync would be handy. I'll add one, but in a different PR to prevent making this one bigger.
There was a problem hiding this comment.
I went back and looked and we already have a test for that code path:
It uses our built-in implementation of IFeatureDefinitionProvider, but there's no difference between our implementation of IFeatureDefinitionProvider, and an external one. The CustomFeatureDefinitionProvider test ensures that an external IFeatureDefinitionProvider can be wired up. So it seems to me like the two tests together provide the coverage we need.
IFeatureDefinitionProvidermethods tested- custom
IFeatureDefinitionProvideris registered and consumed successfully
Let's go with it then! |
This PR introduces customization points to enable pulling feature definitions from anywhere. As mentioned in attached issues, this functionality will enable developers to pull feature definitions from existing legacy sources or external vendors that already support feature management.
New public API
public IFeatureDefinitionProvider;public FeatureDefinition;public FeatureFilterConfiguration;Usage
The readme has been updated to explain the steps required to wire up this new extensibility point. A test named
CustomSettingsProviderhas also been added that provides a small example of a custom feature definition provider.Relevant issues:
#57
#46
#8