Azure / AppConfiguration-DotnetProvider

The .NET Standard configuration provider for Azure App Configuration
https://github.com/Azure/AppConfiguration
MIT License
84 stars 37 forks source link

Make implementation classes in Azure App Configuration public so they can be type checked #456

Closed torarnegustad closed 1 week ago

torarnegustad commented 1 year ago

Hi.

We have recently started looking at the Azure App Configuration provider, and due to some particular requirements we need to process and identify the list of providers from the configuration root.

Most of the other provider implementations are public and can be easily identified, but the AzureAppConfigurationProvider is not.

Could this class (and possibly other exposed classes in the Azure App Configuration provider) be made public instead of internal?

avanigupta commented 1 year ago

Hi @torarnegustad, AzureAppConfigurationProvider implements a public interface IConfigurationRefresher. You could check if your configuration provider implements IConfigurationRefresher to identify AzureAppConfigurationProvider. Will this work for your use case?

torarnegustad commented 1 year ago

Hi,

thanks for your answer. We have actually implemented it by checking for the IConfigurationRefresher interface already, and although this works it feels a bit unsafe regarding future changes to the Azure App Configuration provider.

For now we will continue using the interface, but if there is no particular reason the class is internal I still think you should consider making it public.

GuilhermeDickie commented 7 months ago

+1;

The AzureAppConfigurationSource class being internal also makes specifying the exact order of the IConfigurationBuilder.Sources much harder.

zhiyuanliang-ms commented 3 months ago

Hi, @GuilhermeDickie

The order of IConfigurationBuilder.Sources is just the order how you added them.

image

Could you provide more details or code snippet about your scenario where you want AzureAppConfigurationSource to be public?

GuilhermeDickie commented 3 months ago

Hi, @zhiyuanliang-ms,

Thank you very much for your response.

Our immediate use case is to use AzureAppConfiguration only in Local-Development environments -- to leverage its fantastic integration with Azure Entra so that Developers can use their own Account to get the secrets/configs to run the repos locally -- but not in the production pipeline. In our particular setup, we also want developers to be able to overwrite configurations with appsettings.Development.json, but not (appsettings.) so they can repoint configs to local microservices, databases etc.

The main issue is we're finding is that to specify where in the List of ConfigurationSources we want to add, the path seems to be to declare the whole list from scratch (like in your example) so that we can call the extension methods in the expected order.

The way I understand it, this will force us to reimplement/stop using WebApplication.CreateBuilder() defaults (i.e. care about what the list of configurationSources looked like or how it's being built) in the first place when adding AzureAppConfig -- which we would much rather avoid, since we're dealing with many different apps.

Example:

public class Program
{
    public static void Main(string[] args)
    {
        WebApplicationBuilder builder = WebApplication.CreateBuilder(args);

        if (builder.Environment.IsDevelopment())
        {
            // Add Azure AppConfiguration ConfigurationSource ABOVE appsettings.json, but BELOW 
            // appsettings.{environment}.json so that local settings and secrets can still override
            //  the secrets and settings coming from AzureAppConfig.

            builder.Configuration.AddAzureAppConfigurationSourceBelowEnvironmentAppSettings(...);
        }
    }

In essence, the classes being internal (with no specific interface) forbids us from ever type checking or inspecting the properties.

// Find the appsettings.{environment}.json JsonConfigurationSource.
IConfigurationSource? environmentJsonConfigurationSource = configurationSources
   .FirstOrDefault(configSource => configSource is JsonConfigurationSource jsonConfigSource 
       && string.Compare(jsonConfigSource.Path, 
           $"appsettings.{webHostEnvironment.EnvironmentName}.json",
           StringComparison.InvariantCultureIgnoreCase) == 0);

This might not be ideal if we could avoid it, but it does allows us to port many of our legacy apps to more modern practices much more smoothly and quickly.

Instead we have to assume AzureAppConfigurationSource is the last item (so it has to be captured right after we just add it), or we find the index by its Type.Name as string, And we are unable to try to cast it to something so that we can check the "Endpoint" property, for example. These limitations all feel awkward and arbitrary, and make our dependent code more brittle and complex than it has to be.

(edit: shorter lines in code blocks)

zhiyuanliang-ms commented 3 months ago

Hi, @GuilhermeDickie @torarnegustad If we provide an extension method IsAzureAppConfigurationSource on IConfigurationSource, would it be enough for your use case?

The reason we don't want to expose AzureAppConfigurationSource to public is that you won't need the whole class. What you need is just to be able to know the class type.

GuilhermeDickie commented 3 months ago

Yes, It would cover this specific type-checking use case.

samsadsam commented 2 weeks ago

Hey @GuilhermeDickie, I sent a PR for this and will update you soon on the status of this issue if anything changes :)