-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
azurerm_signalr_service
: Expose upstream_endpoint
#10459
azurerm_signalr_service
: Expose upstream_endpoint
#10459
Conversation
@Brunhil - could you fix up the build errors so we can run the tests and get this merged? thanks! |
Thanks for taking a peek @katbyte ! I was planning on this PR to have the library upgrade for SignalR and the new resource. Did you want me to pull out the library upgrade into a separate PR? |
azurerm_signalr_service_upstream
azurerm_signalr_service
: Expose upstream_setting
78f4706
to
404b809
Compare
Test is failing with the following,
|
Turns out the response we get back isn't always in the order that we submit it. Going to drop the acceptance tests assertion to just verify we have four entries. Test now passes and should be good for review! === RUN TestAccSignalRService_upstreamSetting |
azurerm_signalr_service
: Expose upstream_setting
azurerm_signalr_service
: Expose upstream_setting
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.
Thanks @Brunhil - have some questions about schema but once those are addressed this should be good to merge
@@ -55,6 +62,8 @@ The following arguments are supported: | |||
|
|||
* `features` - (Optional) A `features` block as documented below. | |||
|
|||
* `upstream_setting` - (Optional) An `upstream_setting` block as documented below. Using this block requires the SignalR service to be Serverless. |
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 we add a check for this?
@@ -271,6 +301,10 @@ func resourceArmSignalRServiceRead(d *schema.ResourceData, meta interface{}) err | |||
if err := d.Set("cors", flattenSignalRCors(properties.Cors)); err != nil { | |||
return fmt.Errorf("Error setting `cors`: %+v", err) | |||
} | |||
|
|||
if err := d.Set("upstream_setting", flattenUpstreamSettings(properties.Upstream)); err != nil { |
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 we check and throw an error if the service is serverless?
@@ -73,6 +82,18 @@ A `features` block supports the following: | |||
|
|||
--- | |||
|
|||
An `upstream_setting` block supports the following: | |||
|
|||
* `url_template` - (Required) The upstream URL Template. Possible values are `EnableConnectivityLogs`, `EnableMessagingLogs`, and `ServiceMode`. |
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.
Is this wrong?
Also is url_template the best name for this
@@ -73,6 +82,18 @@ A `features` block supports the following: | |||
|
|||
--- | |||
|
|||
An `upstream_setting` block supports the following: |
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.
i think think we can remove setting here.
This block seems to match things, so maybe
An `upstream_setting` block supports the following: | |
An `upstream_match` block supports the following: |
or match upstream?
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.
Based off my reading of Azure's documentation around this feature I'm not convinced either of our options are best.
Upstream is a preview feature that allows Azure SignalR Service to send messages and connection events to a set of endpoints in serverless mode. You can use upstream to invoke a hub method from clients in serverless mode and let endpoints get notified when client connections are connected or disconnected.
Based off my experience with SignalR in the past I'd imagine this is a "forwarding" mechanism for SignalR service events (clients connected, disconnected, message sent, etc.).
I am thinking that upstream
or upstream_endpoint
would better represent the intended functionality. I'm leaning towards upstream_endpoint
, thoughts?
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.
upstream endpoint makes sense to me 🙂
* `category_pattern` - (Optional) The matching pattern for category names. Possible values are `*`, multiple categories separated by commas `,`, and single categories. Ex: `connections,messages`, `*`, and `connections`. | ||
|
||
* `event_pattern` - (Optional) The matching pattern for event names. Possible values are `*`, multiple events separated by commas `,`, and single events. Ex: `connect,disconnect`, `*`, and `connect`. | ||
|
||
* `hub_pattern` - (Optional) The matching mattern for hub names. Possible values are `*`, multiple hubs separated by commas `,`, and single hubs. Ex: `hub1,hub1`, `*`, `hub1`. |
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.
pattern doesn't really make sense as its really just lists, so could we make these lists instead of strings and join them in code?
* `category_pattern` - (Optional) The matching pattern for category names. Possible values are `*`, multiple categories separated by commas `,`, and single categories. Ex: `connections,messages`, `*`, and `connections`. | |
* `event_pattern` - (Optional) The matching pattern for event names. Possible values are `*`, multiple events separated by commas `,`, and single events. Ex: `connect,disconnect`, `*`, and `connect`. | |
* `hub_pattern` - (Optional) The matching mattern for hub names. Possible values are `*`, multiple hubs separated by commas `,`, and single hubs. Ex: `hub1,hub1`, `*`, `hub1`. | |
* `categories` - (Optional) The categories to match on, or `*` for all. | |
* `events` - (Optional) The events to match on, or `*` for all. | |
* `hubs` - (Optional) The hubs to match on, or `*` for all. |
and then use would be
upstream_setting {
category_pattern = ["connections", "messages"]
event_pattern = ["*"]
hub_pattern = ["hub1"]
url_template = "http://foo.com" // if this is just any url, the template can be removed?
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.
url_template
can just be a URL or a template. See the Azure documentation,
These predefined parameters can be used in the URL pattern. Parameters will be replaced with a specified value when you're evaluating the upstream URL.
For example: http://host.com/{hub}/api/{category}/{event}
When a client connection in the "chat" hub is connected, a message will be sent to this URL: http://host.com/chat/api/connections/connected
Or, if you provide category, event, or hub patterns those will be substituted in the respective places.
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.
that makes sense to me!
I covered most of your comments @katbyte . One I didn't was around validating the input. Given the type changed from a string to a list we can't validate that anymore (based on my knowledge). Let me know your thoughts! Latest changes still pass,
|
unless an empty entry makes sense lets throw a NoEmptyStrings vlaidation on them |
azurerm_signalr_service
: Expose upstream_setting
azurerm_signalr_service
: Expose upstream_endpoint
83829c1
to
e896571
Compare
e896571
to
5b1ba78
Compare
dismissing since changes have been pushed
5b1ba78
to
a6a1d70
Compare
This reverts commit 56b3bab.
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 for pushing those changes @Brunhil
This has been released in version 2.50.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.50.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks! |
Fixes #10436
Upgrade signalr package to use 2020-05-01