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

Add EndpointAnnotation directly in AddProject #2579

Merged
merged 10 commits into from
Mar 5, 2024
Merged

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Mar 2, 2024

Closes #2472

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Mar 2, 2024
@davidfowl
Copy link
Member

davidfowl commented Mar 2, 2024

you can't exclude launch settings later in your code since it's now done inline in AddProject

This seems like a good tradeoff.

specifying a specific launch settings to use via WithLaunchProfile doesn't work (add another param to AddProject?!)

I think this should be the same pattern yes, another parameter. Though I think maybe just a single argument launchProfile.

previous WithEndpoint calls that added an endpoint might not work the same now

Explain?

@davidfowl
Copy link
Member

Let's move forward with this approach.

@davidfowl
Copy link
Member

davidfowl commented Mar 2, 2024

To clarify, the change is to make launchProfile processing part of the call to AddProject.

// Use the default launch profile (first one)
builder.AddProject<Projects.Api>("api");

// Pick a specific launch profile
builder.AddProject<Projects.Api>("api", launchProfile: "http");

// No launch profile
builder.AddProject<Projects.Api>("api", launchProfile: null);

cc @DamianEdwards

@DamianEdwards
Copy link
Member

Assuming the default behavior is maintained this looks good

@BrennanConroy
Copy link
Member Author

So we add a new overload with launchProfile as a required param on AddProject and a null value is equal to ExcludeLaunchProfile? And we keep the current overloads which grab the default launch profile?

@davidfowl
Copy link
Member

davidfowl commented Mar 3, 2024

So we add a new overload with launchProfile as a required param on AddProject and a null value is equal to ExcludeLaunchProfile? And we keep the current overloads which grab the default launch profile?

Yep and we deprecate WithLaunchProfile and ExcludeLaunchProfile the other overloads. I updated #2579 (comment)

@BrennanConroy BrennanConroy marked this pull request as ready for review March 4, 2024 21:33
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 overall. One question for clarification and suggest a timeframe in the obsolete messages. I'm assuming you'll address David's feedback too but overall this looks good. Send it!

@BrennanConroy BrennanConroy merged commit 1d868e8 into main Mar 5, 2024
8 checks passed
@BrennanConroy BrennanConroy deleted the brecon/inline branch March 5, 2024 02:53
DamianEdwards added a commit to dotnet/aspire-samples that referenced this pull request Mar 6, 2024
DamianEdwards added a commit to dotnet/aspire-samples that referenced this pull request Apr 16, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-codeflow for labeling automated codeflow. intentionally a different color!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxyless + LaunchSettings could be improved
5 participants