Add Logger property to ExecuteCommandContext#15678
Add Logger property to ExecuteCommandContext#15678tranhoangtu-it wants to merge 2 commits intomicrosoft:mainfrom
Conversation
tranhoangtu-it
requested review from
JamesNK,
danegsta,
davidfowl,
karolz-ms and
mitchdenny
as code owners
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 15678Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 15678" |
There was a problem hiding this comment.
Pull request overview
Adds a Logger property to ExecuteCommandContext so resource command handlers can log directly to the resource’s log stream without resolving a logger from IServiceProvider.
Changes:
- Added
ILogger LoggertoExecuteCommandContext(public API). - Passed the existing resource logger into
ExecuteCommandContextfromResourceCommandService. - Updated the public API surface file to include the new property.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Aspire.Hosting/ApplicationModel/ResourceCommandAnnotation.cs | Extends ExecuteCommandContext with a Logger property. |
| src/Aspire.Hosting/ApplicationModel/ResourceCommandService.cs | Populates ExecuteCommandContext.Logger using the existing ResourceLoggerService logger. |
| src/Aspire.Hosting/api/Aspire.Hosting.cs | Updates API surface to include the new Logger property. |
| public required CancellationToken CancellationToken { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// The logger for the resource. | ||
| /// </summary> | ||
| public required ILogger Logger { get; init; } |
There was a problem hiding this comment.
Adding required ILogger Logger makes ExecuteCommandContext source-breaking for any consumers that construct it (e.g., tests) and is inconsistent with other callback contexts that default Logger to NullLogger.Instance. Consider making Logger non-required and initializing it to NullLogger.Instance (and still setting it in ResourceCommandService) so the property is always non-null without forcing callers to provide it.
There was a problem hiding this comment.
I'm fine with this. It's only a source breaking change and I doubt many people create this type themselves.
Simple enough to fix by assigning a NullLogger.Instance in tests on upgrade to fix.
The alternative is not making it required and assigning it = null!;. Trades a build time error for runtime errors.
| public required CancellationToken CancellationToken { get; init; } | ||
|
|
||
| /// <summary> | ||
| /// The logger for the resource. | ||
| /// </summary> | ||
| public required ILogger Logger { get; init; } |
There was a problem hiding this comment.
I'm fine with this. It's only a source breaking change and I doubt many people create this type themselves.
Simple enough to fix by assigning a NullLogger.Instance in tests on upgrade to fix.
The alternative is not making it required and assigning it = null!;. Trades a build time error for runtime errors.
|
@sebastienros What's required when adding new API to exported type? Is something special required for ILogger? |
Fixes #15582
Adds a
Loggerproperty toExecuteCommandContextso command handlers can log directly to the resource's log stream without resolving it fromServiceProvider.The logger instance comes from
ResourceLoggerService.GetLogger(resourceId), which is already obtained inResourceCommandService— this change just passes it through to the context.Changes
ResourceCommandAnnotation.cs— Addedrequired ILogger Logger { get; init; }toExecuteCommandContextResourceCommandService.cs— Passes existingloggerwhen constructing the contextAspire.Hosting.cs— Updated API surface file