Support both .NET and microsoft schema for feature flags #566
Conversation
|
We might want to update/remove this warning: |
| @@ -44,5 +44,10 @@ internal class FeatureManagementConstants | |||
| public const string ETag = "ETag"; | |||
| public const string FeatureFlagId = "FeatureFlagId"; | |||
| public const string FeatureFlagReference = "FeatureFlagReference"; | |||
|
|
|||
| // Dotnet schema keys | |||
There was a problem hiding this comment.
We added these fields for .NET schema and then removed the fields when we moved to Microsoft schema in the previous PR.
Now that we're adding support for .NET schema again, do we need to re-add Conditional/AlwaysOn/Status/Disabled fields?
There was a problem hiding this comment.
I believe those were all as a result of variants, so since the feature management library is only processing flags as .NET schema if they don't include variants, these fields shouldn't be necessary. @zhiyuanliang-ms can you fact check me here?
There was a problem hiding this comment.
The field AlwaysOn is still being used by .NET schema. ref doc
In our preview release, we even support On filter which was introduced by @amer's PR to support variant.
A feature flag like this:
{
"id": "AlwaysOnFeature",
"enabled": true
}will look like this in .NET schema
"AlwaysOnFeature": {
"EnabledFor": [
{"Name": "AlwaysOn"}
]
}There was a problem hiding this comment.
But .NET schema also support declaring feature flag in this way:
"AlwaysOnFeature": trueThat's how currently .NET provider does. ref
these fields shouldn't be necessary
Yes.
There was a problem hiding this comment.
Okay. So even though FM library supports "AlwaysOn" feature, we dont need to add that filter in the provider. Enabled/disabled features can be represented as {"feature1": true} or {"feature2": false}.
And FM library is not expecting Status field with .NET schema.
...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs
Outdated
Show resolved
Hide resolved
...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs
Outdated
Show resolved
Hide resolved
…ttps://github.com/Azure/AppConfiguration-DotnetProvider into ajusupovic/support-featuremanagement-dotnet-schema
| @@ -26,13 +27,107 @@ public FeatureManagementKeyValueAdapter(FeatureFilterTracing featureFilterTracin | |||
|
|
|||
| public Task<IEnumerable<KeyValuePair<string, string>>> ProcessKeyValue(ConfigurationSetting setting, Uri endpoint, Logger logger, CancellationToken cancellationToken) | |||
| { | |||
| _isMicrosoftSchema = false; | |||
There was a problem hiding this comment.
I'm expecting this to be on a per flag basis.
There was a problem hiding this comment.
Otherwise adding an unrelated flag that has variants will cause old flags that are working to break.
There was a problem hiding this comment.
ProcessKeyValue is invoked once per flag. So each flag will either use .NET schema or MS schema. Am I missing something?
There was a problem hiding this comment.
You're right. The code would function correctly. I was thrown off because instance state is being used to flow a condition that's only relevant to the method invocation. To avoid that, the decision of which schema to use should be done by the one who calls parse feature flag and does not need to be saved as instance state.
| private List<KeyValuePair<string, string>> ProcessDotnetSchemaFeatureFlag(FeatureFlag featureFlag, ConfigurationSetting setting, Uri endpoint) | ||
| { | ||
| var keyValues = new List<KeyValuePair<string, string>>(); | ||
|
|
There was a problem hiding this comment.
ProcessMicrosoftSchemaFeatureFlag does
if (string.IsNullOrEmpty(featureFlag.Id))
{
return keyValues;
}
I'd suggest we unify the approach.
| @@ -30,9 +30,104 @@ public FeatureManagementKeyValueAdapter(FeatureFilterTracing featureFilterTracin | |||
|
|
|||
| var keyValues = new List<KeyValuePair<string, string>>(); | |||
|
|
|||
| // Check if we need to process the feature flag using the microsoft schema | |||
| if (featureFlag.Variants != null || featureFlag.Allocation != null || featureFlag.Telemetry != null) | |||
There was a problem hiding this comment.
I'd like to treat null and empty the same. Similarly to how client_filters are treated.
amerjusupovic
deleted the
ajusupovic/support-featuremanagement-dotnet-schema
branch
More information