Draft
Conversation
Contributor
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15607Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15607" |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new aspire dashboard top-level CLI command to launch the standalone Aspire Dashboard via the bundled aspire-managed binary, integrating it into the root command set and localized resources.
Changes:
- Registers a new
DashboardCommandin the CLI host and root command, categorized under the Monitoring help group. - Implements foreground and
--detachexecution paths and introduces localized resource strings for the new command. - Adds initial unit tests covering
--helpand the “bundle not available” error case.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Cli.Tests/Utils/CliTestHelper.cs | Registers DashboardCommand in the test DI container. |
| tests/Aspire.Cli.Tests/Commands/DashboardCommandTests.cs | Adds tests for help and missing-bundle error behavior. |
| src/Aspire.Cli/Program.cs | Registers DashboardCommand in the production DI container. |
| src/Aspire.Cli/Commands/RootCommand.cs | Adds dashboard as a root subcommand. |
| src/Aspire.Cli/Commands/DashboardCommand.cs | Implements the new aspire dashboard command (foreground/detach + arg pass-through). |
| src/Aspire.Cli/Resources/DashboardCommandStrings.resx | Adds new localized strings for the dashboard command. |
| src/Aspire.Cli/Resources/DashboardCommandStrings.Designer.cs | Strongly-typed accessor for the new dashboard strings. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.cs.xlf | Localization XLF for cs. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.de.xlf | Localization XLF for de. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.es.xlf | Localization XLF for es. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.fr.xlf | Localization XLF for fr. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.it.xlf | Localization XLF for it. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.ja.xlf | Localization XLF for ja. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.ko.xlf | Localization XLF for ko. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.pl.xlf | Localization XLF for pl. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.pt-BR.xlf | Localization XLF for pt-BR. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.ru.xlf | Localization XLF for ru. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.tr.xlf | Localization XLF for tr. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.zh-Hans.xlf | Localization XLF for zh-Hans. |
| src/Aspire.Cli/Resources/xlf/DashboardCommandStrings.zh-Hant.xlf | Localization XLF for zh-Hant. |
Files not reviewed (1)
- src/Aspire.Cli/Resources/DashboardCommandStrings.Designer.cs: Language not supported
Comment on lines
+67
to
+78
| var dashboardArgs = new List<string> { "dashboard" }; | ||
| dashboardArgs.AddRange(parseResult.UnmatchedTokens); | ||
|
|
||
| var detach = parseResult.GetValue(s_detachOption); | ||
|
|
||
| if (detach) | ||
| { | ||
| return ExecuteDetached(managedPath, dashboardArgs); | ||
| } | ||
|
|
||
| return await ExecuteForegroundAsync(managedPath, dashboardArgs, cancellationToken).ConfigureAwait(false); | ||
| } |
There was a problem hiding this comment.
Current tests cover --help and the bundle-not-available failure, but they don't cover the new argument pass-through behavior (especially handling of --), or the detach/foreground launch paths. Add unit tests that verify the forwarded argument list and the --detach behavior (e.g., by introducing an injectable process launcher abstraction so tests can assert the executable/args/working directory without spawning real processes).
Comment on lines
+68
to
+69
| dashboardArgs.AddRange(parseResult.UnmatchedTokens); | ||
|
|
There was a problem hiding this comment.
parseResult.UnmatchedTokens can include the "--" delimiter (see how ExecCommand explicitly searches for it). Forwarding the delimiter to aspire-managed dashboard will cause everything after it to be treated as positional args, so aspire dashboard -- --urls ... won't work as intended. Strip the delimiter before appending pass-through args (i.e., forward only the tokens after -- when present).
Suggested change
| dashboardArgs.AddRange(parseResult.UnmatchedTokens); | |
| var unmatchedTokens = parseResult.UnmatchedTokens; | |
| var startIndex = 0; | |
| for (var i = 0; i < unmatchedTokens.Count; i++) | |
| { | |
| if (unmatchedTokens[i] == "--") | |
| { | |
| startIndex = i + 1; | |
| break; | |
| } | |
| } | |
| for (var i = startIndex; i < unmatchedTokens.Count; i++) | |
| { | |
| dashboardArgs.Add(unmatchedTokens[i]); | |
| } |
Comment on lines
+84
to
+86
| var process = DetachedProcessLauncher.Start(managedPath, dashboardArgs, Directory.GetCurrentDirectory()); | ||
|
|
||
| _interactionService.DisplayMessage(KnownEmojis.Rocket, |
There was a problem hiding this comment.
Detached mode uses Directory.GetCurrentDirectory() for the child process working directory, which can diverge from ExecutionContext.WorkingDirectory (notably in tests and when the CLI changes working directory semantics). Use ExecutionContext.WorkingDirectory.FullName (and consider passing the same working directory to LayoutProcessRunner.Start for consistency).
Comment on lines
+84
to
+118
| var process = DetachedProcessLauncher.Start(managedPath, dashboardArgs, Directory.GetCurrentDirectory()); | ||
|
|
||
| _interactionService.DisplayMessage(KnownEmojis.Rocket, | ||
| string.Format(CultureInfo.CurrentCulture, DashboardCommandStrings.DashboardStarted, process.Id)); | ||
|
|
||
| return ExitCodeConstants.Success; | ||
| } | ||
|
|
||
| private async Task<int> ExecuteForegroundAsync(string managedPath, List<string> dashboardArgs, CancellationToken cancellationToken) | ||
| { | ||
| _logger.LogDebug("Starting dashboard in foreground: {ManagedPath}", managedPath); | ||
|
|
||
| using var process = LayoutProcessRunner.Start(managedPath, dashboardArgs, redirectOutput: false); | ||
|
|
||
| try | ||
| { | ||
| await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| if (!process.HasExited) | ||
| { | ||
| process.Kill(entireProcessTree: true); | ||
| } | ||
|
|
||
| return ExitCodeConstants.Success; | ||
| } | ||
|
|
||
| if (process.ExitCode != 0) | ||
| { | ||
| _interactionService.DisplayError( | ||
| string.Format(CultureInfo.CurrentCulture, DashboardCommandStrings.DashboardExitedWithError, process.ExitCode)); | ||
| } | ||
|
|
||
| return process.ExitCode == 0 ? ExitCodeConstants.Success : ExitCodeConstants.DashboardFailure; |
There was a problem hiding this comment.
DetachedProcessLauncher.Start(...) / LayoutProcessRunner.Start(...) can throw (e.g., permission issues, missing execute bit, invalid binary). Right now those exceptions bubble to the top-level handler and become a generic "unexpected error" with exit code 1, rather than returning ExitCodeConstants.DashboardFailure with a dashboard-specific message. Catch exceptions around process start/launch and convert them into a clear DisplayError(...) + DashboardFailure exit code.
Suggested change
| var process = DetachedProcessLauncher.Start(managedPath, dashboardArgs, Directory.GetCurrentDirectory()); | |
| _interactionService.DisplayMessage(KnownEmojis.Rocket, | |
| string.Format(CultureInfo.CurrentCulture, DashboardCommandStrings.DashboardStarted, process.Id)); | |
| return ExitCodeConstants.Success; | |
| } | |
| private async Task<int> ExecuteForegroundAsync(string managedPath, List<string> dashboardArgs, CancellationToken cancellationToken) | |
| { | |
| _logger.LogDebug("Starting dashboard in foreground: {ManagedPath}", managedPath); | |
| using var process = LayoutProcessRunner.Start(managedPath, dashboardArgs, redirectOutput: false); | |
| try | |
| { | |
| await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); | |
| } | |
| catch (OperationCanceledException) | |
| { | |
| if (!process.HasExited) | |
| { | |
| process.Kill(entireProcessTree: true); | |
| } | |
| return ExitCodeConstants.Success; | |
| } | |
| if (process.ExitCode != 0) | |
| { | |
| _interactionService.DisplayError( | |
| string.Format(CultureInfo.CurrentCulture, DashboardCommandStrings.DashboardExitedWithError, process.ExitCode)); | |
| } | |
| return process.ExitCode == 0 ? ExitCodeConstants.Success : ExitCodeConstants.DashboardFailure; | |
| try | |
| { | |
| var process = DetachedProcessLauncher.Start(managedPath, dashboardArgs, Directory.GetCurrentDirectory()); | |
| _interactionService.DisplayMessage( | |
| KnownEmojis.Rocket, | |
| string.Format(CultureInfo.CurrentCulture, DashboardCommandStrings.DashboardStarted, process.Id)); | |
| return ExitCodeConstants.Success; | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Failed to start dashboard in detached mode: {ManagedPath}", managedPath); | |
| _interactionService.DisplayError( | |
| string.Format(CultureInfo.CurrentCulture, "Failed to start Aspire dashboard: {0}", ex.Message)); | |
| return ExitCodeConstants.DashboardFailure; | |
| } | |
| } | |
| private async Task<int> ExecuteForegroundAsync(string managedPath, List<string> dashboardArgs, CancellationToken cancellationToken) | |
| { | |
| _logger.LogDebug("Starting dashboard in foreground: {ManagedPath}", managedPath); | |
| try | |
| { | |
| using var process = LayoutProcessRunner.Start(managedPath, dashboardArgs, redirectOutput: false); | |
| try | |
| { | |
| await process.WaitForExitAsync(cancellationToken).ConfigureAwait(false); | |
| } | |
| catch (OperationCanceledException) | |
| { | |
| if (!process.HasExited) | |
| { | |
| process.Kill(entireProcessTree: true); | |
| } | |
| return ExitCodeConstants.Success; | |
| } | |
| if (process.ExitCode != 0) | |
| { | |
| _interactionService.DisplayError( | |
| string.Format(CultureInfo.CurrentCulture, DashboardCommandStrings.DashboardExitedWithError, process.ExitCode)); | |
| } | |
| return process.ExitCode == 0 ? ExitCodeConstants.Success : ExitCodeConstants.DashboardFailure; | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Failed to start dashboard in foreground: {ManagedPath}", managedPath); | |
| _interactionService.DisplayError( | |
| string.Format(CultureInfo.CurrentCulture, "Failed to start Aspire dashboard: {0}", ex.Message)); | |
| return ExitCodeConstants.DashboardFailure; | |
| } |
Contributor
|
🎬 CLI E2E Test Recordings — 52 recordings uploaded (commit View recordings
📹 Recordings uploaded automatically from CI run #23580864045 |
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.
Description
Add a new
aspire dashboardCLI command that starts a standalone Aspire Dashboard instance using theaspire-managed dashboardbinary from the CLI bundle.Features:
--detach): Starts the dashboard in the background, prints the PID, and exitsaspire dashboard -- --urls http://localhost:18888)Implementation details:
aspire-managedbinary viaILayoutDiscoveryand the existing bundle layout systemLayoutProcessRunner.Start()with no output redirection (dashboard inherits the console)DetachedProcessLauncher.Start()for clean background process launchingMonitoringhelp group alongside telemetry/otel commandsChecklist
aspire.devissue: