Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Automated] Update API Surface Area #7374

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 2, 2025

Auto-generated update to the API surface to compare current surface vs latest release. This should only be merged once this surface area ships in a new release.

@joperezr joperezr added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Feb 2, 2025
@joperezr
Copy link
Member

joperezr commented Feb 2, 2025

FYI: @davidfowl @eerhardt

Choose a reason for hiding this comment

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

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

Files not reviewed (15)
  • src/Components/Aspire.Azure.AI.OpenAI/api/Aspire.Azure.AI.OpenAI.cs: Evaluated as low risk
  • src/Components/Aspire.MongoDB.Driver.v3/api/Aspire.MongoDB.Driver.cs: Evaluated as low risk
  • src/Aspire.Hosting.Azure.EventHubs/api/Aspire.Hosting.Azure.EventHubs.cs: Evaluated as low risk
  • src/Aspire.Hosting.Azure.PostgreSQL/api/Aspire.Hosting.Azure.PostgreSQL.cs: Evaluated as low risk
  • src/Aspire.Hosting.Azure.Redis/api/Aspire.Hosting.Azure.Redis.cs: Evaluated as low risk
  • src/Aspire.Hosting.Azure.ServiceBus/api/Aspire.Hosting.Azure.ServiceBus.cs: Evaluated as low risk
  • src/Aspire.Hosting.Azure.SignalR/api/Aspire.Hosting.Azure.SignalR.cs: Evaluated as low risk
  • src/Components/Aspire.NATS.Net/api/Aspire.NATS.Net.cs: Evaluated as low risk
  • src/Components/Aspire.OpenAI/api/Aspire.OpenAI.cs: Evaluated as low risk
  • src/Aspire.Hosting.Azure.CosmosDB/api/Aspire.Hosting.Azure.CosmosDB.cs: Evaluated as low risk
  • src/Components/Aspire.Confluent.Kafka/api/Aspire.Confluent.Kafka.cs: Evaluated as low risk
  • src/Aspire.Hosting.Azure.AppContainers/api/Aspire.Hosting.Azure.AppContainers.cs: Evaluated as low risk
  • src/Aspire.Hosting/api/Aspire.Hosting.cs: Evaluated as low risk
  • src/Aspire.Hosting.Keycloak/api/Aspire.Hosting.Keycloak.cs: Evaluated as low risk
  • src/Aspire.Hosting.Redis/api/Aspire.Hosting.Redis.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

src/Aspire.Hosting.Nats/api/Aspire.Hosting.Nats.cs:32

  • The userName and password parameters are optional but are not being checked for null values when being set in the NatsServerResource constructor. This could lead to potential null reference exceptions if these parameters are accessed without null checks elsewhere in the code. Consider adding null checks or handling these parameters appropriately.
public NatsServerResource(string name, ParameterResource? userName, ParameterResource? password) : base(default!, default) { }
@github-actions github-actions bot force-pushed the update-api-diffs branch 2 times, most recently from f206793 to 7fa712f Compare February 5, 2025 16:16
@github-actions github-actions bot force-pushed the update-api-diffs branch 2 times, most recently from aaa45ff to 483f443 Compare February 7, 2025 16:15
Comment on lines 1544 to 1557
public enum WaitBehavior
{
WaitOnDependencyFailure = 0,
StopOnDependencyFailure = 1
}
Copy link
Member

Choose a reason for hiding this comment

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

@JamesNK You had thoughts on these names right?

@DamianEdwards @eerhardt @ReubenBond ?

Copy link
Member

Choose a reason for hiding this comment

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

It isn't clear to me if StopOnDependencyFailure means:

  • it will stop waiting if a dependency it's waiting on fails to start
  • whether it throws an exception if the stop condition is hit
  • it will stop the resource if the dependency fails after everything is started

Copy link
Member

@davidfowl davidfowl Feb 8, 2025

Choose a reason for hiding this comment

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

FailOnDependencyFailure
StopOnDependencyTerminal

Copy link
Member

Choose a reason for hiding this comment

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

I'd assume the behaviors we want to be able to select are:

  • Keep waiting forever until the dependency has started
  • Cancel waiting and throw an exception if the dependency fails to start

Do we think there's a need for being able to stop waiting and just start anyway if a dependency fails to start?

Copy link
Member

Choose a reason for hiding this comment

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

Unclear if that last one is needed but it could be an option we add later.

Copy link
Member

Choose a reason for hiding this comment

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

That's not bad.

Copy link
Member

Choose a reason for hiding this comment

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

@mitchdenny can this thread be resolved?

Copy link
Member

Choose a reason for hiding this comment

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

The new names are:

public enum WaitBehavior
{
    /// <summary>
    /// If the resource is unavailable, continue waiting.
    /// </summary>
    WaitOnResourceUnavailable,

    /// <summary>
    /// If the resource is unavailable, stop waiting.
    /// </summary>
    StopOnResourceUnavailable
}

Which is different than @ReubenBond's suggestion above. Do we have an official consensus on what names we want here?

Copy link
Member

Choose a reason for hiding this comment

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

I adopted the names from a branch that David had. In terms of enumeration name, I think WaitBehavior is fine. I do like Wait and Throw as enumeration values and am happy to make that change.

Copy link
Member

Choose a reason for hiding this comment

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

I'm probably bikeshedding at this point (but hey, it's API review), but WaitBehavior.Wait and WaitBehavior.Throw kinda doesn't make sense. If it is WaitBehavior and the behavior is Throw, that isn't really "waiting".

@github-actions github-actions bot force-pushed the update-api-diffs branch 3 times, most recently from 34dcaf9 to d4a07e3 Compare February 14, 2025 16:15
@github-actions github-actions bot force-pushed the update-api-diffs branch 3 times, most recently from e980bc9 to 7117075 Compare February 17, 2025 16:15
@eerhardt eerhardt mentioned this pull request Feb 18, 2025
18 tasks
github-actions bot pushed a commit that referenced this pull request Feb 19, 2025
danmoseley pushed a commit that referenced this pull request Feb 19, 2025
@@ -147,6 +153,12 @@ public ApplicationModel.IResourceBuilder<T> CreateResourceBuilder<T>(T resource)
where T : ApplicationModel.IResource { throw null; }
}

public static partial class DistributedApplicationBuilderExtensions
{
public static ApplicationModel.IResourceBuilder<T> CreateResourceBuilder<T>(this IDistributedApplicationBuilder builder, string name)
Copy link
Member

Choose a reason for hiding this comment

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

I know there was a little bit of discussion in #3022 (comment), but I kind of think Get makes more sense here. Yes it "creates a resource builder", but builders are ephemeral. I think it is a bit confusing to call this "Create" when logically you are just getting a builder wrapped around the existing resource.

Thoughts @davidfowl @mitchdenny @ReubenBond @DamianEdwards ?

@Eerhard
Copy link

Eerhard commented Feb 20, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-meta NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.