-
Notifications
You must be signed in to change notification settings - Fork 114
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
Added support for custom feature providers. #79
Added support for custom feature providers. #79
Conversation
New public API public IFeatureDefinitionProvider; public FeatureDefinition; public FeatureFilterConfiguration;
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.
80f0b08
to
40ec2f5
Compare
/// </summary> | ||
class FeatureFilterSettings | ||
public class FeatureFilterConfiguration |
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 am wondering if it will be better if we call this FeatureFilterDefinition
.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with add in the future for that one.
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.
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.
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.
Agreed. Makes sense.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a test for this API too.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and looked and we already have a test for that code path:
await foreach (string feature in featureManager.GetFeatureNamesAsync()) |
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.
IFeatureDefinitionProvider
methods tested- custom
IFeatureDefinitionProvider
is 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
CustomSettingsProvider
has also been added that provides a small example of a custom feature definition provider.Relevant issues:
#57
#46
#8