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

Jump to bottom

Fix log file path based on ORCHARD_APP_DATA environment variable #15364

Merged
merged 6 commits into from
Mar 8, 2024
Merged

Fix log file path based on ORCHARD_APP_DATA environment variable #15364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9dab0eb
Fix log file path based on ORCHARD_APP_DATA environment variable
ns8482e Feb 19, 2024
70c6622
Updated ShellOptionConstants to abstractions
ns8482e Feb 19, 2024
c26f5f6
Revert serilog file to keep current behavior
ns8482e Feb 19, 2024
d5e6791
Moved constant to it's own file
ns8482e Feb 23, 2024
c812c17
spacing
MikeAlhayek Feb 24, 2024
066bd85
Merge branch 'main' into ns/#15363
MikeAlhayek Feb 24, 2024
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
4 changes: 2 additions & 2 deletions src/OrchardCore.Cms.Web/NLog.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
autoReload="true"
internalLogLevel="Warn"
internalLogFile="App_Data/logs/internal-nlog.txt">
internalLogFile="${var:configDir}/logs/internal-nlog.txt">
Copy link

@snakefoot snakefoot Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Notice NLog InternalLogger is very basic, and internalLogFile="..." doesn't support parsing advanced layouts like ${var:configDir}. See also: https://github.com/NLog/NLog/wiki/Internal-Logging (Only support some basic tokens like ${basedir} or ${processdir})


<extensions>
<add assembly="NLog.Web.AspNetCore"/>
Expand All @@ -13,7 +13,7 @@
<targets>
<!-- file target -->
<target xsi:type="File" name="file"
fileName="${var:configDir}/App_Data/logs/orchard-log-${shortdate}.log"
fileName="${var:configDir}/logs/orchard-log-${shortdate}.log"
layout="${longdate}|${orchard-tenant-name}|${aspnet-traceidentifier}|${event-properties:item=EventId}|${logger}|${uppercase:${level}}|${message} ${exception:format=ToString,StackTrace}"
/>

Expand Down
8 changes: 8 additions & 0 deletions src/OrchardCore/OrchardCore.Abstractions/ShellOptionConstants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
namespace OrchardCore;

public class ShellOptionConstants
{
public const string OrchardAppData = "ORCHARD_APP_DATA";
public const string DefaultAppDataPath = "App_Data";
public const string DefaultSitesPath = "Sites";
}
5 changes: 4 additions & 1 deletion src/OrchardCore/OrchardCore.Logging.NLog/HostBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using Microsoft.Extensions.Hosting;
using NLog;
using NLog.Web;
using OrchardCore.Abstractions.Setup;

namespace OrchardCore.Logging;

Expand All @@ -16,7 +17,9 @@ public static IHostBuilder UseNLogHost(this IHostBuilder builder)
.ConfigureAppConfiguration((context, _) =>
{
var environment = context.HostingEnvironment;
LogManager.Configuration.Variables["configDir"] = environment.ContentRootPath;
var appData = System.Environment.GetEnvironmentVariable(ShellOptionConstants.OrchardAppData);
var configDir = string.IsNullOrWhiteSpace(appData) ? $"{environment.ContentRootPath}/{ShellOptionConstants.DefaultAppDataPath}" : appData;
LogManager.Configuration.Variables["configDir"] = configDir;
});
}
}
5 changes: 5 additions & 0 deletions src/OrchardCore/OrchardCore.Logging.NLog/WebHostBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
using NLog;
using NLog.Config;
using NLog.Web;
using OrchardCore.Abstractions.Setup;
using OrchardCore.Environment.Shell;
using OrchardCore.Environment.Shell.Builders.Models;

namespace OrchardCore.Logging;

Expand All @@ -19,6 +22,8 @@ public static IWebHostBuilder UseNLogWeb(this IWebHostBuilder builder)
.ConfigureAppConfiguration((context, _) =>
{
var environment = context.HostingEnvironment;
var appData = System.Environment.GetEnvironmentVariable(ShellOptionConstants.OrchardAppData);
var configDir = string.IsNullOrWhiteSpace(appData) ? $"{environment.ContentRootPath}/{ShellOptionConstants.DefaultAppDataPath}" : appData;
LogManager.Configuration.Variables["configDir"] = environment.ContentRootPath;
});
}
Expand Down
11 changes: 4 additions & 7 deletions src/OrchardCore/OrchardCore/Shell/ShellOptionsSetup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System.IO;
using Microsoft.Extensions.Hosting;
using Microsoft.Extensions.Options;
using OrchardCore.Abstractions.Setup;

namespace OrchardCore.Environment.Shell
{
Expand All @@ -9,10 +10,6 @@ namespace OrchardCore.Environment.Shell
/// </summary>
public class ShellOptionsSetup : IConfigureOptions<ShellOptions>
{
private const string OrchardAppData = "ORCHARD_APP_DATA";
private const string DefaultAppDataPath = "App_Data";
private const string DefaultSitesPath = "Sites";

private readonly IHostEnvironment _hostingEnvironment;

public ShellOptionsSetup(IHostEnvironment hostingEnvironment)
Expand All @@ -22,18 +19,18 @@ public ShellOptionsSetup(IHostEnvironment hostingEnvironment)

public void Configure(ShellOptions options)
{
var appData = System.Environment.GetEnvironmentVariable(OrchardAppData);
var appData = System.Environment.GetEnvironmentVariable(ShellOptionConstants.OrchardAppData);

if (!string.IsNullOrEmpty(appData))
{
options.ShellsApplicationDataPath = Path.Combine(_hostingEnvironment.ContentRootPath, appData);
}
else
{
options.ShellsApplicationDataPath = Path.Combine(_hostingEnvironment.ContentRootPath, DefaultAppDataPath);
options.ShellsApplicationDataPath = Path.Combine(_hostingEnvironment.ContentRootPath, ShellOptionConstants.DefaultAppDataPath);
}

options.ShellsContainerName = DefaultSitesPath;
options.ShellsContainerName = ShellOptionConstants.DefaultSitesPath;
}
}
}
4 changes: 2 additions & 2 deletions src/Templates/OrchardCore.ProjectTemplates/content/OrchardCore.Templates.Cms.Web/NLog.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
autoReload="true"
internalLogLevel="Warn"
internalLogFile="App_Data/logs/internal-nlog.txt">
internalLogFile="${var:configDir}/logs/internal-nlog.txt">
Copy link

@snakefoot snakefoot Mar 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Notice NLog InternalLogger is very basic, and internalLogFile="..." doesn't support parsing advanced layouts like ${var:configDir}. See also: https://github.com/NLog/NLog/wiki/Internal-Logging (Only support some basic tokens like ${basedir} or ${processdir})


<extensions>
<add assembly="NLog.Web.AspNetCore"/>
Expand All @@ -13,7 +13,7 @@
<targets>
<!-- file target -->
<target xsi:type="File" name="file"
fileName="${var:configDir}/App_Data/logs/orchard-log-${shortdate}.log"
fileName="${var:configDir}/logs/orchard-log-${shortdate}.log"
layout="${longdate}|${orchard-tenant-name}|${aspnet-traceidentifier}|${event-properties:item=EventId}|${logger}|${uppercase:${level}}|${message} ${exception:format=ToString,StackTrace}"
/>

Expand Down
1 change: 1 addition & 0 deletions ...lates/OrchardCore.ProjectTemplates/content/OrchardCore.Templates.Cms.Web/appsettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
{
"Name": "File",
"Args": {
// Replace App_Data with %ORCHARD_APP_DATA% to change log file location to use environment variable.
"path": "App_Data/logs/orchard-log.txt",
Copy link
Member

Choose a reason for hiding this comment

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

Should we also change App_Data with %ORCHARD_APP_DATA%? Then in the comment, we can explain what %ORCHARD_APP_DATA% does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did change - but then I reverted it - as in case of default when ORCHARD_APP_DATA is not defined, it would change the current behavior, and would create logs in ContentRoot\logs instead of ContentRoot\App_Data\logs

Copy link
Member

Choose a reason for hiding this comment

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

I think it is a very low risk breaking change that we can adapt. We should consider adding it and then document it in the 1.9 release as a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in template affects only new sites that's created with dotnet new - IMO we should not change template with %ORCHARD_APP_DATA%

"rollingInterval": "Day",
"outputTemplate": "{Timestamp:yyyy-MM-dd HH:mm:ss.ffff}|{TenantName}|{MachineName}|{RequestId}|{SourceContext}|{Level:u3}|{Message:lj}{NewLine}{Exception}",
Expand Down
Loading