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

Enable Kestrel config reload in ConfigureWebHostDefaults #22528

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Jun 4, 2020

The PR to enable reloadable Kestrel config (#21072) originally changed the behavior of the existing KestrelServerOptions.Configure(IConfiguration) method to support reloading config and added a new overload to opt out. We decided that was too breaking, so we switched it to opt in.

I think that changing the behavior of ConfigureWebHostDefaults is worth the break though. ConfigureWebHostDefaults already calls AddJsonFile with reloadOnChange set to true, so having Kestrel reloadOnChange is more consistent. I also find it unlikely that it's common to change Kestrel config and expect Kestrel not to react to it.

I'm targeting preview6 so we can get feedback on this change earlier.

@halter73 halter73 added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Jun 4, 2020
@halter73 halter73 added this to the 5.0.0-preview6 milestone Jun 4, 2020
@ghost ghost added the area-hosting label Jun 4, 2020
@halter73
Copy link
Member Author

halter73 commented Jun 5, 2020

@davidfowl @Tratcher Do you think we should do this?

@Tratcher
Copy link
Member

Tratcher commented Jun 5, 2020

How would I opt out? Add this in Main?

            builder.ConfigureKestrel((builderContext, options) =>
            {
                options.Configure(builderContext.Configuration.GetSection("Kestrel"), reloadOnChange: false);
            })

@halter73
Copy link
Member Author

halter73 commented Jun 5, 2020

How would I opt out? Add this in Main?

That would work since the last call to KestrelServerOptions.Configure() wins. I don't expect opting out will be too common though.

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

As long as there is a way to disable it I'm fine.

@halter73 halter73 force-pushed the halter73/reload-by-default branch from 1555cba to 453da00 Compare June 9, 2020 21:50
@halter73 halter73 requested a review from a team June 9, 2020 21:50
@halter73 halter73 changed the base branch from release/5.0-preview6 to master June 9, 2020 21:51
@halter73 halter73 merged commit 6c31da5 into master Jun 11, 2020
@halter73 halter73 deleted the halter73/reload-by-default branch June 11, 2020 00:47
@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
area-hosting Includes Hosting breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants