Fix missing feature flag status and enabled logic #525
Conversation
| // Always on | ||
| keyValues.Add(new KeyValuePair<string, string>(featureFlagPath, true.ToString())); | ||
| // Add the AlwaysOn filter instead of setting the flag to "true" so feature management doesn't skip evaluating variants and telemetry when present | ||
| if (featureFlag.Variants != null || featureFlag.Telemetry != null) |
There was a problem hiding this comment.
Just want to mention that this condition could be removed so that we just always send AlwaysOn instead of setting the flag to true. It simplifies the logic and would better handle any future additions to the feature definition, but it would basically remove the optimization in the feature management library to skip evaluating the rest of the flag if it's set to true.
There was a problem hiding this comment.
I prefer removing the optimization for simplicity / if we add more fields in the future, we don't have to update this.
...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs
Outdated
Show resolved
Hide resolved
...ns.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs
Outdated
Show resolved
Hide resolved
| { | ||
| keyValues.Add(new KeyValuePair<string, string>($"{FeatureManagementConstants.SectionName}:{featureFlag.Id}", false.ToString())); | ||
| // Only explicitly set the flag to false if there are no variants to override the enabled state | ||
| if (featureFlag.Variants == null) |
There was a problem hiding this comment.
I think this would need the same "Variants == null" && "Telemetry == null" logic as above.
But more importantly, I think we just remove this shortcut as well. Old FM will see no Filters and treat it as false, New FM will respect the status of Disabled
There was a problem hiding this comment.
I think I agree with removing it now too
No description provided.