Change CLI flag LogLevel to log-level#3650
Open
RubenCerna2079 wants to merge 2 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the dab start CLI flag for engine log level from --LogLevel to the convention-aligned --log-level, while keeping --LogLevel as a hidden legacy alias and updating related comments/tests across CLI, Service, and MCP components.
Changes:
- Rename CLI option from
--LogLevelto--log-level(with legacy alias retained). - Update CLI-to-Service argument forwarding to emit
--log-leveland add a deprecation warning for--LogLevel. - Update comments and unit/e2e tests to reference
--log-level, including MCP log-level precedence documentation.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Updates inline comments to reference --log-level. |
| src/Service/Program.cs | Updates service-side log-level extraction to look for --log-level. |
| src/Service.Tests/UnitTests/DynamicLogLevelProviderTests.cs | Updates test comments to reference --log-level. |
| src/Core/Telemetry/ILogLevelController.cs | Updates interface documentation to reference --log-level. |
| src/Cli/Utils.cs | Updates CLI documentation strings to reference --log-level. |
| src/Cli/Program.cs | Updates early-flag scan to recognize --log-level. |
| src/Cli/Exporter.cs | Updates StartOptions construction to include new legacy parameter. |
| src/Cli/CustomLoggerProvider.cs | Updates comments to reference --log-level behavior in MCP mode. |
| src/Cli/ConfigGenerator.cs | Adds legacy handling/deprecation warning and forwards --log-level to the service. |
| src/Cli/Commands/StartOptions.cs | Adds --log-level option and hidden legacy --LogLevel option. |
| src/Cli.Tests/EndToEndTests.cs | Updates e2e coverage to use --log-level and adds legacy cases. |
| src/Cli.Tests/CustomLoggerTests.cs | Updates test docs to reference --log-level. |
| src/Azure.DataApiBuilder.Mcp/Core/McpStdioServer.cs | Updates MCP docs/comments to reference --log-level. |
Comments suppressed due to low confidence (1)
src/Cli/Program.cs:67
- ParseEarlyFlags only recognizes the new "--log-level" flag. When users pass the legacy "--LogLevel" (which this PR still supports), MCP stdio logging behavior will be wrong because Utils.IsCliOverriding/CliLogLevel won’t be set before logger creation (leading to suppressed stderr output in MCP mode).
if (string.Equals(arg, "--mcp-stdio", StringComparison.OrdinalIgnoreCase))
{
Utils.IsMcpStdioMode = true;
}
else if (string.Equals(arg, "--log-level", StringComparison.OrdinalIgnoreCase) && i + 1 < args.Length)
{
Utils.IsCliOverriding = true;
if (Enum.TryParse<LogLevel>(args[i + 1], ignoreCase: true, out LogLevel cliLogLevel))
{
Comment on lines
+240
to
242
| // Check if --log-level was explicitly specified via CLI (case-insensitive parsing) | ||
| int logLevelIndex = Array.FindIndex(args, a => string.Equals(a, "--log-level", StringComparison.OrdinalIgnoreCase)); | ||
| bool hasCliLogLevel = logLevelIndex >= 0 && logLevelIndex + 1 < args.Length; |
Comment on lines
+833
to
+835
| [DataRow("--log-level information", DisplayName = "Case sensitivity: LogLevel Information from command line.")] | ||
| [DataRow("--LogLevel 0", DisplayName = "Case sensitivity: LogLevel legacy Information from command line.")] | ||
| [DataRow("--LogLevel information", DisplayName = "Case sensitivity: LogLevel legacy Information from command line.")] |
Comment on lines
+41
to
+45
| [Option("log-level", SetName = "loglevel", Required = false, HelpText = LOGLEVEL_HELPTEXT)] | ||
| public LogLevel? LogLevel { get; } | ||
|
|
||
| [Option("LogLevel", SetName = "LogLevel", Required = false, HelpText = LOGLEVEL_HELPTEXT, Hidden = true)] | ||
| public LogLevel? LogLevelLegacy { get; } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why make this change?
--LogLevelshould be--loglevel(and case insensitive) like all the other CLI flags #3257What is this change?
We changed the
dab start--LogLevelflag to--log-levelin order to have it follow the conventions we already have. We also allow for the CLI to still use the previous--LogLevelflag but add a warning sign to ensure the user knows it is deprecated. Lastly, also changed all of the comments that mention the previous--LogLeveland updated it to use the new--log-level.How was this tested?
Changed the tests that were for
--LogLevelso they now use--log-leveland added a few extra cases to ensure the--LogLevelflag still worksSample Request(s)
dab start --log-level information
dab start --LogLevel warning