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 Microsoft.Extensions.Configuration provider for azure queue streaming #8929

Merged
merged 8 commits into from
Feb 5, 2025

Conversation

tskimmett
Copy link
Contributor

@tskimmett tskimmett commented Mar 31, 2024

I'm interested in using Aspire with an Orleans project I'm working on which uses Azure storage for persistence/streaming, but I saw Orleans doesn't yet support the streaming config sent over by the Aspire apphost. I based my changes off of #8764 and did some pretty minimal manual testing using the orleans sample in the Aspire playground.

Microsoft Reviewers: Open in CodeFlow

{
public void Configure(ISiloBuilder builder, string name, IConfigurationSection configurationSection)
{
builder.AddAzureQueueStreams(name, (OptionsBuilder<AzureQueueOptions> optionsBuilder) =>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if this also needs to support configuring the MessageVisibilityTimeout. I can get that added in if so.

@ReubenBond
Copy link
Member

@benjaminpetit PTAL - what should IConfiguration support look like for Azure Queues

@tskimmett a test would be helpful here. Eg, update some/all of the existing Azure Queue Streaming tests to use this functionality.

@ReubenBond ReubenBond changed the title add Microsoft.Extensions.Configuration provider for azure queue streaming Add Microsoft.Extensions.Configuration provider for azure queue streaming May 15, 2024
@tskimmett
Copy link
Contributor Author

I looked to see if there were relevant tests that could be updated for this and couldn't really tell, but I can take another look at that.

@tskimmett
Copy link
Contributor Author

I've updated AQStreamingTests.cs to make use of the new configuration approach for streaming. Notably, this uncovered the problem that the ProviderBuilder I added was incomplete, in that it was not registered for the client, only the Silo.

@tskimmett
Copy link
Contributor Author

tskimmett commented Jun 23, 2024

As a side note, I ran into some trouble running tests to begin with for a few reasons:

  • I was missing a basic config json file with a connection string
    • I added one on my local machine to get around this. Maybe there are docs explaining local test setup, but I couldn't find any.
  • The tests don't work by default on macOS because the testing framework code is written against storage emulator (deprecated).
    • I got around this by adding a config setting which disables the automatic emulator startup and manually made sure I had Azurite running.

If you'd like, I can open a PR for that last point to slightly improve the dev experience on non-windows platforms, but not sure if you're too concerned about that.

@tskimmett tskimmett force-pushed the config-ext/streaming-aqs branch from dcafdc5 to 45fde99 Compare June 29, 2024 00:57
@tskimmett
Copy link
Contributor Author

@ReubenBond @benjaminpetit Let me know if you have any more suggestions or feel more tests are necessary.

@benjaminpetit
Copy link
Member

I think it's a good start, but it should also be possible to configure MessageVisibilityTimeout.

For tests, I think it would be better to have simple unit tests that just test the config parsing (I believe there is property to get the AccountName on the QueueServiceClient for example).

@tskimmett
Copy link
Contributor Author

Sorry it's been so long, life got in the way, but I am determined to help improve the Aspire integration (albeit selfishly 🙂). Please let me know what else needs to be done to get this over the finish line.

@tskimmett
Copy link
Contributor Author

@benjaminpetit Any chance you can take another look?

@abbottdev
Copy link

any updates on this PR @benjaminpetit / @ReubenBond / @jsedlack?

@ReubenBond ReubenBond force-pushed the config-ext/streaming-aqs branch from 33acad1 to 4a2a6f2 Compare February 5, 2025 18:04
@ReubenBond
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

fix
@ReubenBond ReubenBond merged commit c1a30a3 into dotnet:main Feb 5, 2025
18 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants