Skip to content

Fix persistent lifetime support for Azure Event Hubs emulator#7467

Merged
davidfowl merged 4 commits intomainfrom
sebros/ehpersist
Feb 7, 2025
Merged

Fix persistent lifetime support for Azure Event Hubs emulator#7467
davidfowl merged 4 commits intomainfrom
sebros/ehpersist

Conversation

@sebastienros
Copy link
Copy Markdown
Contributor

Description

Using the new Aspire Store and applying the same patterns for Config.json as with Azure Service Bus.

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?

Comment thread src/Aspire.Hosting.Azure.EventHubs/AzureEventHubsExtensions.cs Outdated
Co-authored-by: David Fowler <davidfowl@gmail.com>
@davidfowl
Copy link
Copy Markdown
Contributor

Do these tests properly verify that the container is persistent? I expected to see something more functional testy

var storageResource = builder.ApplicationBuilder
.AddAzureStorage($"{builder.Resource.Name}-storage")
.WithParentRelationship(builder.Resource)
.RunAsEmulator();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it possible to keep the RunAsEmulator() call close to where the storage resource is declared? It feels really far away now.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe what might help is moving the Health Check code to the bottom of the method - like it is for ServiceBus.

Copy link
Copy Markdown
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Change looks good. I had a nit.

I also agree that we should have a test that verifies this (somehow).

@davidfowl
Copy link
Copy Markdown
Contributor

Conflict 😄

@davidfowl davidfowl merged commit 1100e7e into main Feb 7, 2025
@davidfowl davidfowl deleted the sebros/ehpersist branch February 7, 2025 23:53
@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 10, 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.

3 participants