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

Add ConfigurationManager #55338

Merged
merged 6 commits into from
Jul 13, 2021
Merged

Add ConfigurationManager #55338

merged 6 commits into from
Jul 13, 2021

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Jul 8, 2021

This adds a type already used by ASP.NET Core's new WebApplcation and WebApplicationBuilder that allows reading from configuration (e.g. appsettings.json and DOTNET_/ASPNETCORE_ environment variables) while still being able to add new configuration sources without explicitly rebuilding config. Every time a source is added via the IConfigurationBuilder interface, the IConfiguration updates automatically and immediately.

New API

namespace Microsoft.Extensions.Configuration
{
+    public sealed class ConfigurationManager : IConfigurationRoot, IConfigurationBuilder, IDisposable
+    {
+        public ConfigurationManager();
+        public string? this[string key] { get; set; }
+        public IConfigurationSection GetSection(string key);
+        public void Dispose();
+    }

The members that are required to implement IConfigurationBuilder will be implemented explicitly, so members like IList<IConfigurationSource> IConfigurationBuilder.Sources don't pollute intellisense. Extension methods are generally used to add configuration sources.

Usage Example

WebApplicationBuilder essentially already exposes this type as a public property. The problem it is trying to solve is being able to read config from appsettings.json and DOTNET_/ASPNETCORE_ while configuring the host's IServiceCollection while at the same time being able to add new configuration sources or even change the content root without having to introduce additional build stages.

    using var config = new ConfigurationManager();

    config.AddEnvironmentVariables(prefix: "MyCustomPrefix_");

    if (config["FileConfig"] == "enabled")
    {
        config.AddJsonFile("MyConfig.json", optional: true, reloadOnChange: true);
    } 

    string myValueFromJson = config["JsonConfigValue"];
    // ...

We have docs where we demonstrate manually building an IConfigurationBuilder, reading from the built IConfiguration, and then throwing that away to add a new config source. This is an alternative to that.

Fixes #51770

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jul 8, 2021

Tagging subscribers to this area: @maryamariyan, @safern
See info in area-owners.md if you want to be subscribed.

Issue Details

This adds a type already used by ASP.NET Core's new WebApplcation and WebApplicationBuilder that allows reading from configuration (e.g. appsettings.json and DOTNET_/ASPNETCORE_ environment variables) while still being able to add new configuration sources without explicitly rebuilding config. Every time a source is added via the IConfigurationBuilder interface, the IConfiguration updates automatically and immediately.

New API

namespace Microsoft.Extensions.Configuration
{
+    public sealed class Config : IConfigurationRoot, IConfigurationBuilder, IDisposable
+    {
+        public Configuration();
+        public string? this[string key] { get; set; }
+        public IConfigurationSection GetSection(string key);
+        public void Dispose();
+    }

The members that are required to implement IConfigurationBuilder will be implemented explicitly, so members like IList<IConfigurationSource> IConfigurationBuilder.Sources don't pollute intellisense. Extension methods are generally used to add configuration sources.

Usage Example

WebApplicationBuilder essentially already exposes this type as a public property. The problem it is trying to solve is being able to read config from appsettings.json and DOTNET_/ASPNETCORE_ while configuring the host's IServiceCollection while at the same time being able to add new configuration sources or even change the content root without having to introduce additional build stages.

    using var config = new Config();

    config.AddEnvironmentVariables(prefix: "MyCustomPrefix_");

    if (config["FileConfig"] == "enabled")
    {
        config.AddJsonFile("MyConfig.json", optional: true, reloadOnChange: true);
    } 

    string myValueFromJson = config["JsonConfigValue"];
    // ...

We have docs where we demonstrate manually building an IConfigurationBuilder, reading from the built IConfiguration, and then throwing that away to add a new config source. This is an alternative to that.

Fixes #51770

Author: halter73
Assignees: -
Labels:

area-Extensions-Configuration, new-api-needs-documentation

Milestone: -

@halter73
Copy link
Member Author

@pakrym @davidfowl Does this look good to you?

@safern
Copy link
Member

safern commented Jul 12, 2021

Did we get a consensus on the name yet? As it was stated on the issue I don't think Config follows our naming guidelines?

@davidfowl
Copy link
Member

@safern We're clearing up the naming but can you review the PR?

var memVal2 = config["Mem2:KeyInMem2"];
var memVal3 = config["MEM3:KEYINMEM3"];

// Assert
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This comment doesn't seem that useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally agree about the uselessness of // Arrange // Act // Assert comments, but this was copied from ConfigurationTest. If we change it, I'd rather do it in one pass.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I don't mind if we do that cleanup in a separate PR.

Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@halter73 halter73 changed the title Add Config : IConfigurationRoot, IConfigurationBuilder Add ConfigurationManager : IConfigurationRoot, IConfigurationBuilder Jul 13, 2021
@halter73 halter73 changed the title Add ConfigurationManager : IConfigurationRoot, IConfigurationBuilder Add ConfigurationManager Jul 13, 2021
@halter73 halter73 merged commit a490a34 into main Jul 13, 2021
@halter73 halter73 deleted the halter73/51770 branch July 13, 2021 17:26
@ghost ghost locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a type implementing both IConfiguration and IConfigurationBuilder
4 participants