Consistently send messages to stderr when format is JSON. And display status message in more situations#14670
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14670Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14670" |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #14646 by adding a "Getting logs..." status message to provide user feedback during log collection in the aspire logs command. The change improves user experience when the command takes time to collect logs, especially when no arguments are provided.
Changes:
- Added "Getting logs..." localized status message displayed during log collection
- Wrapped
CollectLogsAsynccalls withShowStatusAsyncto display the status message in both snapshot and watch modes - Added a development launch profile for easier testing of the logs command
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Cli/Commands/LogsCommand.cs | Wrapped log collection operations with status message display |
| src/Aspire.Cli/Resources/LogsCommandStrings.resx | Added "GettingLogs" resource string |
| src/Aspire.Cli/Resources/LogsCommandStrings.Designer.cs | Generated code for new resource string |
| src/Aspire.Cli/Resources/xlf/*.xlf | Localization files updated with new string marked as state="new" |
| src/Aspire.Cli/Properties/launchSettings.json | Added "logs" launch profile for development convenience |
Files not reviewed (1)
- src/Aspire.Cli/Resources/LogsCommandStrings.Designer.cs: Language not supported
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #22420069544 |
JamesNK
force-pushed
the
jamesnk/logs-display-get-message
branch
from
3fb284d to
d9e2ac5
Compare
JamesNK
changed the title
JamesNK
force-pushed
the
jamesnk/logs-display-get-message
branch
from
124aab1 to
448d831
Compare
| /// <summary> | ||
| /// Checks whether this command has a --format option whose parsed value is <see cref="OutputFormat.Json"/>. | ||
| /// </summary> | ||
| private bool IsJsonFormatRequested(ParseResult parseResult) |
There was a problem hiding this comment.
Why not just parseResult.GetValue("--format")?
There was a problem hiding this comment.
I only want to change the default console output if the command supports it, i.e. has a --format option. That means we need to get it, and since we have it then we can use it to get the value.
|
I think it would be good to have an E2E CLI test for all of these JSON outputting commands. Something that basically runs the CLI through its paces and does |
mitchdenny
left a comment
mitchdenny
left a comment
There was a problem hiding this comment.
Approve pending some E2E CLI tests.
Description
Consistently output non-JSON content to stderr when format is JSON. Set in BaseCommand to automatically set value.
Also display status message in more commands even when format is JSON because it's sent to stderr.
Also display status message when getting logs because it can take a while. Addresses #14646
Checklist
<remarks />and<code />elements on your triple slash comments?aspire.devissue: