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 an example for providing a pre-built configuration to Host #15370

Closed
UncleDave opened this issue Oct 29, 2019 · 13 comments · Fixed by #15518
Closed

Add an example for providing a pre-built configuration to Host #15370

UncleDave opened this issue Oct 29, 2019 · 13 comments · Fixed by #15518
Assignees
Labels
Pri2 Source - Docs.ms Docs Customer feedback via GitHub Issue

Comments

@UncleDave
Copy link
Contributor

UncleDave commented Oct 29, 2019

As far as I can tell there is currently no example which shows how to:

  1. Clear out the default configuration sources added by CreateDefaultBuilder.
  2. Add a pre-built IConfiguration via AddConfiguration.

As raised in this StackOverflow question.

If this is something that would be beneficial to the docs I'd be happy to open a PR.


Document Details

Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

@dotnet-bot dotnet-bot added ⌚ Not Triaged Source - Docs.ms Docs Customer feedback via GitHub Issue labels Oct 29, 2019
@guardrex
Copy link
Collaborator

Hello @UncleDave ... Thanks for the comment, but we're not leaning in that direction (i.e., showing every way to handle configuration. We are being a little bit opinionated about what's shown, especially for the goal of not overloading new devs and devs new to ASP.NET Core. Let's take those separately ...

Clear providers

Devs can just clear them. Otherwise, devs build their own host and just don't call CreateDefaultBuilder. Of course, the dev can just ignore them (i.e., if they aren't using settings files ... it just no-ops on settings files; not using command-line args ... again ... no-ops; not using Secret Manager ... no-ops; etc.). These are fairly straightforward approaches.

.ConfigureAppConfiguration((hostingContext, config) =>
{
    config.Sources.Clear();
    // New config providers here
})

The main pattern recommended by engineering is shown in the host and config topics. We'll probably let more advanced devs work on more advanced/different patterns on their own. Given the no-op behavior of the config providers with CreateDefaultBuilder, I don't see much value but do see some significant downside in an already looooong topic with a lot of content. Explain in more detail why you think it would important to add the coverage.

Pre-built config

We had something like that ... sort of like that ... for the Web Host.

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/web-host?view=aspnetcore-3.0#override-configuration

... so only the concept is similar going forward with the Generic Host. I really think ConfigureAppConfiguration is a much better pattern to adopt.

In ...

.ConfigureAppConfiguration((hostingContext, config) => { ... })

config is an IConfigurationBuilder, and that can be created anywhere and just assigned. I don't think that it's difficult to work out. I think that only a tiny fraction of devs would build config that way. The ones who do probably are proficient devs and know how to take care of it. Again, provide more detail on why/how this would help in the context of a long, detailed topic.

@guardrex guardrex self-assigned this Oct 29, 2019
@guardrex guardrex added this to the Backlog milestone Oct 29, 2019
@UncleDave
Copy link
Contributor Author

Hi @guardrex, thanks for the response.

With regards to clearing the default providers my main argument is that if the docs are explaining that CreateDefaultBuilder will add some default providers then it should also explain how to remove those if they're not desirable. I agree with you on no-ops, it's definitely trivial to leave the defaults alone and just override if needs be, but regardless I think some individuals will prefer to explicitly add sources and have full control. As for the increased length of the document your code example is actually exactly what I would have proposed, for a change of +10 lines when header and one line explanation is included.

On the general topic of document length, for what it's worth, I wouldn't consider it a valid argument for not adding more content. I agree that content must be relevant and you can't document every single use case under the sun, but if I'm visiting the docs page I won't be sitting down to read the whole thing from top to bottom - I'll be clicking on a sub-header and going to a specific section to look something up, in which case I'd much rather the docs be comprehensive than short.

I think these additions would help with what I believe is a pretty common use case: configuring something (most likely logging) before creating the host, then reusing the same config values without building another IConfiguration.

config is an IConfigurationBuilder, and that can be created anywhere and just assigned.

Could you clarify what you mean here? Are you saying an IConfigurationBuilder can be assigned to the config argument like so?

.ConfigureAppConfiguration((hostingContext, config) =>
{
    config = myConfigBuilder;
})

I'm sure I've misunderstood because I don't think that's possible.

If you'd like I can show you my proposed changes and you can decide specifically if they'd be valuable or not.

@guardrex
Copy link
Collaborator

guardrex commented Oct 29, 2019

A few general remarks first ...

On the editorial policy (for lack of a better term), it's challenging to figure out how opinionated the Fundamentals docs should be given the wide experience and knowledge of the readership.

Engineering and docs teams generally draw the line in Fundamentals topics at the primary, suggested approach that accomplishes most of the use cases ... admittedly a fuzzy line that's rather subjective.

I hear u on the not thinking length is a valid argument, but we disagree on that point in this node. We have many readers new to programming and new to .NET. Many readers consume our docs reading them in full right down the TOC until they at least get past the Fundamentals node. They read these like chapters of a book.

This node is trying to be focused and careful not to overwhelm the reader with less important details. Perhaps, we need more advanced and alternative scenario docs, and that is a work-in-progress. It would be nice to have an Advanced Configuration topic with edge cases and details on how the framework treats configuration ... the juicy engineering details. However, we just don't have the staffing to do that at this time.

But on the specifics here ...

Clearing providers coverage sounds good to me given that it will be a quick/short add with a great spot to add it. I take it that you're suggesting this for the bottom of the Default configuration section? ...

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?view=aspnetcore-3.0#default-configuration

... something along the lines of ...

A sentence explaining that the providers can be cleared/replaced ... and then something like ...

.ConfigureAppConfiguration((hostingContext, config) =>
{
    config.Sources.Clear();
    // New config providers here
})

Or, what do you propose?

Next ...

configuring something (most likely logging) before creating the host

That's not the recommended approach: The config flows into logging configuration (i.e., no need to apply a single config in two spots), which is why (for example) the settings in the app settings file work. Then, there's coverage for logging configuration in the logging topic with additional details on adding configuration for logging beyond CreateDefaultBuilder ...

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/logging/?view=aspnetcore-3.0#add-providers

... BUT ... the underlying problem is that we don't call out the flow of config providers well enough. We have the cross-links to other providers, which contain their config ...

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?view=aspnetcore-3.0#providers

... but we're weak on the "flow" concept imo. The word isn't used, and it's a good word for how it works.

misunderstood

No ... my bad. 🤦‍♂ I should've said ADD there, not "assign." ... What you called out in your opening remark.

config.AddConfiguration(builtConfig);

We should consider adding the manual host builder approach with a built configuration along the lines of what was done in the Web Host topic. However, I'd like to see that in the Generic Host topic, not here. This could cross-link back out to that section. Again, it might make a nice single-sentence cross-link from the Default configuration section.

Conclusion

(Thank the 🙏 server gods! 🙏 ... He's finally going to shut up! 😄 lol)

Float back your ideas for the PR ... not the exact content, per se ... just a few notes on what you want to do based on the discussion. If it looks good, yes, we welcome a PR. Otherwise, what I'll do is take this on in ... mmmm 🤔 ... probably December. 🏃😅

@guardrex
Copy link
Collaborator

guardrex commented Oct 29, 2019

... and I'm thinking about your remark on the SO post about logging for startup ...

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/error-handling?view=aspnetcore-3.0#startup-exception-handling

... but cross-links to the Web Host topic.

[EDIT] I should ask u tho what you mean ... did you notice a scenario given the default logging providers (debug/console/event source + event log on Windows) that some startup errors were not getting logged?

[EDIT] ..... mmm 🤔 ... I think that coverage looks pretty darn good ... https://docs.microsoft.com/en-us/aspnet/core/fundamentals/logging/?view=aspnetcore-3.0#add-providers ... I'm not feel'in at the moment (or recalling reader feedback) that the guidance there for logging config, including host builder timeframe logging, is missing.

@UncleDave
Copy link
Contributor Author

I understand what you're saying about length of the article, and I agree that maybe the 2nd suggestion does not belong in the Fundamentals node and would be happier in the advanced node you mentioned. I do have further thoughts on the subject, but maybe given that we're talking about changes to the Fundamentals node it would be more appropriate to discuss them in another issue and focus on the "source clearing" subject here as we seem to be on the same page with it.

A sentence explaining that the providers can be cleared/replaced ... and then something like ...

Yeah your snippet is pretty much verbatim what I'm suggesting, actually the only difference is my comment has slightly different wording but that's neither here nor there.

I take it that you're suggesting this for the bottom of the Default configuration section?

I was actually going to put it under ConfigureAppConfiguration but it seems like it would fit in either section, maybe more appropriate in the section you suggested but I'll defer to you on that.

My full suggestion was this:


Remove default providers

To remove the providers added by CreateDefaultBuilder, call Sources.Clear() first:

.ConfigureAppConfiguration((hostingContext, config) =>
{
    config.Sources.Clear();
    // Add providers here
})

@guardrex
Copy link
Collaborator

Excellent, yeah ... I like it. Yes, I recommend in (at the bottom) of the Default configuration section because that goes right to the point you were making: CreateDefaultBuilder adds these ... you can knock them all out with ....... and that's what the sentence is saying and the example is showing.

When you open your PR, mark your opening line of the opening comment with "Addresses" for this issue ...

Addresses #15370

... we'll get that merged and it won't automatically close this issue. I'll swing back around in Nov/Dev to work on the config provider "flow" concept and check the cross-linking.

If you also want to let me work the host builder with a built config (similar to that Web Host topic example) and cross-link that into the config topic, I can do that, too.

@guardrex
Copy link
Collaborator

guardrex commented Oct 29, 2019

... and I'll just throw this out there. It's also possible for it to go ...

.ConfigureHostConfiguration(config =>
{
    config.Sources.Clear();
    // Add providers here
})
.ConfigureAppConfiguration((hostingContext, config) =>
{
    config.Sources.Clear();
    // Add providers here
})

... but that is also fairly obvious. Since the host is only configured with DOTNET_ prefixed env vars and command-line args, it's probably not the main thing on a dev's mind. It's app config ... that's probably the main use case.

yeah ........ probably overkill to do that. 🤔

@UncleDave
Copy link
Contributor Author

Sounds good, I'll whip up that PR.

If you also want to let me work the host builder with a built config (similar to that Web Host topic example) and cross-link that into the config topic, I can do that, too.

Just to clarify, do you mean add a small section regarding using a built config to the Generic Host topic, then link to it from the Configuration topic? Or the other way around?

It's app config ... that's probably the main use case.

I agree.

@guardrex
Copy link
Collaborator

Yes ... in the Generic Host topic showing a manually built host that doesn't call CreateDefaultBuilder and applies a config ... just spit ball'in here 🤔 ... perhaps it fits well in the Host configuration section under the current content of that section. Something like .......

public static void Main(string[] args)
{
    var builtConfig = new ConfigurationBuilder()
        .SetBasePath(System.IO.Directory.GetCurrentDirectory())
        .AddJsonFile("hosting.json", optional: true)
        .AddCommandLine(args)
        .Build();

    var host = new HostBuilder();
        .ConfigureHostConfiguration(config =>
        {
            config.AddConfiguration(builtConfig);
        })
        .Build();

    host.Start();
}

... I really have a feeling that engineering is going to say why not just place that directly in the ConfigureHostConfiguration ...

public static void Main(string[] args)
{
    var host = new HostBuilder();
        .ConfigureHostConfiguration(config =>
        {
            config.SetBasePath(System.IO.Directory.GetCurrentDirectory());
            config.AddJsonFile("hosting.json", optional: true);
            config.AddCommandLine(args);
        })
        .Build();

    host.Start();
}

Seems like a solution in search of a problem! 😄 lol

@UncleDave
Copy link
Contributor Author

I think we got our wires crossed a little bit discussing this, when I said the following:

configuring something (most likely logging) before creating the host

I meant for the purpose of logging errors in Build(), but that's not really the important part. My intent there was to show a real world example of building a config before the host, then reusing the same config instead of having the host build a whole new one from the same providers.

I've created a repo that will hopefully explain better than my word salad.

https://github.com/UncleDave/DotNetCorePreBuiltConfigExample

The point is I'm reusing the same config, I can think of at least one other use case besides logging startup errors. I'm not saying the docs need an example that says "Hey if you want to catch errors from Build() use a try-catch ya dingus", just that there are legitimate examples of building the config before the host, and if I was to do that I'd want to reuse the same config.

I definitely wouldn't call this a fundamental though, so it would probably fit better in the previously mentioned advanced section, I just wanted to clear up what I'm proposing here.

@guardrex
Copy link
Collaborator

@Tratcher TL;DR ... Would you put a quick 👁️ on this startup 3rd party logging pattern (UncleDave/DotNetCorePreBuiltConfigExample GH repo) and determine if you would like the pattern covered in the docs? Too advanced/edgy or good to show?

@Tratcher
Copy link
Member

We'll probably need coverage for an advanced logging setup scenario like that. A lot of people are asking how to log in ConfigureServices, which isn't directly supported in 3.0.
dotnet/aspnetcore#9337

@guardrex
Copy link
Collaborator

Thanks ... I'll work on something to show u when I get back to this. Should be soon-ish ... I have it on the November sprint.

@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Pri2 Source - Docs.ms Docs Customer feedback via GitHub Issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants