Azure / azure-functions-core-tools

Command line tools for Azure Functions
MIT License
1.31k stars 433 forks source link

Consider adding support for hierarchical configuration in local.settings.json #2263

Open pakrym opened 3 years ago

pakrym commented 3 years ago

We are planning to use a hierarchical configuration to support different modes of connecting to the service by having.

This is the current shape of settings:

{
    "Values": {
      "StorageConnection": "UseDevelopmentStorage=true",
    }
}

This is what we want to support for AAD auth.

{
    "Values": {
        "StorageConnection": {
            "endpoint": "https://pakrym0test0storage.blob.core.windows.net/",
        }
    }
}

Considering that functions runtime uses IConfiguration that already supports hierarchical data but we need to make sure values gets there in the correct shape. AppSettingsFile would need to change to support this.

Setting reading would need to change https://github.com/Azure/azure-functions-core-tools/blob/28be3f19626161c5cca94d2d4376a2a3350c923b/src/Azure.Functions.Cli/Common/AppSettingsFile.cs#L11

Currently, it's possible to work around the absence of the feature by using the ASP.NET Core double underscore syntax (https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?view=aspnetcore-3.1#environment-variables)

{
    "Values": {
        "StorageConnection__endpoint": "https://pakrym0test0storage.blob.core.windows.net/"
    }
}

cc @fabiocav @mattchenderson @paulbatum

apawast commented 3 years ago

@gzuber can you please take a look at this?

jeffhollan commented 3 years ago

@anthonychu brought up some really good points here. Specifically today people map local.settings.json values 1:1 with app settings. If we make that change, that mapping no longer works. When I got to cloud, which value do I put in app settings (key/value)? We also have some experiences today where we pull or push to app settings. Same problem, in that that would break. @pakrym @paulbatum @fabiocav given the above concerns, is this something we can live with not doing so that app settings key / value remains 1:1 with local settings?

paulbatum commented 3 years ago

So the correct mapping would be to convert the hierarchy into the double-underscore format. So in the example above, the appsetting key would be StorageConnection__endpoint. Are you saying that we should make sure that Visual Studio is onboard for doing that mapping work before we add the ability to support the hierarchy format to core tools?

gzuber commented 3 years ago

I just wanted to chime in here and mention that User Secrets supports hierarchical settings by default and could be an alternative.

Was going to mention that here but was waiting until the feature is fully implemented (working on https://github.com/Azure/azure-functions-core-tools/pull/2260 and a Docs PR)

jeffhollan commented 3 years ago

Candidly I don't know the details of what is required from Azure SDK team and the ask - I'll lean on @apawast and @anthonychu to lock on next steps - but the main thing I was worried about is if some user follows some wizard or tool and ends up with a local.settings.json value that is hierarchal - I worry they will be confused after publishing and reading our docs and being like "how do I turn local.settings.json into these app settings in the consumption plan?" Or they run func azure functionapp fetch-app-settings and the local.settings.json file now no longer works or looks entirely different because it isn't heirarchal. If neither of these will be impacted or planned to be impacted it may be fine, but wanted to make sure we'd considered the potential inconsistencies between local and cloud config.

anthonychu commented 3 years ago

Yep. Would love to learn more about what the requirements are here.

Jeff outlined most of my concerns. I have an additional concern in that neither hierarchical nor the double underscore notation are natural to non .NET developers. But if I had to choose, I'd rather have double underscore and no hierarchy because we can document it one way for local.settings.json, app settings Azure, and environment variables in containers.

evilpilaf commented 3 years ago

I get that the __ notation is not common for .net developers but it is the recommended way in the documentation for hierarchical environment variables for netcore projects, it's very frustrating to have it not work at all when trying to port an existing application to a function and then needing to unravel all your appsettings.Development.json, being unable to map my local settings to a strongly typed POCO for consumption within my project is very frustrating as well

evilpilaf commented 3 years ago

It's specially difficult given the lack of documentation around this subject