Skip to content

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Apr 3, 2025

Description

1- There is an overload that doesn't take the parameter.
2- Property can't be created with a null PublisherName.

Contributes to #7811

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@Copilot Copilot AI review requested due to automatic review settings April 3, 2025 22:06
@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Apr 3, 2025
Copy link
Contributor

@Copilot 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.

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; }

@sebastienros sebastienros changed the title Sebros/nullpublisherr Remove null/nullable parameter from DistributedApplicationExecutionContext Apr 3, 2025
@sebastienros sebastienros requested a review from mitchdenny April 3, 2025 22:07
@@ -36,7 +36,7 @@ public DistributedApplicationExecutionContext(DistributedApplicationOperation op
/// <summary>
/// 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; }
Copy link
Member Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't pass a publisher name then it is null - which is valid if you are not in publisher mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

public DistributedApplicationExecutionContext(DistributedApplicationOperation operation) : this(operation, "manifest")

won't be null, right?

Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

Looks good, just make PublisherName nullable again.

@sebastienros
Copy link
Member Author

/backport to release/9.2

Copy link
Contributor

github-actions bot commented Apr 3, 2025

Started backporting to release/9.2: https://github.com/dotnet/aspire/actions/runs/14254488342

@sebastienros sebastienros merged commit 96651ac into main Apr 3, 2025
174 checks passed
@sebastienros sebastienros deleted the sebros/nullpublisherr branch April 3, 2025 23:12
@github-actions github-actions bot locked and limited conversation to collaborators May 4, 2025
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.

2 participants