Change CLI flag --LogLevel to --log-level#3653
Open
RubenCerna2079 wants to merge 9 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes the dab start logging flag to --log-level to align with the CLI’s kebab-case conventions, while retaining the legacy --LogLevel as a deprecated alias and updating internal documentation/tests accordingly.
Changes:
- Renamed the
dab startflag from--LogLevelto--log-leveland introduced a hidden legacy option (--LogLevel) with a deprecation warning. - Updated CLI/service log-level plumbing to pass
--log-levelthrough to the engine and refreshed comments/docs to match. - Updated unit/end-to-end tests to use
--log-level, adding coverage for the legacy flag still functioning.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service/Startup.cs | Updates comments to reference the new --log-level startup argument. |
| src/Service/Program.cs | Updates service-side CLI arg parsing/docs to use --log-level for log level initialization. |
| src/Service.Tests/UnitTests/DynamicLogLevelProviderTests.cs | Updates test docs to reference --log-level. |
| src/Core/Telemetry/ILogLevelController.cs | Updates interface docs for precedence to reference --log-level. |
| src/Cli/Utils.cs | Updates CLI state/docs to reference --log-level. |
| src/Cli/Properties/launchSettings.json | Modifies CLI debug profile arguments (currently includes a machine-specific path). |
| src/Cli/Program.cs | Updates early flag parsing to detect --log-level before logger creation. |
| 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 stdio mode. |
| src/Cli/ConfigGenerator.cs | Implements legacy --LogLevel handling with a deprecation warning and forwards --log-level to the engine. |
| src/Cli/Commands/StartOptions.cs | Adds --log-level option and a hidden legacy --LogLevel option. |
| src/Cli.Tests/EndToEndTests.cs | Updates E2E tests to use --log-level and adds legacy --LogLevel 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 detects the new
--log-levelflag. If a user uses the legacy--LogLevelflag (which this PR intends to keep working), the early logging state (Utils.IsCliOverriding/Utils.CliLogLevel) won’t be set, which can keep CLI logs suppressed in MCP stdio mode despite the user explicitly opting in to logging.
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))
{
…cli-loglevel-flag' into dev/rubencerna/fix-cli-loglevel-flag
souvikghosh04
left a comment
Contributor
There was a problem hiding this comment.
Posting comments so far. rethink if we really need the legacy flag or should error out on unsupported format as we generally do and document these.
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.Added new
isLogLevelLegacyvalue that is passed into theDynamicLogLevelProvider.cswhich would then be used to add the log in case the deprecated flag is used in an Aspire scenario. In which the flag is passed to the dll directly and not through CLIHow 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