Remove null/nullable parameter from DistributedApplicationExecutionContext#8533
Remove null/nullable parameter from DistributedApplicationExecutionContext#8533sebastienros merged 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/Aspire.Hosting/DistributedApplicationExecutionContextOptions.cs:24
- The removal of the default null for publisherName enforces non-null values, but there is no validation to ensure that a valid publisherName is provided when operation is set to Publish. Consider adding an argument check and throw an exception if publisherName is null or empty to prevent misconfiguration.
public DistributedApplicationExecutionContextOptions(DistributedApplicationOperation operation, string? publisherName)
src/Aspire.Hosting/DistributedApplicationExecutionContext.cs:39
- Changing PublisherName to non-nullable requires ensuring that it is always initialized properly. Verify that all code paths assign a valid non-null value to PublisherName to avoid potential runtime exceptions.
public string PublisherName { get; set; }
| /// The name of the publisher that is being used if <see cref="Operation"/> is set to <see cref="DistributedApplicationOperation.Publish"/>. | ||
| /// </summary> | ||
| public string? PublisherName { get; set; } | ||
| public string PublisherName { get; set; } |
There was a problem hiding this comment.
@mitchdenny This one was not sure but since there is no way to pass a null values from the ctors. Unless we expect someone to set the value afterwards.
There was a problem hiding this comment.
If you don't pass a publisher name then it is null - which is valid if you are not in publisher mode.
There was a problem hiding this comment.
public DistributedApplicationExecutionContext(DistributedApplicationOperation operation) : this(operation, "manifest")won't be null, right?
mitchdenny
left a comment
There was a problem hiding this comment.
Looks good, just make PublisherName nullable again.
|
/backport to release/9.2 |
|
Started backporting to release/9.2: https://github.com/dotnet/aspire/actions/runs/14254488342 |
Description
1- There is an overload that doesn't take the parameter.
2- Property can't be created with a
nullPublisherName.Contributes to #7811
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):