Skip to content

Commit 643970b

Browse files
Fix CLI overrides LogLevel value from config file (#3426)
## Why make this change? - Closes issue #3258 Whenever we use the `dab start` command the CLI overrides the minimum LogLevel from the configuration file. This is acceptable only if we use the `--LogLevel` flag. If we do not use that flag, the LogLevel for each namespace should be decided by the configuration file. ## What is this change? This change moves the `args.Add` methods inside the `ConfigGenerator.cs` so that they are only applied when we use the `--LogLevel` flag. Having these arguments causes DAB to not allow any changes to the LogLevel in the loggers, since those arguments were always being added, DAB always assumed the CLI was overriding the LogLevel when it was not expected as it would use the `--LogLevel` flag to determine if it was an `IsLogLevelOverridenByCli` scenario. ## How was this tested? - [ ] Integration Tests - [x] Unit Tests - [x] Manual Tests Tested with different configurations of the namespaces in the `log-level` property inside the config file. Note: ALL these manual tests were tested by running the CLI as well as running DAB through Visual Studio. Config file includes: ``` "log-level": { "Azure.DataApiBuilder.Core.Services.ISqlMetadataProvider": "Information", "Azure.DataApiBuilder.Core": "Debug", "Azure.DataApiBuilder.Config.FileSystemRuntimeConfigLoader": "Error", "default": "Warning" } ``` Expected results: - All the logs from `Azure.DataApiBuilder.Core.Services.ISqlMetadataProvider` will provide logs from log level information and above. - All logs from `Azure.DataApiBuilder.Core` and its sub-namespaces that are not from `ISqlMetadataProvider` will provide logs from log level debug and above. - All logs from `Azure.DataApiBuilder.Config.FileSystemRuntimeConfigLoader` will provide logs from log level error and above. - Everything else will provide logs from log level warning and above, including CLI. Config file includes: ``` "log-level": { "Azure.DataApiBuilder.Core": "Information", "Azure.DataApiBuilder.Config": "Debug", } ``` Expected results: - All logs from `Azure.DataApiBuilder.Core` and its sub-namespaces will provide logs from log level information and above. - All logs from `Azure.DataApiBuilder.Config` and its sub-namespaces will provide logs from log level debug and above. - Everything else will provide logs from log level debug and above with `host.mode = development`, including CLI. Config file includes: ``` "log-level": { "Default: none } ``` Expected results: - No logs will be printed, including CLI. ## Sample Request(s) dab start --LogLevel information dab start (With config file default value debug)
1 parent 3d0ee44 commit 643970b

2 files changed

Lines changed: 80 additions & 10 deletions

File tree

src/Cli.Tests/EndToEndTests.cs

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using Azure.DataApiBuilder.Config.Converters;
55
using Azure.DataApiBuilder.Product;
6+
using Azure.DataApiBuilder.Service;
67
using Cli.Constants;
78
using Microsoft.Data.SqlClient;
89

@@ -857,6 +858,83 @@ public void TestEngineStartUpWithVerboseAndLogLevelOptions(string logLevelOption
857858
StringAssert.Contains(output, $"User provided config file: {TEST_RUNTIME_CONFIG_FILE}", StringComparison.Ordinal);
858859
}
859860

861+
/// <summary>
862+
/// Validates that `dab start` correctly sets <see cref="Startup.IsLogLevelOverriddenByCli"/>
863+
/// based on whether the --LogLevel CLI flag is provided.
864+
///
865+
/// When the --LogLevel flag is provided, IsLogLevelOverriddenByCli should be true.
866+
/// When the --LogLevel flag is omitted (log level comes from the config file), IsLogLevelOverriddenByCli should be false.
867+
/// </summary>
868+
/// <param name="cliLogLevel">The --LogLevel CLI flag value, or null to omit the flag.</param>
869+
/// <param name="expectedIsOverridden">Expected value of Startup.IsLogLevelOverriddenByCli.</param>
870+
[DataTestMethod]
871+
[DataRow(null, false, DisplayName = "IsLogLevelOverriddenByCli is false")]
872+
[DataRow(LogLevel.Error, true, DisplayName = "IsLogLevelOverriddenByCli is true")]
873+
public async Task TestStartCommandResolvesLogLevelFromConfigOrFlag(
874+
LogLevel? cliLogLevel,
875+
bool expectedIsOverridden)
876+
{
877+
string baseConfig = @"
878+
{
879+
""$schema"": """ + DAB_DRAFT_SCHEMA_TEST_PATH + @""",
880+
""data-source"": {
881+
""database-type"": ""mssql"",
882+
""connection-string"": """ + SAMPLE_TEST_CONN_STRING + @"""
883+
},
884+
""runtime"": {
885+
""rest"": {
886+
""path"": ""/api"",
887+
""enabled"": true
888+
},
889+
""graphql"": {
890+
""path"": ""/graphql"",
891+
""enabled"": true,
892+
""allow-introspection"": true
893+
},
894+
""host"": {
895+
""mode"": ""development"",
896+
""cors"": {
897+
""origins"": [],
898+
""allow-credentials"": false
899+
},
900+
""authentication"": {
901+
""provider"": ""Unauthenticated""
902+
}
903+
},
904+
""telemetry"": {
905+
""log-level"": {
906+
""Azure.DataApiBuilder.Core.Services.ISqlMetadataProvider"": ""Information"",
907+
""Azure.DataApiBuilder.Core"": ""Debug"",
908+
""Azure.DataApiBuilder.Service.Controllers.RestController"": ""Error"",
909+
""default"": ""Warning""
910+
}
911+
}
912+
},
913+
""entities"": {}
914+
}";
915+
916+
// Merge in an entity so the config is not rejected for having an empty entities section.
917+
string configWithLogLevel = AddPropertiesToJson(baseConfig, BASIC_ENTITY_WITH_ANONYMOUS_ROLE);
918+
_fileSystem!.File.WriteAllText(TEST_RUNTIME_CONFIG_FILE, configWithLogLevel);
919+
920+
StartOptions options = new(
921+
verbose: false,
922+
logLevel: cliLogLevel,
923+
isHttpsRedirectionDisabled: false,
924+
mcpStdio: false,
925+
mcpRole: null,
926+
config: TEST_RUNTIME_CONFIG_FILE);
927+
928+
// Run TryStartEngineWithOptions on a background task because StartEngine blocks until the host shuts down.
929+
Task engineTask = Task.Run(() =>
930+
TryStartEngineWithOptions(options, _runtimeConfigLoader!, _fileSystem!));
931+
932+
// Wait for the engine to finish loading the config.
933+
await Task.Delay(TimeSpan.FromSeconds(5));
934+
935+
Assert.AreEqual(expectedIsOverridden, Startup.IsLogLevelOverriddenByCli);
936+
}
937+
860938
/// <summary>
861939
/// Validates that valid usage of verbs and associated options produce exit code 0 (CliReturnCode.SUCCESS).
862940
/// Verifies that explicitly implemented verbs (add, update, init, start) and appropriately

src/Cli/ConfigGenerator.cs

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2732,18 +2732,10 @@ public static bool TryStartEngineWithOptions(StartOptions options, FileSystemRun
27322732
}
27332733

27342734
minimumLogLevel = (LogLevel)options.LogLevel;
2735+
args.Add("--LogLevel");
2736+
args.Add(minimumLogLevel.ToString());
27352737
_logger.LogInformation("Setting minimum LogLevel: {minimumLogLevel}.", minimumLogLevel);
27362738
}
2737-
else
2738-
{
2739-
minimumLogLevel = deserializedRuntimeConfig.GetConfiguredLogLevel();
2740-
HostMode hostModeType = deserializedRuntimeConfig.IsDevelopmentMode() ? HostMode.Development : HostMode.Production;
2741-
2742-
_logger.LogInformation($"Setting default minimum LogLevel: {minimumLogLevel} for {hostModeType} mode.", minimumLogLevel, hostModeType);
2743-
}
2744-
2745-
args.Add("--LogLevel");
2746-
args.Add(minimumLogLevel.ToString());
27472739

27482740
// This will add args to disable automatic redirects to https if specified by user
27492741
if (options.IsHttpsRedirectionDisabled)

0 commit comments

Comments
 (0)