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.

Sign up for GitHub

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

Jump to bottom

Fix Azure ServiceBus persistent container support #7136

Merged
merged 23 commits into from
Feb 5, 2025
Merged

Fix Azure ServiceBus persistent container support #7136

merged 23 commits into from
Feb 5, 2025

Conversation

sebastienros
Copy link
Member

@sebastienros sebastienros commented Jan 17, 2025 •
edited
Loading

Description

Fixes #7071

Issues identified:

  • the lifetime is not forwarded to the dependent container (sqledge)
  • the password was new on every start, invalidating reusability
  • the volume was using a new folder on every start, invalidating reusability

Now the password and the folder are stored in user secrets and reused.

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?

This was referenced Jan 18, 2025
Open
Open
@davidfowl
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl
Copy link
Member

AddAzureServiceBusWithEmulatorGetsExpectedPort - actual failing test

@sebastienros sebastienros marked this pull request as ready for review January 22, 2025 20:43
@sebastienros sebastienros requested a review from eerhardt January 23, 2025 00:12
@danmoseley danmoseley requested a review from Copilot January 27, 2025 19:23
eerhardt
eerhardt reviewed Jan 29, 2025
View reviewed changes
src/Shared/AspireStore.cs Outdated Show resolved Hide resolved
sebastienros and others added 5 commits January 29, 2025 12:42
davidfowl
davidfowl reviewed Feb 3, 2025
View reviewed changes
src/Aspire.Hosting.Azure.ServiceBus/AzureServiceBusExtensions.cs
Comment on lines +231 to +234
if (builder.Resource.IsEmulator)
{
throw new InvalidOperationException("The Azure Service Bus resource is already configured to run as an emulator.");
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this override the behavior? Why would it throw?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without this there is an exception when we call RunAsEmulator twice (I did this mistake at some point):

Unhandled exception. Aspire.Hosting.DistributedApplicationException: Endpoint with name 'emulator' already exists. Endpoint name may not have been explicitly specified and was derived automatically from scheme argument (e.g. 'http', 'https', or 'tcp'). Multiple calls to WithEndpoint (and related methods) may result in a conflict if name argument is not specified. Each endpoint must have a unique name. For more information on networking in .NET Aspire see: https://aka.ms/dotnet/aspire/networking

It took me a while to realize the problem.

Copy link
Member

Choose a reason for hiding this comment

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

I think we'll want to handle this differently but this is ok for now.

davidfowl
davidfowl reviewed Feb 4, 2025
View reviewed changes
src/Aspire.Hosting/AspireStore.cs Outdated
/// </summary>
/// <param name="builder">The <see cref="IDistributedApplicationBuilder"/>.</param>
/// <returns>A new instance of <see cref="AspireStore"/>.</returns>
public static AspireStore Create(IDistributedApplicationBuilder builder)
Copy link
Member

@davidfowl davidfowl Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is a weird API. Can we make this internal and instead expose something on the builder? (ext method). I guess we can do that later in review.

eerhardt
eerhardt reviewed Feb 4, 2025
View reviewed changes
src/Aspire.Hosting/AspireStoreExtensions.cs
/// </summary>
/// <param name="builder">The <see cref="IDistributedApplicationBuilder"/>.</param>
/// <returns>The <see cref="IAspireStore"/>.</returns>
public static IAspireStore CreateStore(this IDistributedApplicationBuilder builder)
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 we can iterate on this API, so we don't have to block on this, but I think this is going to clutter up normal dev's intellisense. Normal apphost devs don't need to use this API, so I don't think it should be top-level like "AddContainer" or "AddPostgres", etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok to move to Azure.Hosting.ApplicationModel instead?

eerhardt
eerhardt approved these changes Feb 5, 2025
View reviewed changes
src/Aspire.Hosting/IAspireStore.cs
/// </summary>
/// <remarks>
/// The store is created in the ./obj folder of the Application Host.
/// If the ASPIRE_STORE_DIR environment variable is set this will be used instead.
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
/// If the ASPIRE_STORE_DIR environment variable is set this will be used instead.
/// If the ASPIRE__STORE__PATH environment variable is set this will be used instead.

src/Aspire.Hosting/IAspireStore.cs
/// If the ASPIRE_STORE_DIR environment variable is set this will be used instead.
///
/// The store is specific to a <see cref="IDistributedApplicationBuilder"/> instance such that each application can't
/// conflict with others. A <em>.aspire</em> prefix is also used to ensure that the folder can be delete without impacting
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
/// conflict with others. A <em>.aspire</em> prefix is also used to ensure that the folder can be delete without impacting
/// conflict with others. A <em>.aspire</em> prefix is also used to ensure that the folder can be deleted without impacting

@sebastienros sebastienros merged commit 4e9896b into main Feb 5, 2025
65 checks passed
@sebastienros sebastienros deleted the sebros/sbpersist branch February 5, 2025 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidfowl davidfowl davidfowl left review comments

@eerhardt eerhardt eerhardt approved these changes

Copilot code review Copilot Awaiting requested review from Copilot

@mitchdenny mitchdenny Awaiting requested review from mitchdenny mitchdenny is a code owner

@ReubenBond ReubenBond Awaiting requested review from ReubenBond

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The service bus emulator is never persistent when marked as such
3 participants
@sebastienros @davidfowl @eerhardt