Add support for BeforeStart Pipeline steps#13780
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13780Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13780" |
…e BeforeStartEvent. This allows for ordering of BeforeStart subscriptions.
…rer and run mode provision steps.
eerhardt
force-pushed
the
BeforeStartPipelineStep
branch
from
0112492 to
8e6c2d8
Compare
Fix tests
…Step # Conflicts: # src/Aspire.Hosting.Azure/AzureEnvironmentResource.cs # src/Aspire.Hosting/Pipelines/DistributedApplicationPipeline.cs # src/Aspire.Hosting/Pipelines/WellKnownPipelineSteps.cs # tests/Aspire.Hosting.Tests/Pipelines/DistributedApplicationPipelineTests.cs
The PR converted AzureResourcePreparer (and others) to a pipeline step that runs as part of the 'before-start' pipeline phase, but several other infrastructure components (AzureContainerAppsInfrastructure, AzureAppServiceInfrastructure) still subscribe to BeforeStartEvent. Because DistributedApplication.ExecuteBeforeStartHooksAsync used to publish BeforeStartEvent before invoking the pipeline, those subscribers ran without the role-assignment resources the pipeline step is responsible for materializing, producing broken references (and a deploy hang). Reorder ExecuteBeforeStartHooksAsync so the 'before-start' pipeline step runs first, then BeforeStartEvent, then the (obsolete) lifecycle hooks. This matches the contract the new pipeline step needs while keeping the existing subscriber model intact. Also: - BaseContainerAppContext: skip null env vars and args to avoid an NRE exposed because pipeline ResolveStepsAsync eagerly evaluates configuration callbacks. - AzureFrontDoorTests.AddAzureFrontDoorThrowsWhenOriginHasNoExternalEndpoints: the failure now surfaces from the pipeline as an InvalidOperationException with a wrapped inner; assert against exception.ToString() and execute via ExecuteBeforeStartHooksAsync. - AzureEnvironmentResourceExtensionsTests: rename run-mode test to reflect the PR's new behavior (hidden resource is added to the model rather than being skipped entirely). - AzureProvisionerExtensions: drop a dead commented using directive and a stale comment. - Revert the snapshot baselines that were updated to match the buggy ordering. With the pipeline running before BeforeStartEvent, the generated bicep parameter order matches main again. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cture to pipeline steps Replaces the previous Option 2 fix (reordering BeforeStartEvent vs pipeline). Both infrastructures are now registered as pipeline steps that depend on the 'azure-prepare-resources' step and feed into 'before-start'. This models the dependency between role-assignment materialization and deployment-target generation correctly. - AzureContainerAppsInfrastructure: no longer IDistributedApplicationEventingSubscriber; exposed as DI singleton with PrepareDeploymentTargetsAsync method. - AzureAppServiceInfrastructure: same conversion. - AzureContainerAppExtensions / AzureAppServiceEnvironmentExtensions: register pipeline steps with marker singletons for idempotency, dependsOn azure-prepare-resources, requiredBy before-start. - AzureEnvironmentResource: PrepareResourcesStepName now public const for cross-package use. - DistributedApplication: reverted Option 2 reorder (pipeline back to running after BeforeStartEvent / lifecycle hooks since AppContainers/AppService no longer subscribe to BeforeStartEvent). - Tests: 8 NotSupportedException assertions updated to assert outer InvalidOperationException with inner NotSupportedException (pipeline wraps non-DistributedApplicationException failures). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ExecuteStepsAsTaskDag computes parent/level info via GetStepHierarchyByStep and passes it to IPipelineActivityReporter.CreateStepAsync so steps render nested in the dashboard. ExecuteStepsSequentially was calling the simpler CreateStepAsync(name, ct) overload, causing sequentially-executed steps (e.g. before-start) to appear flat regardless of their dependency relationships. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…moved to the resource itself now.
…ed to the resource itself now.
…e is only 1 in the model. Also some clean up
|
Re-running the failed jobs in the CI workflow for this pull request because 1 job was identified as retry-safe transient failures in the CI run attempt.
|
|
❌ Deployment E2E Tests failed — 26 passed, 1 failed, 0 cancelled View test results and recordings
|
IEvangelist
left a comment
IEvangelist
left a comment
There was a problem hiding this comment.
This is a really nice addition and the design is clean — reusing pipeline steps (with their DependsOn/RequiredBy graph) for BeforeStart ordering is a much more composable story than trying to add ordering to event subscribers, and extending that pattern across the Azure compute hosting packages ties it all together well. The Clone() + isolated sequential execution for BeforeStart is a thoughtful way to keep singleton pipeline state clean, and the EnsureComputeEnvironmentAnnotationsApplied helper centralizes a bit of scattered convention.
A few concerns noted inline — mostly around unintended behavior changes from replacing mode-gated event subscribers with ungated pipeline steps, and a couple of idempotency concerns now that steps may run in more code paths.
| /// a target environment — without each consumer needing to re-implement the "single env wins" | ||
| /// fallback. | ||
| /// </remarks> | ||
| private static void EnsureComputeEnvironmentAnnotationsApplied(DistributedApplicationModel appModel) |
There was a problem hiding this comment.
When >1 compute environment is present this is a silent no-op. That's fine per the XML remarks, but combined with the per-env prepare steps (which skip resources not bound to their env), a developer who forgets WithComputeEnvironment(...) on a compute resource will silently end up with no DeploymentTargetAnnotation — and therefore nothing deployed — rather than a clear error. Worth confirming there's an explicit validation step elsewhere that fails with a helpful message for orphaned compute resources in the multi-env case; if not, this would be a good place to emit one.
There was a problem hiding this comment.
This is a good catch. We used to throw an error with Allow multiple compute environment resources in an app model (microsoft/aspire#8820). But Fix: Skip resources targeted to different compute environments (microsoft/aspire#14177) regressed this error.
Since this is already broken in main, I will fix it in a follow up PR
- Only add run-mode-azure-provision step in run mode. - Remove comments that no longer make sense
|
Re-running the failed jobs in the CI workflow for this pull request because 2 jobs were identified as retry-safe transient failures in the CI run attempt.
|
Description
There are many cases where we need to order BeforeStartEvent subscriptions. For example, if someone wants something to run before the AzureResourcePreparer, so it can add role assignments correctly, there is no way to ensure the AzureResourcePreparer's BeforeStartEvent subscription hasn't already run.
To fix this, we reuse the Pipeline steps, which already has dependencies built in to it. Then we convert the Azure BeforeStartEvent subscriptions to pipeline steps, so they can be depended on (or required by).
I can do the other compute environments in a separate PR to keep the changes easier to review. Here's the draft: Refactor K8s, Docker Compose, and AKS infrastructure to pipeline steps (eerhardt/aspire#157)
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate