Skip to content

Allow suppressing the use of environment variables #25136

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

Merged
merged 3 commits into from
Aug 25, 2020

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Aug 21, 2020

#20328 The GenericWebHost and WebHostBuilder automatically add a config provider for "ASPNETCORE_" environment variables with no opportunity to disable it. This is problematic for:

  • Test scenarios
  • Running two hosts in one process
  • Running in a child process where environment variables are inherited by default

This change adds an option to suppress environment based config.

public static IHostBuilder ConfigureWebHost(this IHostBuilder builder, Action<IWebHostBuilder> configure, Action<WebHostBuilderOptions> configureWebHostBuilder);

public WebHostBuilder(WebHostBuilderOptions options);

    public class WebHostBuilderOptions
    {
        /// <summary>
        /// Indicates if "ASPNETCORE_" prefixed environment variables should be added to configuration.
        /// The default is true.
        /// </summary>
        public bool UseEnvironmentConfiguration { get; set; } = true;
    }
// We can leave WebHostBuilder alone
- public WebHostBuilder(WebHostBuilderOptions options);

-  public bool UseEnvironmentConfiguration { get; set; } = true;
+  public bool SuppressUseEnvironmentConfiguration { get; set; }

@Tratcher Tratcher added area-hosting api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 21, 2020
@Tratcher Tratcher added this to the 5.0.0-rc1 milestone Aug 21, 2020
@Tratcher Tratcher self-assigned this Aug 21, 2020
@davidfowl
Copy link
Member

Why is this on the HostBuilder?

@Tratcher
Copy link
Member Author

Why is this on the HostBuilder?

Because it has to be called before ConfigureWebHost/Defaults in order to affect their behavior.

@Tratcher Tratcher closed this Aug 21, 2020
@Tratcher Tratcher reopened this Aug 21, 2020
@davidfowl
Copy link
Member

This needs an API review. I'm not a fan of having this be an extension method on the IHostBuilder that affects the webshost builder scenario. We might need another way to do this (like passing a bool to ConfigureWebHost).

@Tratcher Tratcher added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 24, 2020
@ghost
Copy link

ghost commented Aug 24, 2020

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@Tratcher Tratcher requested a review from JunTaoLuo August 24, 2020 17:24
@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 24, 2020
@ghost
Copy link

ghost commented Aug 24, 2020

Hello @Tratcher!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@Tratcher Tratcher merged commit e409c97 into dotnet:release/5.0 Aug 25, 2020
@Tratcher Tratcher deleted the tratcher/noenv branch August 25, 2020 06:28
@amcasey amcasey added the area-hosting Includes Hosting label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-hosting Includes Hosting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants