-
Notifications
You must be signed in to change notification settings - Fork 720
feat: aspire exec 2 #10240
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
feat: aspire exec 2 #10240
Conversation
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.
Pull Request Overview
This PR integrates “exec” functionality into the existing run mode by introducing an ExecResourceManager that initializes via BeforeStartEvent, reuses the resource pipeline, and streams command logs over the backchannel. The CLI’s exec command is added (instead of a separate --operation exec), and both unit and E2E tests are updated to cover the new behavior.
- Move exec initialization from a background service to event-driven in
ExecResourceManager - Wire up exec logging through the backchannel (
ExecAsync) and updateDistributedApplicationBuilder - Add
execcommand to CLI, update interaction services, and expand test coverage
Reviewed Changes
Copilot reviewed 53 out of 54 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting/Exec/ExecResourceManager.cs | New manager for exec resource initialization and log streaming |
| src/Aspire.Hosting/DistributedApplicationBuilder.cs | Configure exec options, subscribe to BeforeStartEvent |
| src/Aspire.Hosting/Exec/ExecOptions.cs | Define ExecOptions settings |
| src/Aspire.Hosting/Backchannel/AppHostRpcTarget.cs / BackchannelDataTypes.cs | Add ExecAsync and CommandOutput types |
| src/Aspire.Cli/Commands/ExecCommand.cs | Implement exec CLI command |
| src/Aspire.Cli/Interaction/IInteractionService.cs / ConsoleInteractionService.cs | Add WriteConsoleLog API and formatting |
| tests/Aspire.Cli.Tests/Commands/ExecCommandTests.cs / tests/Aspire.Cli.Tests/E2E/ExecTests.cs | Unit and E2E tests for exec scenarios |
Files not reviewed (1)
- src/Aspire.Cli/Resources/ExecCommandStrings.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
src/Aspire.Hosting/Exec/ExecResourceManager.cs:44
- [nitpick]
StreamExecResourceLogscontains complex logic for waiting, state transitions, and log streaming but lacks direct unit tests; adding targeted tests for resource initialization, waiting logic, and log output would improve confidence.
public async IAsyncEnumerable<CommandOutput> StreamExecResourceLogs([EnumeratorCancellation] CancellationToken cancellationToken)
src/Aspire.Cli/Interaction/IInteractionService.cs:28
- The new
WriteConsoleLogmethod lacks an XML doc comment; consider adding a<summary>and parameter descriptions to align with existing interface documentation.
void WriteConsoleLog(string message, int? lineNumber = null, string? type = null, bool isErrorMessage = false);
| return targetExecResource switch | ||
| { | ||
| ProjectResource prj => BuildAgainstProjectResource(prj), | ||
| _ => throw new NotImplementedException(nameof(targetExecResource)) |
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.
When this throws what is the experience? What crashes? How do you observe it?
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.
refactored a bit - now any exception occured during CreateExecResource will be saved and then translated to the cli (user) in the same manner. Changed exception messages to be user-friendly and not having any implementation details.
| annotation is EnvironmentAnnotation or EnvironmentCallbackAnnotation | ||
| or ResourceRelationshipAnnotation or WaitAnnotation)) |
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.
Why ResourceRelationshipAnnotation ?
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.
|
|
||
| var migrationName = "AddVersion"; | ||
|
|
||
| var apiModelProjectDir = @$"{MSBuildUtils.GetRepoRoot()}\playground\DatabaseMigration\DatabaseMigration.ApiModel\DatabaseMigration.ApiModel.csproj"; |
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.
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 test is quarantined at a moment due to problems running it on the agent (there are a couple). I have an issue filed to take a look at this later.
| var resourceOption = new Option<string>("--resource", "-r"); | ||
| resourceOption.Description = ExecCommandStrings.TargetResourceArgumentDescription; | ||
| Options.Add(resourceOption); | ||
|
|
||
| var startResourceOption = new Option<string>("--start-resource", "-s"); | ||
| startResourceOption.Description = ExecCommandStrings.StartTargetResourceArgumentDescription; | ||
| Options.Add(startResourceOption); |
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.
@DeagleGross this feedback shouldn't block the PR merging, just more wanted to trigger a conversation with @DamianEdwards about these switches.
I was wondering whether this should be --wait-for instead of --start-resource. I think this probably aligns better with how this executable resource might look if it was defined in the apphost itself.
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.
Seems to me that waiting for the target resource would be an additional and dependent option to starting the target resource? i.e. they're not the same thing, I may or may not want to start the target resource. Perhaps this should instead be something like --resource-action with choices of none, start, wait-for, etc.? We would likely have to debate the default as the "optimum" default is likely very scenario driven, e.g. EF migration creation vs. container exec, etc. Maybe the default could be different depending on the kind of resource specified by --resource? Some things to consider.
| _ = Task.Run(async () => | ||
| { | ||
| await _resourceNotificationService.WaitForResourceAsync(execResource!.Name, targetStates: KnownResourceStates.TerminalStates, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // hack: https://github.com/dotnet/aspire/issues/10245 | ||
| // workarounds the race-condition between streaming all logs from the resource, and resource completion | ||
| await Task.Delay(1000, CancellationToken.None).ConfigureAwait(false); | ||
|
|
||
| _resourceLoggerService.Complete(dcpExecResourceName); // complete stops the `WatchAsync` async-foreach below | ||
| }, cancellationToken); |
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.
@karolz-ms do you have any recommendations here?
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.
@DeagleGross you shouldn't be completing the log stream. The system should do that when the execution ends.
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.
unfortunately, that does not work as we would expect. DCP does not close the WatchAsync on its own once the resource completes. We need to complete it somehow today, otherwise cli interaction with just hang here.
I have filed a separate issue to work on this; for now we would have a Task.Delay as a workaround, and we will update it once we have a solution (either new API or different DCP behavior or whatever)
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.
I had a conversation with @DeagleGross about this, and I think it is fine to take this implementation as is is for now, but we should use the issue that Dmitry has opened to dig in and understand why WatchAsync does not complete for resources that reach the final state (which is the case here) because I think it should, and I explained why in the related issue.
| /// <returns> | ||
| /// A new instance of <see cref="IDistributedApplicationTestingBuilder"/>. | ||
| /// </returns> | ||
| public static IDistributedApplicationTestingBuilder Create( |
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.
What is the purpose of the changes in Aspire.Hosting.Testing?
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.
I am trying to setup tests against existing apphost (in my case playground/DatabaseMigrations). I need to lookup an assembly like that: typeof(DatabaseMigration_AppHost).Assembly. I have hid it under internal + exposed to Aspire.Cli.Tests via InternalsVisibleTo.
About the approach: my approach with using an existing apphost with some dependency of resources setup gives a great debugging experience and a way to fully test running AppHost + Dcp + even streaming the logs via AppHostRpcTarget. Tests code represent cli in this case, and apphost is, well, apphost.
I would really love to keep such an approach - it allowed me to iterate quickly and basically checks that functionality works as intended e2e, and if you insist on it - I can move usage to other test apphost (for example TestingAppHost1), but I dont see many problems targeting playground.
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 is the thing I'm now most concerned about with this PR. We've learned from experience that these kinds of tests tend to be less reliable and tend to get disabled and quarantined pretty quickly reducing test coverage.
As a team we need to come up with a better way to do end to end testing for the CLI which isn't quite so flaky.
| private async Task<DistributedApplication> BuildAppAsync(string[] args, Action<DistributedApplicationOptions, HostApplicationBuilderSettings>? configureBuilder = null) | ||
| { | ||
| configureBuilder ??= (appOptions, _) => { }; | ||
| var builder = DistributedApplicationTestingBuilder.Create(args, configureBuilder, typeof(DatabaseMigration_AppHost).Assembly) |
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.
I think for the test cases here I would like to avoid taking a project reference dependency on the database migration project. This might also alleviate the need to make public API changes to the DistributedApplicationTestingBuilder. Instead I think a more efficient approach (and a approach that helps better isolate issues) is to build an apphost in the test case itself then instansiate a backchannel to the apphost inside the testcase itself.
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.
Instead I think a more efficient approach (and a approach that helps better isolate issues) is to build an apphost in the test case itself then instansiate a backchannel to the apphost inside the testcase itself.
Sorry, did not catch this idea. I answered about the approach taken here: #10240 (comment)
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.
Functionality wise this is working for me now (even for really short running commands). I think that we still have some more work to do though. Here are my high level notes:
-
Lets see if we can avoid making any public API changes to the DistributedTestApplicationBuilder (it seems like we've done this just to validate the scenario with the migration projects, my suggestion is do a smaller more focused set of tests, ones that focus on testing the apphost side,and ones that focus on testing the CLI side - mocking out the other side each time).
-
I think we need to get rid of the light background on the text we are rendering to the screen. Just don't set the background color and let the terminal use what it has. In that case rendering yellow for the pre-text might not be great.
-
exit codes. I think we should consider returning the exit code from the executable resource when it runs. At the moment regardless of whether the command succeeds or fails we get exit code 0 which isn't great for scripting. This means we might need to add an exit code to the command output which is nullable, but on the last command output we send back populate the value with the exit code and return that from the CLI side.
tests/TestingAppHost1/TestingAppHost1.AppHost/appsettings.Development.json
Outdated
Show resolved
Hide resolved
tests/Aspire.Cli.Tests/TestServices/TestConsoleInteractionService.cs
Outdated
Show resolved
Hide resolved
I think we really want to have the exit code be returned from the CLI process. I expect that people will want to script aspire exec and they'll need to use the exit codes to determine success/failure of the executable resources. Returning the exit code as text forces them to have to do something like grep the text output to extract the text output. Why not just add a nullable field to the |
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.
Lets ship it. Keep a close eye on the helix tests, we may need to quarantine and take a different approach if these tests fail.



Effectively is #10061 but with a couple of changes around DCP and usage.
Changes
DcpExectutornow.BackgroundServicenowExecResourceManagersubscribes toBeforeStartEventand initializes the exec resource there. Whole resource manager pipeline is reused now.ExecResourceManagerto not generate its own DCP name for theExecutableResource, but to wait until DCP starts the resource and fetch it.--operation exec, but it's an extension ofrunmode. Determines if it is exec or not based on--commandflag existence.Fixes #9859