Split up publish and deploy and unify graphs#11175
Conversation
|
🚀 Dogfood this PR with: curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11175Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11175" |
There was a problem hiding this comment.
Pull Request Overview
This PR contributes to separating publish and deploy operations while unifying resource deployment graphs. The changes remove the sequential execution of publishing followed by deploying callbacks when deploying, making the operations truly independent.
Key Changes
- Modified publishing logic to only execute deploy callbacks when deploying, and only publish callbacks when publishing
- Removed the ResourceDeploymentGraph class and simplified Azure resource provisioning to run in parallel instead of in dependency order
- Made output path optional for deploy operations while keeping it required for publish operations
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/PublishingTests.cs | Updated tests to verify publishing callbacks are not called during deploy operations |
| tests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs | Modified test expectations to verify files are not created during deploy-only operations |
| tests/Aspire.Cli.Tests/Commands/DeployCommandTests.cs | Added test for deploy command working without output path |
| src/Aspire.Hosting/Publishing/Publisher.cs | Refactored to separate publish and deploy logic flows |
| src/Aspire.Hosting/Publishing/DeployingContext.cs | Made OutputPath nullable to support deploy without artifacts |
| src/Aspire.Hosting.Azure/Provisioning/ResourceDeploymentGraph.cs | Removed entire file containing dependency graph implementation |
| src/Aspire.Hosting.Azure/AzureDeployingContext.cs | Replaced graph-based deployment with parallel provisioning |
| src/Aspire.Cli/Resources/DeployCommandStrings.resx | Updated description to indicate output path is optional |
| src/Aspire.Cli/Commands/PublishCommandBase.cs | Made output path nullable and removed default value factory |
| src/Aspire.Cli/Commands/PublishCommand.cs | Updated to handle nullable output path with fallback |
| src/Aspire.Cli/Commands/DeployCommand.cs | Modified to conditionally include output path in arguments |
| if (resource is AzureBicepResource bicepResource) | ||
| { | ||
| var resourceTask = await deployingStep.CreateTaskAsync($"Provisioning {resource.Name}", cancellationToken).ConfigureAwait(false); | ||
| await using (resourceTask.ConfigureAwait(false)) | ||
| var resourceTask = await deployingStep.CreateTaskAsync($"Deploying {resource.Name}", cancellationToken).ConfigureAwait(false); | ||
|
|
||
| var provisioningTask = Task.Run(async () => | ||
| { | ||
| await bicepProvisioner.GetOrCreateResourceAsync(bicepResource, context, cancellationToken).ConfigureAwait(false); | ||
| await resourceTask.CompleteAsync($"Successfully provisioned {resource.Name}", CompletionState.Completed, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| await using (resourceTask.ConfigureAwait(false)) | ||
| { | ||
| try | ||
| { | ||
| bicepResource.ProvisioningTaskCompletionSource = new(TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
|
||
| await bicepProvisioner.GetOrCreateResourceAsync(bicepResource, provisioningContext, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| bicepResource.ProvisioningTaskCompletionSource?.TrySetResult(); | ||
|
|
||
| await resourceTask.CompleteAsync($"Successfully provisioned {bicepResource.Name}", CompletionState.Completed, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| await resourceTask.CompleteAsync($"Failed to provision {bicepResource.Name}: {ex.Message}", CompletionState.CompletedWithError, cancellationToken).ConfigureAwait(false); | ||
| bicepResource.ProvisioningTaskCompletionSource?.TrySetException(ex); | ||
| throw; | ||
| } | ||
| } | ||
| }, cancellationToken); | ||
|
|
||
| provisioningTasks.Add(provisioningTask); | ||
| } |
There was a problem hiding this comment.
The type check 'resource is AzureBicepResource bicepResource' is redundant since the collection 'bicepResources' is already typed as List. The foreach variable can be used directly as AzureBicepResource.
|
This is a good change overall, but I do worry about the inability to diagnose deployment failures (by looking at the intermediate files). We'll need to build UX for this. |
|
PS: I'm surprised you haven't fixed this yet 😄 |
I think we can do something like |
|
🚀 Dogfood this PR with: curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11175Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11175" |
* Split up publish and deploy and unify graphs * Feedback * Quarantine test
Contributes towards #10448.