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

The hostcontext did not have the environment variable information in … #10531

Merged
merged 4 commits into from
Jan 22, 2019

Conversation

mattspicer1
Copy link
Contributor

…ConfigureAppConfiguration. ConfigureHostConfiguration must be called first

I've tested this locally. The original code would always use production as the hosted environment.

…ConfigureAppConfiguration. ConfigureHostConfiguration must be called first
@guardrex
Copy link
Collaborator

Hello @mattspicer1 ... Correct! ... especially given we tossed in consumption of the environmental appsettings file.

ConfigureAppConfiguration is used but only for app config. We document ConfigureHostConfiguration over at https://docs.microsoft.com/aspnet/core/fundamentals/host/generic-host#configurehostconfiguration.

First, two little things ...

  • Change the prefix to ASPNETCORE_.
  • I think change the var from configHost to just config. That's the way that engineering has it in source.

Here's a ❓ for @Tratcher ...

  1. We could go as @mattspicer1 has done here and just call AddEnvironmentVariables.
  2. Or possibly better ... go ahead for cross-teaching/exposure and show the following for host config ...
    • Env vars
    • hosting.json
    • Command-line
  3. Possibly even better ... build out an IConfiguration.

If we go with Option 3 ...

  1. In the Program class, we'd have an IConfiguration field ...

    private readonly IConfiguration _config;
  2. Inside Program.Main, we'd build it using all of those sources ...

    _config = new ConfigurationBuilder()
        .AddEnvironmentVariables(prefix: "ASPNETCORE_")
        .AddJsonFile("hosting.json", optional: true)
        .AddCommandLine(args)
        .Build();
  3. Used in ConfigureHostConfiguration ...

    .ConfigureHostConfiguration(config =>
    {
        config.AddConfiguration(_config);
    });
  4. ... and ❓ ... command-line args don't flow? I noticed in CreateDefaultBuilder (for the Web Host today) that .AddCommandLine(args); is built for configuration on WebHostBuilder but also called again in ConfigureAppConfiguration, so I'm confused on the need for the 2nd call. If they did flow, then I guess here we can drop the .AddCommandLine(args); from ConfigureAppConfiguration.

Thing is tho that the upcoming templates won't do all of this, correct? ConfigureWebHostDefaults is only going to call ConfigureWebDefaults OOB, so host config will be left to the dev and the docs in terms of the OOB experience with templates (apparently).

Anywho ... wrt the PR here ... @Tratcher, what would u like to do?

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

I don't see why you'd do number 3, It's better to let the host build the config.

Command line args do flow, CreateDefaultBuilder adds them twice to preserve precedence over lower priority config sources.

See the new template for 3.0 preview2 https://github.com/aspnet/AspNetCore/blob/e004e5aab29dc26beaafaed49c7aa4b780b6e98f/src/Templating/src/Microsoft.DotNet.Web.ProjectTemplates/content/WebApi-CSharp/Program.cs#L21-L27
Host.CreateDefaultBuilder does most of the same magic for you.

https://github.com/aspnet/Extensions/blob/954eb19b814847ee91ba9632d69b1e35e026191c/src/Hosting/Hosting/src/Host.cs

@mattspicer1
Copy link
Contributor Author

I'm currently running in Cloud Foundry using environment variables, so this example is good for that situation. I'll change the prefix to ASPNETCORE

@guardrex
Copy link
Collaborator

@mattspicer1 ... Just drop the prefix totally and change configHost to config.

@guardrex
Copy link
Collaborator

guardrex commented Jan 22, 2019

Env var configuration should flow to the app .... soooo ... finally! lol

Strike config.AddEnvironmentVariables(); from the ConfigureAppConfiguration call.

That should wrap this puppy 🐶 up.

[EDIT] Unless @Tratcher you want another override option for env vars there ... in which case ... it would move down in the providers.

@guardrex
Copy link
Collaborator

That's fine unless Chris wants to keep it and move it down. It's a minor point tho, so we're probably good to go now.

https://github.com/aspnet/Extensions/blob/954eb19b814847ee91ba9632d69b1e35e026191c/src/Hosting/Hosting/src/Host.cs#L46

@Tratcher
Copy link
Member

This is fine. In 3.0 we'll remove this sample and replace it with a library.

@guardrex
Copy link
Collaborator

replace it with a library.

Excellent! ... I'm in favor of one LESS sample here. 😄

Thanks for the submission @mattspicer1! 🚀 🚀

@guardrex guardrex merged commit 6af24e5 into dotnet:master Jan 22, 2019
@mattspicer1 mattspicer1 deleted the EnvironmentIssue branch January 23, 2019 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants