Skip to content

Expose IServiceProvider on DistributedApplicationExecutionContext.#4668

Merged
karolz-ms merged 2 commits intomainfrom
mitchdenny/iserviceprovider-on-callbackcontext
Jul 1, 2024
Merged

Expose IServiceProvider on DistributedApplicationExecutionContext.#4668
karolz-ms merged 2 commits intomainfrom
mitchdenny/iserviceprovider-on-callbackcontext

Conversation

@mitchdenny
Copy link
Copy Markdown
Member

@mitchdenny mitchdenny commented Jun 26, 2024

Experimenting with exposing IServiceProvider via DistributedApplicationExecutionContext with minimal API breaking changes.

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Jun 26, 2024
@mitchdenny mitchdenny requested a review from karolz-ms June 26, 2024 14:11
public IServiceProvider ServiceProvider
{
get =>_serviceProvider ?? throw new InvalidOperationException("The ServiceProvider has not been set.");
internal set => _serviceProvider = value;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably won't be able to stay internal because it makes it impossible to unit test easily.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can't be internal, then you can't write tests for it.

Copy link
Copy Markdown
Contributor

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take this

@JamesNK
Copy link
Copy Markdown
Member

JamesNK commented Jun 27, 2024

I think you need another constructor. It could take both arguments: DistributedApplicationOperation and IServiceProvider. But that doesn't scale.

Or you could create an options type, e.g. DistributedApplicationExecutionContextOptions that has properties to set on it. A new constructor that takes the options copies the values to the new DistributedApplicationExecutionContext.

@mitchdenny mitchdenny requested review from JamesNK and karolz-ms July 1, 2024 12:44
@mitchdenny
Copy link
Copy Markdown
Member Author

@JamesNK pretty much went with your proposal here.

@mitchdenny mitchdenny marked this pull request as ready for review July 1, 2024 12:45
@mitchdenny mitchdenny self-assigned this Jul 1, 2024
@mitchdenny mitchdenny added this to the 8.1 milestone Jul 1, 2024
@dotnet-bot
Copy link
Copy Markdown
Contributor

‼️ Found issues ‼️

Project Coverage Type Expected Actual
Microsoft.Extensions.ServiceDiscovery Branch 81 79.31 🔻
Microsoft.Extensions.ServiceDiscovery Line 81 79.01 🔻

Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=726046&view=codecoverage-tab

@karolz-ms karolz-ms merged commit 856af32 into main Jul 1, 2024
@karolz-ms karolz-ms deleted the mitchdenny/iserviceprovider-on-callbackcontext branch July 1, 2024 21:48
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants