Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

Adds hosting model as an extra deployment parameter #1261

Merged
merged 7 commits into from
Nov 10, 2017
Merged

Adds hosting model as an extra deployment parameter #1261

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d108d69
adds hosting model to deployment parameters
jkotalik Nov 9, 2017
2a7433d
fixcomment
jkotalik Nov 9, 2017
7a075d6
hostingModel is case sensitive
jkotalik Nov 9, 2017
8f38fd0
logging
jkotalik Nov 9, 2017
44306e3
Feedback
jkotalik Nov 9, 2017
f0996f1
ordering on enum
jkotalik Nov 9, 2017
009b200
ordering change
jkotalik Nov 10, 2017
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
2 changes: 2 additions & 0 deletions src/Microsoft.AspNetCore.Server.IntegrationTesting/Common/DeploymentParameters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ public DeploymentParameters(

public string PublishedApplicationRootPath { get; set; }

public HostingModel HostingModel { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Set a default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@JunTaoLuo JunTaoLuo Nov 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably should be out of process.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


/// <summary>
/// Environment variables to be set before starting the host.
/// Not applicable for IIS Scenarios.
Expand Down
11 changes: 11 additions & 0 deletions src/Microsoft.AspNetCore.Server.IntegrationTesting/Common/HostingModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace Microsoft.AspNetCore.Server.IntegrationTesting
{
public enum HostingModel
{
OutOfProcess,
InProcess
}
}
7 changes: 7 additions & 0 deletions src/Microsoft.AspNetCore.Server.IntegrationTesting/Deployers/IISExpressDeployer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,13 @@ public override async Task<DeploymentResult> DeployAsync()
Logger.LogTrace($"Config File Content:{Environment.NewLine}===START CONFIG==={Environment.NewLine}{{configContent}}{Environment.NewLine}===END CONFIG===", serverConfig);
}

if (serverConfig.Contains("[HostingModel]"))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
var hostingModel = DeploymentParameters.HostingModel.ToString();
serverConfig.Replace("[HostingModel]", hostingModel);
Logger.LogDebug("Writing HostingModel '{hostingModel}' to config", hostingModel);
}

File.WriteAllText(DeploymentParameters.ServerConfigLocation, serverConfig);
}

Expand Down