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.

Sign up for GitHub

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

[Host] Allow disabling reloadOnChange for Host's CreateDefaultBuilder #2940

Merged
6 commits merged into from
Mar 19, 2020
Merged

[Host] Allow disabling reloadOnChange for Host's CreateDefaultBuilder #2940

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1b2d0b6
Add environment var to allow disabling live reload in default builder
kjerk Feb 5, 2020
126381c
Switch to using hostingContext, rename flag to fit nomenclature.
kjerk Feb 7, 2020
1a9b615
Change flag to memory data source. Change content root default instea…
kjerk Feb 11, 2020
de16b88
Update config key to be hierarchical. Change await on positive case t…
kjerk Mar 3, 2020
51c3a03
Uncomment a development test case.
kjerk Mar 4, 2020
f0d48a3
Apply suggestions from code review
Tratcher Mar 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/Hosting/Hosting/src/Host.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,11 @@ public static IHostBuilder CreateDefaultBuilder(string[] args)
builder.ConfigureAppConfiguration((hostingContext, config) =>
{
var env = hostingContext.HostingEnvironment;

var reloadOnChange = hostingContext.Configuration.GetValue("HOSTBUILDER_CONFIG_RELOAD", true);
Copy link
Member

Choose a reason for hiding this comment

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

The host uses top level keys like contentRoot.
https://docs.microsoft.com/en-us/aspnet/core/fundamentals/host/generic-host?view=aspnetcore-3.1#contentrootpath
configReload should be adequate here.

Copy link
Author

Choose a reason for hiding this comment

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

Is there some prefix I can add to that to disambiguate? I was trying to follow the {ASPNETCORE/DOTNET}_FEATURE_DESCRIPTION nomenclature of other aspnetcore and dotnet flags sort of like DOTNET_USE_POLLING_FILE_WATCHER

This would mean that DOTNET_CONFIGRELOAD specified in an environment variable would trip the behavior added in this PR, and that to me sounds like it's entirely disabling config reloading, which is not quite the case, as one's own calls to AddJsonFile are left alone.

Copy link
Member

Choose a reason for hiding this comment

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

We can at least use the hierarchical config pattern.
Also, config keys are not case sensitive. Changed for readability.

Suggested change
var reloadOnChange = hostingContext.Configuration.GetValue("HOSTBUILDER_CONFIG_RELOAD", true);
var reloadFlagConfig = new Dictionary<string, string>() {{ "hostbuilder:configreload", "false" }};

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, this has been updated in my most recent commit de16b88


config.AddJsonFile("appsettings.json", optional: true, reloadOnChange: true)
.AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true, reloadOnChange: true);
config.AddJsonFile("appsettings.json", optional: true, reloadOnChange: reloadOnChange)
.AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true, reloadOnChange: reloadOnChange);

if (env.IsDevelopment() && !string.IsNullOrEmpty(env.ApplicationName))
{
Expand Down
51 changes: 51 additions & 0 deletions src/Hosting/Hosting/test/HostTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Diagnostics.Tracing;
using System.IO;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -81,6 +82,56 @@ public void CreateDefaultBuilder_EnablesValidateOnBuild()
Assert.Throws<AggregateException>(() => hostBuilder.Build());
}

[Fact]
public void CreateDefaultBuilder_ConfigJsonDoesNotReload()
Tratcher marked this conversation as resolved.
Show resolved Hide resolved
{
Environment.SetEnvironmentVariable("DOTNET_HOSTBUILDER_CONFIG_RELOAD", "false");
Tratcher marked this conversation as resolved.
Show resolved Hide resolved
Environment.CurrentDirectory = Path.GetTempPath();
Tratcher marked this conversation as resolved.
Show resolved Hide resolved

string SaveRandomConfig()
{
var newMessage = $"Hello ASP.NET Core: {Guid.NewGuid():N}";
File.WriteAllText(Path.Combine(Path.GetTempPath(), "appsettings.json"), $"{{ \"Hello\": \"{newMessage}\" }}");
return newMessage;
}

var dynamicConfigMessage1 = SaveRandomConfig();
var host = Host.CreateDefaultBuilder().Build();
var config = host.Services.GetRequiredService<IConfiguration>();

Assert.Equal(dynamicConfigMessage1, config["Hello"]);

var dynamicConfigMessage2 = SaveRandomConfig();
Task.Delay(1000).Wait(); // Give reload time to fire if it's going to.
Tratcher marked this conversation as resolved.
Show resolved Hide resolved
Assert.NotEqual(dynamicConfigMessage1, dynamicConfigMessage2); // Messages are different.
Assert.Equal(dynamicConfigMessage1, config["Hello"]); // Config did not reload
}

[Fact]
public void CreateDefaultBuilder_ConfigJsonDoesReload()
{
Environment.SetEnvironmentVariable("DOTNET_HOSTBUILDER_CONFIG_RELOAD", "true");
Environment.CurrentDirectory = Path.GetTempPath();

string SaveRandomConfig()
{
var newMessage = $"Hello ASP.NET Core: {Guid.NewGuid():N}";
File.WriteAllText(Path.Combine(Path.GetTempPath(), "appsettings.json"), $"{{ \"Hello\": \"{newMessage}\" }}");
return newMessage;
}

var dynamicConfigMessage1 = SaveRandomConfig();
var host = Host.CreateDefaultBuilder().Build();
var config = host.Services.GetRequiredService<IConfiguration>();

Assert.Equal(dynamicConfigMessage1, config["Hello"]);

var dynamicConfigMessage2 = SaveRandomConfig();
Task.Delay(1000).Wait(); // Give reload time to fire if it's going to.
Assert.NotEqual(dynamicConfigMessage1, dynamicConfigMessage2); // Messages are different.
Assert.Equal(dynamicConfigMessage2, config["Hello"]); // Config DID reload.
}

internal class ServiceA { }

internal class ServiceB
Expand Down