-
Notifications
You must be signed in to change notification settings - Fork 519
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 support for SignalR emulator #6793
base: main
Are you sure you want to change the base?
Conversation
src/Aspire.Hosting.Azure.SignalR/Aspire.Hosting.Azure.SignalR.csproj
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.SignalR/Aspire.Hosting.Azure.SignalR.csproj
Outdated
Show resolved
Hide resolved
@dotnet-policy-service agree company="Microsoft" |
src/Aspire.Hosting.Azure.SignalR/Aspire.Hosting.Azure.SignalR.csproj
Outdated
Show resolved
Hide resolved
playground/signalr/serverless/SignalRServerless.AppHost/README.md
Outdated
Show resolved
Hide resolved
playground/signalr/serverless/SignalRServerless.AppHost/Program.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Liangying.Wei <[email protected]>
…/cqnguy23/aspire into add-support-for-signalr-emulator
Hi @eerhardt @davidfowl Azure Function requires an app key |
We were just talking with @fabiocav yesterday about a similar scenario for HttpTriggers. .NET Aspire doesn't have support inbox today, but are discussing how it can be supported. |
Hi @eerhardt, could you help add this package |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this to the existing Azure Functions playground sample instead of creating a new one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or collapse it into the existing SignalR playground app? I'm not sure we need 2 in the repo.
Also, is the "Functions" part of this superfluous? This PR is for the SignalR emulator. It might be overkill trying to define/decide how Functions and Azure SignalR work together in this PR. It will probably distract/delay getting the emulator support in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe in one SignalR playground but creating 2 SignalR resources, one for serverless one for default mode. For Azure SignalR emulator, it works for serverless scenarios only, which mostly works together with Azure Function. I think it would be a great serverless sample if Function works. But yeah if Function SignalRTrigger is not yet ready, surely we could have a simpler sample for the serverless scenario to let the emulator support ready first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But yeah if Function SignalRTrigger is not yet ready, surely we could have a simpler sample for the serverless scenario to let the emulator support ready first.
I'm inclined to pull out the Functions-specific changes from this PR and just leave the SignalR support. Supporting triggers for Functions in Aspire requires some additional work (see #6414). It would make review easier to scope this to just the SignalR-emulator specific changes and move the Functions changes to a separate PR.
Is there a way to validate the emulator locally without implementing a Functions app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captainsafia Sure. I can just spin up a simple web app to invoke API from emulator without using Functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@captainsafia removed the Functions out of the scope and used a web application to validate the emulator instead. Could you check again?
playground/signalr/serverless/SignalRServerless.Functions/Program.cs
Outdated
Show resolved
Hide resolved
playground/signalr/serverless/SignalRServerless.Functions/Function.cs
Outdated
Show resolved
Hide resolved
_logger.LogInformation($"{invocationContext.ConnectionId} has disconnected"); | ||
} | ||
|
||
public class NewConnection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Consider using a record type here.
src/Aspire.Hosting.Azure.SignalR/SignalREmulatorContainerImageTags.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting.Azure.SignalR/AzureSignalREmulatorResource.cs
Outdated
Show resolved
Hide resolved
/// Configures an Azure SignalR resource to be emulated. This resource requires an <see cref="AzureSignalRResource"/> to be added to the application model. Please note that the resource will be emulated in <b>Serverless mode</b>. | ||
/// </summary> | ||
/// <remarks> | ||
/// This version of the package defaults to the <inheritdoc cref="SignalREmulatorContainerImageTags.Tag"/> tag of the <inheritdoc cref="SignalREmulatorContainerImageTags.Registry"/>/<inheritdoc cref="SignalREmulatorContainerImageTags.Image"/> container image. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - @sebastienros - this will need to be fixed with #6747
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could I just fix it in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sebastienros do we know what the right fix is?
using Microsoft.Extensions.Diagnostics.HealthChecks; | ||
|
||
namespace Aspire.Hosting.Azure.SignalR; | ||
internal sealed class AzureSignalRHealthCheck : IHealthCheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on contributing this health check to https://github.com/xabaril/AspNetCore.Diagnostics.HealthChecks/? It doesn't need to block this PR, but the idea is that having a rich library of health checks (outside of Aspire) helps the whole community.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Azure SignalR has it's own SDK with HealthCheck embeded, this class might be only useful for this emulator scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good. I pulled it locally, and it all worked for me.
Thanks for the contribution!
playground/signalr/SignalRServerlessWeb/Properties/launchSettings.json
Outdated
Show resolved
Hide resolved
playground/signalr/SignalRServerlessWeb/Properties/launchSettings.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @cqnguy23!
However, it looks like the new test is failing:
Can you take a look? |
Can you help me re-run the tests? I encountered this error a few times and found that re-running the tests would solve it. |
We need our tests to be consistent and stable. If there is a problem, we should add retries/waits to the test code. |
Description
Fixes #6676
Checklist
<remarks />
and<code />
elements on your triple slash comments?breaking-change
template):doc-idea
template):Microsoft Reviewers: Open in CodeFlow
Addresses issue #6676