Skip to content

Conversation

eerhardt
Copy link
Member

Since, by default, publishing a .NET Asipre app will create a new resource group, not specifying a resource group won't work - since the current resource group hasn't been created yet. Because of this, require callers to explicitly specify the resource group to use. They can still say null to mean "use the current" if that's really what they want.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
  • Did you add public API?
    • No
  • Does the change make any security assumptions or guarantees?
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Is this introducing a breaking change?

Since, by default, publishing a .NET Asipre app will create a new resource group, not specifying a resource group won't work - since the current resource group hasn't been created yet. Because of this, require callers to explicitly specify the resource group to use. They can still say `null` to mean "use the current" if that's really what they want.
@eerhardt eerhardt requested review from Copilot, captainsafia, davidfowl and mitchdenny and removed request for Copilot February 21, 2025 21:15
@@ -74,7 +74,7 @@ public static IResourceBuilder<T> RunAsExisting<T>(this IResourceBuilder<T> buil
/// <param name="nameParameter">The name of the existing resource.</param>
/// <param name="resourceGroupParameter">The name of the existing resource group, or <see langword="null"/> to use the current resource group.</param>
/// <returns>The resource builder with the existing resource annotation added.</returns>
public static IResourceBuilder<T> PublishAsExisting<T>(this IResourceBuilder<T> builder, IResourceBuilder<ParameterResource> nameParameter, IResourceBuilder<ParameterResource>? resourceGroupParameter = null)
public static IResourceBuilder<T> PublishAsExisting<T>(this IResourceBuilder<T> builder, IResourceBuilder<ParameterResource> nameParameter, IResourceBuilder<ParameterResource>? resourceGroupParameter)
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to make the same change for the RunAsExisting methods? If someone specifies a Azure:ResourceGroup in their user secrets (and not a Prefix), the resource group can be existing. So it is more likely to work if you don't specify a resource group.

Copy link
Member Author

Choose a reason for hiding this comment

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

Given @mitchdenny's comment #7733 (comment), I think we should make the same change to RunAsExisting. It gives us room to take a single string to mean the whole resourceID in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed

@eerhardt
Copy link
Member Author

/backport to release/9.1

Copy link
Contributor

Started backporting to release/9.1: https://github.com/dotnet/aspire/actions/runs/13465391305

@mitchdenny
Copy link
Member

We can probably let this go in as is but have we considered using a fully qualified resource ID as well?

Given that scenario how would we disambiguate the APIs (since both would just be parameters).

@mitchdenny
Copy link
Member

In case it wasn't obvious the reason for my question was around using existing resources that might be in a different sub which is actually pretty common.

@eerhardt
Copy link
Member Author

We can probably let this go in as is but have we considered using a fully qualified resource ID as well?

I believe we talked about that orginally, but went with "resourceName" and "resourceGroup" separately.

Given that scenario how would we disambiguate the APIs (since both would just be parameters).

This change actually makes it easier becuase in the future we can add an overload that takes a single string which is the full resource ID. If we are going to do that, we should also make this same change to the RunAsExisting methods, so we can have the single string version in the future.

@eerhardt
Copy link
Member Author

/backport to release/9.1

Copy link
Contributor

Started backporting to release/9.1: https://github.com/dotnet/aspire/actions/runs/13465537766

@mitchdenny
Copy link
Member

This change actually makes it easier becuase in the future we can add an overload that takes a single string which is the full resource ID. If we are going to do that, we should also make this same change to the RunAsExisting methods, so we can have the single string version in the future.

Very good point.

@eerhardt eerhardt added this to the 9.1 milestone Feb 21, 2025
@eerhardt eerhardt merged commit 78adb1f into dotnet:main Feb 21, 2025
69 of 70 checks passed
@eerhardt eerhardt deleted the ExistingResourceGroup branch February 21, 2025 23:15
@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 10, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 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.

3 participants