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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tskimmett
Copy link

@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
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
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
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
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
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
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
Author

@benjaminpetit Any chance you can take another look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants