Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Sep 30, 2025

  • Explore repository structure and understand the issue
  • Fix ParameterProcessor.CollectDependentParameterResourcesAsync to use existing executionContext
  • Fix ParameterProcessor.CollectDependentParameterResourcesAsync to skip resources excluded from publish
  • Add test for execution context with ServiceProvider access
  • Add test for skipping resources excluded from publish
  • Run broader test suite to ensure no regressions - All 37 Orchestrator tests passing
  • Address feedback to simplify implementation by using existing executionContext

Summary of Changes

Code Changes

  1. src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs:
    • Use existing executionContext from constructor instead of creating new execution context
    • Added check to skip resources excluded from publish using IsExcludedFromPublish() extension method

Test Changes

  1. tests/Aspire.Hosting.Tests/Orchestrator/ParameterProcessorTests.cs:
    • Added InitializeParametersAsync_UsesExecutionContextOptions_DoesNotThrow test to verify ServiceProvider access works without throwing InvalidOperationException
    • Added InitializeParametersAsync_SkipsResourcesExcludedFromPublish test to verify resources marked with ManifestPublishingCallbackAnnotation.Ignore are skipped during parameter collection

Test Results

  • All 37 tests in the Orchestrator namespace passing
  • Both new tests passing
  • No build warnings or errors
Original prompt

Summary

Fix two issues in ParameterProcessor.InitializeParametersAsync(DistributedApplicationModel ...):

  1. The method constructs a DistributedApplicationExecutionContext for the publish operation using the obsolete constructor overload that does not supply DistributedApplicationExecutionContextOptions, causing callbacks that depend on ServiceProvider (or other option-derived services) to throw InvalidOperationException: IServiceProvider is not available because execution context was not constructed with DistributedApplicationExecutionContextOptions.
  2. While collecting dependent parameter resources the code processes every resource in the model, including those that are explicitly excluded from publish (annotated with ManifestPublishingCallbackAnnotation.Ignore). These should be skipped using the existing IsExcludedFromPublish() extension method.

Required Changes

  1. In src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs, within CollectDependentParameterResourcesAsync, change the creation of the publish execution context from:
var publishExecutionContext = new DistributedApplicationExecutionContext(DistributedApplicationOperation.Publish);

to

var publishExecutionContext = new DistributedApplicationExecutionContext(
    new DistributedApplicationExecutionContextOptions(DistributedApplicationOperation.Publish));
  1. In the same method, when iterating model.Resources, skip any resource where resource.IsExcludedFromPublish() returns true:
foreach (var resource in model.Resources)
{
    if (resource.IsExcludedFromPublish())
    {
        continue;
    }
    await ProcessResourceDependenciesAsync(...);
}

Tests

Add a new test class (e.g. ParameterProcessorTests) under the appropriate test project (likely tests/Aspire.Hosting.Tests/). Two new tests:

  1. InitializeParametersAsync_UsesExecutionContextOptions_DoesNotThrow:

    • Arrange a model with a custom resource whose environment variable callback accesses context.ExecutionContext.ServiceProvider (which previously would throw with the old constructor).
    • Call InitializeParametersAsync(model) and assert no exception.
  2. InitializeParametersAsync_SkipsResourcesExcludedFromPublish:

    • Arrange a model with a resource annotated with ManifestPublishingCallbackAnnotation.Ignore that (if processed) would add a dependent parameter reference.
    • Verify that after initialization the dependent parameter was not processed/collected (e.g., by asserting the environment callback counter remains zero or the parameter is not in the processed set / received no notifications).

Provide any minimal supporting fakes/mocks needed (e.g., simple implementations or test doubles for ResourceNotificationService, ResourceLoggerService, and IInteractionService). Reuse existing test utilities if available.

Acceptance Criteria

  • Both changes implemented.
  • New tests pass and fail (red) before the fix (verified logically) and pass (green) after the fix.
  • No regression in existing tests.
  • Code adheres to repository coding style.

Notes

  • Ensure necessary using directives are present for extension methods.
  • Keep changes minimal and focused.
*This pull request was created as a result of the following prompt from Copilot chat.* > ### Summary > Fix two issues in `ParameterProcessor.InitializeParametersAsync(DistributedApplicationModel ...)`: > 1. The method constructs a `DistributedApplicationExecutionContext` for the publish operation using the obsolete constructor overload that does not supply `DistributedApplicationExecutionContextOptions`, causing callbacks that depend on `ServiceProvider` (or other option-derived services) to throw `InvalidOperationException: IServiceProvider is not available because execution context was not constructed with DistributedApplicationExecutionContextOptions.` > 2. While collecting dependent parameter resources the code processes every resource in the model, including those that are explicitly excluded from publish (annotated with `ManifestPublishingCallbackAnnotation.Ignore`). These should be skipped using the existing `IsExcludedFromPublish()` extension method. > > ### Required Changes > 1. In `src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs`, within `CollectDependentParameterResourcesAsync`, change the creation of the publish execution context from: > ```csharp > var publishExecutionContext = new DistributedApplicationExecutionContext(DistributedApplicationOperation.Publish); > ``` > to > ```csharp > var publishExecutionContext = new DistributedApplicationExecutionContext( > new DistributedApplicationExecutionContextOptions(DistributedApplicationOperation.Publish)); > ``` > 2. In the same method, when iterating `model.Resources`, skip any resource where `resource.IsExcludedFromPublish()` returns true: > ```csharp > foreach (var resource in model.Resources) > { > if (resource.IsExcludedFromPublish()) > { > continue; > } > await ProcessResourceDependenciesAsync(...); > } > ``` > > ### Tests > Add a new test class (e.g. `ParameterProcessorTests`) under the appropriate test project (likely `tests/Aspire.Hosting.Tests/`). Two new tests: > > 1. `InitializeParametersAsync_UsesExecutionContextOptions_DoesNotThrow`: > - Arrange a model with a custom resource whose environment variable callback accesses `context.ExecutionContext.ServiceProvider` (which previously would throw with the old constructor). > - Call `InitializeParametersAsync(model)` and assert no exception. > > 2. `InitializeParametersAsync_SkipsResourcesExcludedFromPublish`: > - Arrange a model with a resource annotated with `ManifestPublishingCallbackAnnotation.Ignore` that (if processed) would add a dependent parameter reference. > - Verify that after initialization the dependent parameter was not processed/collected (e.g., by asserting the environment callback counter remains zero or the parameter is not in the processed set / received no notifications). > > Provide any minimal supporting fakes/mocks needed (e.g., simple implementations or test doubles for `ResourceNotificationService`, `ResourceLoggerService`, and `IInteractionService`). Reuse existing test utilities if available. > > ### Acceptance Criteria > - Both changes implemented. > - New tests pass and fail (red) before the fix (verified logically) and pass (green) after the fix. > - No regression in existing tests. > - Code adheres to repository coding style. > > ### Notes > - Ensure necessary `using` directives are present for extension methods. > - Keep changes minimal and focused. >

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…ed resources

- Use DistributedApplicationExecutionContextOptions constructor in CollectDependentParameterResourcesAsync
- Copy ServiceProvider from existing execution context when available
- Skip resources excluded from publish using IsExcludedFromPublish()
- Add tests to verify ServiceProvider access and excluded resource filtering

Co-authored-by: davidfowl <[email protected]>
Copilot AI changed the title [WIP] Fix ParameterProcessor publish execution context creation and skip excluded resources Fix ParameterProcessor to use ExecutionContextOptions and skip excluded resources Sep 30, 2025
Copilot AI requested a review from davidfowl September 30, 2025 07:16
Copilot finished work on behalf of davidfowl September 30, 2025 07:16
Use the executionContext from constructor instead of creating a new publishExecutionContext

Co-authored-by: davidfowl <[email protected]>
Copilot finished work on behalf of davidfowl September 30, 2025 07:23
@github-actions
Copy link
Contributor

github-actions bot commented Sep 30, 2025

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11734

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11734"

@davidfowl davidfowl marked this pull request as ready for review September 30, 2025 07:33
@Copilot Copilot AI review requested due to automatic review settings September 30, 2025 07:33
Copy link
Contributor

Copilot AI left a 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 fixes two issues in the ParameterProcessor to properly handle ExecutionContextOptions and skip resources excluded from publish during parameter collection.

  • Fix ParameterProcessor to use existing executionContext instead of creating new one without proper options
  • Add filtering to skip resources marked as excluded from publish using IsExcludedFromPublish() extension method
  • Add comprehensive tests to verify both fixes work correctly

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs Modified CollectDependentParameterResourcesAsync to use existing executionContext and skip excluded resources
tests/Aspire.Hosting.Tests/Orchestrator/ParameterProcessorTests.cs Added two new tests to verify ExecutionContextOptions usage and excluded resource filtering

@davidfowl davidfowl merged commit c1980e2 into main Sep 30, 2025
311 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 9.6 milestone Sep 30, 2025
@davidfowl
Copy link
Member

/backport to release/9.5

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants