Skip to content

Conversation

eerhardt
Copy link
Member

Description

This allows them to be referenceable and waitable in the future.

Also rename the classes and put them in the appropriate namespace.

Probably easiest to review each commit separately.

Contributes to #7407

Checklist

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

…asses.

Also change the namespace of public types to be consistent with other Azure resources.
This allows them to be referenceable and waitable in the future.

Also rename the classes and put them in the appropriate namespace.

Contributes to dotnet#7407
.AddServiceBusSubscription("subscription1");
sb.ConfigureInfrastructure(infrastructure =>
{
var subscription = infrastructure.GetProvisionableResources().OfType<ServiceBusSubscription>().Single(q => q.BicepIdentifier == "subscription1");
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this PR, but we should consider combining this into a single method call:

var subscription = infrastructure.GetProvisionableResource<ServiceBusSubscription>("subscription1");

Perhaps with a sibling for other cases:

var subscription = infrastructure.GetProvisionableResource<ServiceBusSubscription>(q => q.BicepIdentifier == "subscription1");

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we want the Azure.Provisioning APIs to supply these kidns of helpers.

cc @tg-msft

/// Adds an Azure Service Bus Subscription resource to the application model.
/// </summary>
/// <param name="builder">The Azure Service Bus Topic resource builder.</param>
/// <param name="name">The name of the subscription.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <param name="name">The name of the subscription.</param>
/// <param name="name">The name of the subscription resource.</param>

/// Adds an Azure Service Bus Queue resource to the application model. This resource requires an <see cref="AzureServiceBusResource"/> to be added to the application model.
/// </summary>
/// <param name="builder">The Azure Service Bus resource builder.</param>
/// <param name="name">The name of the queue.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <param name="name">The name of the queue.</param>
/// <param name="name">The name of the queue resource.</param>

/// Adds an Azure Service Bus Topic resource to the application model. This resource requires an <see cref="AzureServiceBusResource"/> to be added to the application model.
/// </summary>
/// <param name="builder">The Azure Service Bus resource builder.</param>
/// <param name="name">The name of the topic.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <param name="name">The name of the topic.</param>
/// <param name="name">The name of the topic resource.</param>

/// Adds an Azure Service Bus Topic resource to the application model. This resource requires an <see cref="AzureServiceBusResource"/> to be added to the application model.
/// </summary>
/// <param name="builder">The Azure Service Bus resource builder.</param>
/// <param name="name">The name of the topic.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <param name="name">The name of the topic.</param>
/// <param name="name">The name of the topic resource.</param>

Copy link
Member

Choose a reason for hiding this comment

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

That sounds more ambiguous to me, as though the resource representing the topic could have a different name to the topic itself. Calling it the "name of the topic" matches the Service Bus API docs eg here: https://learn.microsoft.com/en-us/dotnet/api/azure.messaging.servicebus.servicebussender.-ctor?view=azure-dotnet#azure-messaging-servicebus-servicebussender-ctor(azure-messaging-servicebus-servicebusclient-system-string-azure-messaging-servicebus-servicebussenderoptions)

Copy link
Member Author

Choose a reason for hiding this comment

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

though the resource representing the topic could have a different name to the topic itself.

It 100% can be a different name. The issue with child resources (well, Aspire resources in general) is that the resource name needs to be unique for the whole application. So if you wanted a PostgreSQL database to be named "projects" and your Redis server to be named "projects", you can't because the names collide. To solve this, child resources (like SQL databases) have room for 2 names:

  • The resource name
  • The physical database name (which defaults to the resource name if it isn't provided)

So if you also wanted a ServiceBus topic to be named "projects", you would need to make the resource name unique, and supply the name "projects" via the topicName parameter:

sb.AddServiceBusTopic("projects-topic", topicName: "projects");

/// Adds an Azure Service Bus Topic resource to the application model.
/// </summary>
/// <param name="builder">The Azure Service Bus resource builder.</param>
/// <param name="name">The name of the topic.</param>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// <param name="name">The name of the topic.</param>
/// <param name="name">The name of the topic resource.</param>

@davidfowl davidfowl merged commit 3e1e5ac into dotnet:main Feb 11, 2025
70 checks passed
@davidfowl
Copy link
Member

@eerhardt send a follow up addressing the comments

@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages 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-integrations Issues pertaining to Aspire Integrations packages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants