Adds hosting model as an extra deployment parameter #1261

jkotalik · 2017-11-09T21:51:40Z

dnfclas · 2017-11-09T21:51:44Z

@jkotalik,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

JunTaoLuo · 2017-11-09T22:24:32Z

src/Microsoft.AspNetCore.Server.IntegrationTesting/Common/DeploymentParameters.cs

@@ -106,6 +106,8 @@ public class DeploymentParameters

        public string PublishedApplicationRootPath { get; set; }

+        public HostingModel HostingModel { get; set; }


Set a default value?

Other values don't have default values (including enums). See: https://github.com/aspnet/Hosting/pull/1261/files#diff-8478a3aebbec4604e8f2cf79bb152195R105

By default, it would take the first value as the default (or whatever is the enum equivalent of 0). Do we want to default all tests to InProcess?

Probably should be out of process.

I'm okay with changing the order of the enums but personally I prefer the more explicit approach where we just set the default value. Takes out all ambiguity.

JunTaoLuo · 2017-11-09T22:27:09Z

src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/IISExpressDeployer.cs

@@ -148,6 +148,9 @@ private async Task<(Uri url, CancellationToken hostExitToken)> StartIISExpressAs
                        {
                            Logger.LogTrace($"Config File Content:{Environment.NewLine}===START CONFIG==={Environment.NewLine}{{configContent}}{Environment.NewLine}===END CONFIG===", serverConfig);
                        }
+                        var hostingModel = DeploymentParameters.HostingModel.ToString();
+                        serverConfig.Replace("[HostingModel]", hostingModel);
+                        Logger.LogDebug("Writing HostingModel '{hostingModel} to ", hostingModel);


mismatched single quote and missing "config" after "to"

JunTaoLuo · 2017-11-09T22:28:22Z

src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/IISExpressDeployer.cs

@@ -148,6 +148,9 @@ private async Task<(Uri url, CancellationToken hostExitToken)> StartIISExpressAs
                        {
                            Logger.LogTrace($"Config File Content:{Environment.NewLine}===START CONFIG==={Environment.NewLine}{{configContent}}{Environment.NewLine}===END CONFIG===", serverConfig);
                        }
+                        var hostingModel = DeploymentParameters.HostingModel.ToString();
+                        serverConfig.Replace("[HostingModel]", hostingModel);


Why not just chain it here:

Hosting/src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/IISExpressDeployer.cs

Lines 138 to 141 in 05fd382

serverConfig =

serverConfig

.Replace("[ApplicationPhysicalPath]", contentRoot)

.Replace("[PORT]", port.ToString());

The contentRoot and port are inputs to StartIISExpressAsync. HostingModel isn't guaranteed to be set. I will add a check to see if HostingModel exists before setting it though.

JunTaoLuo · 2017-11-09T23:07:26Z

korebuild-lock.txt

@@ -1,2 +1,2 @@
-version:2.1.0-preview1-15549
-commithash:f570e08585fec510dd60cd4bfe8795388b757a95
+version:2.1.0-preview1-15564


@natemcmaster are these changes expected every now and then?

sure, if you need a new version of build tools. The only reason to hold off for now is that the jump from 2.1.0-preview1-15549 to 2.1.0-preview1-15564 puts you ahead of aspnet/Universe

cref https://github.com/aspnet/Universe/blob/dev/korebuild-lock.txt

JunTaoLuo · 2017-11-10T00:12:56Z

src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/IISExpressDeployer.cs

@@ -149,6 +149,13 @@ private async Task<(Uri url, CancellationToken hostExitToken)> StartIISExpressAs
                            Logger.LogTrace($"Config File Content:{Environment.NewLine}===START CONFIG==={Environment.NewLine}{{configContent}}{Environment.NewLine}===END CONFIG===", serverConfig);
                        }

+                        if (serverConfig.Contains("[HostingModel]"))


Not needed as string.Replace will no-op if the old value is not found. Also we should not change the config after the changes were logged in https://github.com/aspnet/Hosting/pull/1261/files#diff-727b9f7850879229f0c886f7448a0b04R149. I still think it's best to include these updates at https://github.com/aspnet/Hosting/pull/1261/files#diff-727b9f7850879229f0c886f7448a0b04R138

This check is for logging. It doesn't make sense to say we are replacing the hostingModel if the hosting model isn't replaced. I do agree with moving it above the config file content though.

These are debug logs so there's no harm in saying we tried replacing the hosting model but didn't end up finding anything to replace. Then again, I don't feel too strongly about it either way.

JunTaoLuo

Before checking this in, can you try adding a test that tests the in proc hosting model. In server tests maybe?

jkotalik · 2017-11-10T02:49:44Z

The tests in IISIntegration are effectively the same as what are in ServerTests. However those tests are currently disabled until ANCM is in universe. If IIS integration relies on a new change to ANCM, there is no way to pick up that change. Eventually, ServerTests will have tests for InProcess, however it is too painful.

JunTaoLuo

Our travis builds seem to be unreliable lately. Random errors all the time. Not related to this change though.

jkotalik added 2 commits November 9, 2017 13:47

adds hosting model to deployment parameters

d108d69

fixcomment

2a7433d

jkotalik requested review from Tratcher and JunTaoLuo November 9, 2017 21:51

dnfclas added the cla-already-signed label Nov 9, 2017

jkotalik added 2 commits November 9, 2017 14:05

hostingModel is case sensitive

7a075d6

logging

8f38fd0

JunTaoLuo reviewed Nov 9, 2017

View reviewed changes

jkotalik added 2 commits November 9, 2017 15:09

Feedback

44306e3

ordering on enum

f0996f1

JunTaoLuo reviewed Nov 10, 2017

View reviewed changes

ordering change

009b200

JunTaoLuo reviewed Nov 10, 2017

View reviewed changes

Tratcher approved these changes Nov 10, 2017

View reviewed changes

JunTaoLuo approved these changes Nov 10, 2017

View reviewed changes

jkotalik merged commit 40bd6e5 into dev Nov 10, 2017

jkotalik deleted the jkotalik/hostingModel branch November 10, 2017 18:07

jkotalik mentioned this pull request Nov 10, 2017

Allow IISExpress deployer to deploy either inprocess or outofprocess based on a deployment parameter #1260

Closed

guardrex mentioned this pull request Mar 14, 2018

ANCM: IISExpress and switching hostingModel dotnet/AspNetCore.Docs#5599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds hosting model as an extra deployment parameter #1261

Adds hosting model as an extra deployment parameter #1261

jkotalik commented Nov 9, 2017

dnfclas commented Nov 9, 2017

JunTaoLuo Nov 9, 2017

jkotalik Nov 9, 2017

JunTaoLuo Nov 9, 2017 •

edited

Loading

jkotalik Nov 9, 2017

JunTaoLuo Nov 10, 2017

JunTaoLuo Nov 9, 2017

JunTaoLuo Nov 9, 2017

jkotalik Nov 9, 2017

JunTaoLuo Nov 9, 2017

natemcmaster Nov 10, 2017

natemcmaster Nov 10, 2017

JunTaoLuo Nov 10, 2017

jkotalik Nov 10, 2017

JunTaoLuo Nov 10, 2017

JunTaoLuo left a comment

jkotalik commented Nov 10, 2017

JunTaoLuo left a comment

		@@ -106,6 +106,8 @@ public class DeploymentParameters

		public string PublishedApplicationRootPath { get; set; }

		public HostingModel HostingModel { get; set; }

	serverConfig =
	serverConfig
	.Replace("[ApplicationPhysicalPath]", contentRoot)
	.Replace("[PORT]", port.ToString());

Adds hosting model as an extra deployment parameter #1261

Adds hosting model as an extra deployment parameter #1261

Conversation

jkotalik commented Nov 9, 2017

dnfclas commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunTaoLuo left a comment

Choose a reason for hiding this comment

jkotalik commented Nov 10, 2017

JunTaoLuo left a comment

Choose a reason for hiding this comment

JunTaoLuo Nov 9, 2017 •

edited

Loading