-
Notifications
You must be signed in to change notification settings - Fork 853
Add command result support for resource commands #15622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,22 @@ public enum IconVariant | |
| Filled | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Specifies the format of a command result. | ||
| /// </summary> | ||
| public enum CommandResultFormat | ||
| { | ||
| /// <summary> | ||
| /// Plain text result. | ||
| /// </summary> | ||
| Text, | ||
|
|
||
| /// <summary> | ||
| /// JSON result. | ||
| /// </summary> | ||
| Json | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// A factory for <see cref="ExecuteCommandResult"/>. | ||
| /// </summary> | ||
|
|
@@ -124,12 +140,27 @@ public static class CommandResults | |
| /// </summary> | ||
| public static ExecuteCommandResult Success() => new() { Success = true }; | ||
|
|
||
| /// <summary> | ||
| /// Produces a success result with result data. | ||
| /// </summary> | ||
| /// <param name="result">The result data.</param> | ||
| /// <param name="resultFormat">The format of the result data. Defaults to <see cref="CommandResultFormat.Text"/>.</param> | ||
| public static ExecuteCommandResult Success(string result, CommandResultFormat resultFormat = CommandResultFormat.Text) => new() { Success = true, Result = result, ResultFormat = resultFormat }; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentionally non-nullable — if you have no result, call |
||
|
|
||
| /// <summary> | ||
| /// Produces an unsuccessful result with an error message. | ||
| /// </summary> | ||
| /// <param name="errorMessage">An optional error message.</param> | ||
| public static ExecuteCommandResult Failure(string? errorMessage = null) => new() { Success = false, ErrorMessage = errorMessage }; | ||
|
|
||
| /// <summary> | ||
| /// Produces an unsuccessful result with an error message and result data. | ||
| /// </summary> | ||
| /// <param name="errorMessage">The error message.</param> | ||
| /// <param name="result">The result data.</param> | ||
| /// <param name="resultFormat">The format of the result data. Defaults to <see cref="CommandResultFormat.Text"/>.</param> | ||
| public static ExecuteCommandResult Failure(string errorMessage, string result, CommandResultFormat resultFormat = CommandResultFormat.Text) => new() { Success = false, ErrorMessage = errorMessage, Result = result, ResultFormat = resultFormat }; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, should
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reasoning — this overload is for when you explicitly have both an error message and result data. The existing |
||
|
|
||
| /// <summary> | ||
| /// Produces a canceled result. | ||
| /// </summary> | ||
|
|
@@ -162,6 +193,16 @@ public sealed class ExecuteCommandResult | |
| /// An optional error message that can be set when the command is unsuccessful. | ||
| /// </summary> | ||
| public string? ErrorMessage { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// An optional result value produced by the command. | ||
| /// </summary> | ||
| public string? Result { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// The format of the <see cref="Result"/> value. | ||
| /// </summary> | ||
| public CommandResultFormat? ResultFormat { get; init; } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,7 +106,13 @@ public async Task<ExecuteCommandResult> ExecuteCommandAsync(IResource resource, | |
|
|
||
| if (failures.Count == 0 && cancellations.Count == 0) | ||
| { | ||
| return new ExecuteCommandResult { Success = true }; | ||
| var successWithResult = results.FirstOrDefault(r => r.Result is not null); | ||
| return new ExecuteCommandResult | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if there are different replicas of a resource? Should the result aggregate all results and send those back in an array of results or something like that?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re @joperezr's replicas comment, yes, it should - maybe also have the ability to run a command on only one instance?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently takes the first non-null result from replicas. Aggregating into an array would change the result contract and add complexity. Running on a single instance is a separate feature. Both are worth tracking as follow-ups but out of scope for v1. |
||
| { | ||
| Success = true, | ||
| Result = successWithResult?.Result, | ||
| ResultFormat = successWithResult?.ResultFormat | ||
| }; | ||
| } | ||
| else if (failures.Count == 0 && cancellations.Count > 0) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -285,6 +285,16 @@ internal sealed class ExecuteResourceCommandResponse | |
| /// Gets the error message if the command failed. | ||
| /// </summary> | ||
| public string? ErrorMessage { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the result data produced by the command. | ||
| /// </summary> | ||
| public string? Result { get; init; } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. enapsulate w/format?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same — encapsulation is a good future refactor. |
||
|
|
||
| /// <summary> | ||
| /// Gets the format of the result data (e.g. "none", "text", "json"). | ||
| /// </summary> | ||
| public string? ResultFormat { get; init; } | ||
| } | ||
|
|
||
| #endregion | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -360,7 +360,7 @@ async Task WatchResourceConsoleLogsInternal(bool suppressFollow, CancellationTok | |
|
|
||
| public override async Task<ResourceCommandResponse> ExecuteResourceCommand(ResourceCommandRequest request, ServerCallContext context) | ||
| { | ||
| var (result, errorMessage) = await serviceData.ExecuteCommandAsync(request.ResourceName, request.CommandName, context.CancellationToken).ConfigureAwait(false); | ||
| var (result, errorMessage, commandResult, resultFormat) = await serviceData.ExecuteCommandAsync(request.ResourceName, request.CommandName, context.CancellationToken).ConfigureAwait(false); | ||
| var responseKind = result switch | ||
| { | ||
| ExecuteCommandResultType.Success => ResourceCommandResponseKind.Succeeded, | ||
|
|
@@ -369,11 +369,24 @@ public override async Task<ResourceCommandResponse> ExecuteResourceCommand(Resou | |
| _ => ResourceCommandResponseKind.Undefined | ||
| }; | ||
|
|
||
| return new ResourceCommandResponse | ||
| var response = new ResourceCommandResponse | ||
| { | ||
| Kind = responseKind, | ||
| ErrorMessage = errorMessage ?? string.Empty | ||
| ErrorMessage = errorMessage ?? string.Empty, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this also use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a grpc quirk |
||
| ResultFormat = resultFormat switch | ||
| { | ||
| ApplicationModel.CommandResultFormat.Text => Aspire.DashboardService.Proto.V1.CommandResultFormat.Text, | ||
| ApplicationModel.CommandResultFormat.Json => Aspire.DashboardService.Proto.V1.CommandResultFormat.Json, | ||
| _ => Aspire.DashboardService.Proto.V1.CommandResultFormat.None | ||
| } | ||
| }; | ||
|
|
||
| if (commandResult is not null) | ||
| { | ||
| response.Result = commandResult; | ||
| } | ||
|
|
||
| return response; | ||
| } | ||
|
|
||
| private async Task ExecuteAsync(Func<CancellationToken, Task> execute, ServerCallContext serverCallContext) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't seem right, shouldn't we be adding a parameter to ShowStatusAsync that accepts ConsoleOutput, like DisplayRawText does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we want all messages to go to stderr and output to be stdout. This is similar to what we do when —format json is specified