Skip to content
This repository was archived by the owner on Nov 21, 2018. It is now read-only.

Add Microsoft.AspNet.Configuration targeting default configuration schema #117

Closed
HaoK opened this issue May 17, 2017 · 7 comments
Closed

Comments

@HaoK
Copy link
Member

HaoK commented May 17, 2017

• Create a new Microsoft.AspNetCore.Configuration that will contain all of our default config schema/setup which lives only in the meta package.
• It will provide a single services.ConfigureAspNetDefaults() [naming TBD] which will add all of the default config conventions, none of the existing AddXyz() methods will use IConfiguration (and we should potentially remove any overloads that exist today)
• Implementation detail: a ConfigureAspNet that implements IConfigureOptions for all things we want to configure by default
• For preview 2, this will consist of whatever is being configured in the templates today (Authentication, IdentityService, Kestrel)
• Longer term: Have intellense/schema for the important config settings.
• The structure of the config will be nested: example using today’s indidual Auth template

  "Microsoft": {
    "Logging": {
      "IncludeScopes": false,
      "Debug": {
        "LogLevel": {
          "Default": "Warning"
        }
      },
      "Console": {
        "LogLevel": {
          "Default": "Warning"
        }
      }
    },
    "AspNetCore": {
      "Authentication": {
        "IdentityService": {
          "ClientId": "C06ACE17-3212-454B-84AB-14DDB2FC58E0",
          "TokenRedirectUrn": "urn:self:aspnet:identity:integrated"
        }
      },
      "Hosting": {
        "Kestrel": { // https://github.com/aspnet/MetaPackages/blob/dev/src/Microsoft.AspNetCore/KestrelServerOptionsSetup.cs
          "Endpoints": {
            "Localhost": {
              "Address": "127.0.0.1",
              "Port": "5000"
            },
            "LocalhostWithHttps": {
              "Address": "127.0.0.1",
              "Port": "44333",
              "Certificate": {
                "Source": "Store",
                "StoreLocation": "LocalMachine",
                "StoreName": "My",
                "Subject": "CN=localhost"
              }
            }
          }
        }
@cwe1ss
Copy link

cwe1ss commented May 18, 2017

Does it really have to be that nested? This looks very XMLy 😄

The "Microsoft" and "AspNetCore" containers seem to be unnecessary because they probably won't have properties on their own. Would be nicer if we could just use "Logging", "Authentication", "Hosting" (or maybe even "Kestrel") as top-level entries. People will be smart enough to use different titles for their own areas.

It would also make it easier/more readable for flat-item providers -
Microsoft:AspNetCore:Hosting:Kestrel:Endpoints:LocalhostWithHttps:Certificate:StoreLocation is a pretty long key.

@HaoK
Copy link
Member Author

HaoK commented May 19, 2017

After discussing with @davidfowl and @danroth27 updated implementation plan:

  • Introduce a new Options.Intrastructure.ConfigureDefaultOptions<TOptions>
  • ConfigureAspNetDefaults() will add an open generic IConfigureOptions<TOptions> that consumes
    ConfigureDefaultOptions
  • Features that want to participate in the default configuration setup will just always add a ConfigureDefaultOptions<XyzOptions> as part of AddXyz similar to today.

cc @Eilon @DamianEdwards @ajcvickers @divega @glennc

@HaoK
Copy link
Member Author

HaoK commented May 26, 2017

4b18cf5 has added ConfigureAspNetCoreDefaults() which is now called by WebHost.CreateDefaultBuilder. Auth has already switched over to registering ConfigureDefaultOptions by default.

Templates and Kestrel still need to be updated to use the new config schema shown in dotnet/templating#862

@poke
Copy link
Contributor

poke commented Jul 5, 2018

@HaoK Hey, just curious, why has this feature been removed a month later after it was introduced? I’ve been jumping around and looking at various issues in other repos but I couldn’t find something that explained the decision to roll back. – Thanks!

@HaoK
Copy link
Member Author

HaoK commented Jul 5, 2018

We decided not to introduce any default config binding like this across the board since its already possible to easily bind a config section to an options, something like this is a pattern which works across already:

   AddXyz(o => config.GetSection("something:xyz").Bind(o))

@poke
Copy link
Contributor

poke commented Jul 5, 2018

Ah, yeah, I have been wondering about that since this didn’t actually add functionality that wasn’t already available in a slightly different form before. But this being organized on a large-scale and then unshipped a month later made me curious. Thanks for the clarification! :)

@HaoK
Copy link
Member Author

HaoK commented Jul 5, 2018

Yeah the feature that was driving all that config coordination was cut, which let us roll this back

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

No branches or pull requests

3 participants