Skip to content
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

Should .NET provider detect duplicated feature flags? #542

Closed
zhiyuanliang-ms opened this issue Apr 10, 2024 · 1 comment
Closed

Should .NET provider detect duplicated feature flags? #542

zhiyuanliang-ms opened this issue Apr 10, 2024 · 1 comment

Comments

@zhiyuanliang-ms
Copy link
Contributor

zhiyuanliang-ms commented Apr 10, 2024

Currently, our portal allows users to declare two feature flags with the same name. Note that the keys of two feature flags are different.

image

The provider will use the id property from the json value of the feature flag key-value as the path where to put the feature flag to the configuration.
https://github.com/Azure/AppConfiguration-DotnetProvider/blob/main/src/Microsoft.Extensions.Configuration.AzureAppConfiguration/FeatureManagement/FeatureManagementKeyValueAdapter.cs#L44

This will cause duplicated properties in the feature management configuration. These feature flags are different on server side, but on the client side, only the first one will be used by the feature management.

Now we have display_name and id in MicrosoftFeatureFlagSchema v2: https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/appconfiguration/Azure.Data.AppConfiguration/src/Models/FeatureFlagConfigurationSetting.cs#L36

The portal doesn't set the display_name for feature flag. In my mind, the id should match the key. And the display_name should be the name displayed on the portal. Portal should not allow ids of feature flags to be duplicated. If portal works as this, then our .NET provider would be fine.

I found that portal has updated the json schema:
image So I guess this could be considered as a bug of portal.

I didn't find display_name here But I think it makes sense since we will only use id. Or should it also be put in the configuration?

@zhenlan
Copy link
Contributor

zhenlan commented Apr 11, 2024

First, let me explain why it is the way it is. Then we can discuss options.

The design of the key is to allow users to add namespaces for feature flags. For example, you may have two different apps and they both use a feature flag that happens to have the same name.

Name Key
Beta .appconfig.featureflag/App1/Beta
Beta .appconfig.featureflag/App2/Beta

Then in the code of your app, you can load feature flags that are relevant to your app.

.UseFeatureFlags(featureFlagOptions =>
           {
               featureFlagOptions.Select("App1/*");
           });

What you observed is expected. Our .NET FM library always takes the first one that matches the feature flag id.

As far as the naming goes, if we want the consistency, we can rename "Name" to "Id" in the Feature Manager portal. We also need to update the create/edit view and rename "Feature flag name" to "Feature flag Id". This is open for discussion.

Regarding the "display_name", it's an optional field and we don't use it. We could have dropped it if we started over, but it's already part of the SDK, so it's harder to drop it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants