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

azurerm_web_pubsub_hub - add event_listener property #21145

Merged
merged 9 commits into from
Apr 5, 2023

Conversation

xiaxyi
Copy link
Contributor

@xiaxyi xiaxyi commented Mar 27, 2023

acc test:
--- PASS: TestAccWebPubsubHub_withMultipleEventListenerSettingsUpdate (2040.01s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/signalr 2040.772s


* `system_event_name_filter` - (Optional) Specify the list of system events. Supported values are `connect`, `connected` and `disconnected`.

* `user_event_name_filter` - (Optional) Specify the matching event names. There are 3 kind of patterns supported: * `*` matches any event name * `,` Combine multiple events with `,` for example `event1,event2`, it matches event `event1` and `event2` * The single event name, for example `event1`, it matches `event1`. Defaults to `"*"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this property be a lint then instead of CSV?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks kt for the review, sorry that I can't quite follow your question, can you be more specific about it? Thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry list. ie as it is * or event1,event2 it could be user_event_name_filter = ["*"] and user_event_name_filter = ['event1","event2"]

@xiaxyi
Copy link
Contributor Author

xiaxyi commented Mar 31, 2023

Thanks Kt for the review, the property is defined as string in api, just like user event pattern in event handler.
image

Are you suggesting that we make it as a list and do the list-string-list conversion in the provider?

@katbyte
Copy link
Collaborator

katbyte commented Apr 2, 2023

@xiaxyi - i understand that is what it is in the API, however that appears to be a string taking a CSV list. so i think it makes sense to define it in terraform as a list and compose the CSV string so users can leverage arrays loops etc in terraform?

@xiaxyi
Copy link
Contributor Author

xiaxyi commented Apr 3, 2023

Thanks @katbyte for the suggestion, code and document is updated and local test is passed:

--- PASS: TestAccWebPubsubHub_withMultipleEventListenerSettingsUpdate (2012.61s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/signalr       2013.561s

image

Elem: &pluginsdk.Resource{
Schema: map[string]*pluginsdk.Schema{
"user_event_name_filter": {
Type: pluginsdk.TypeSet,
Copy link
Member

Choose a reason for hiding this comment

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

This probably wants to be a list instead of a set

Suggested change
Type: pluginsdk.TypeSet,
Type: pluginsdk.TypeList,

},

"system_event_name_filter": {
Type: pluginsdk.TypeSet,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Suggested change
Type: pluginsdk.TypeSet,
Type: pluginsdk.TypeList,

@xiaxyi
Copy link
Contributor Author

xiaxyi commented Apr 4, 2023

Thanks @stephybun , you are correct, we do need to make the property an ordered list. I changed the type.

--- PASS: TestAccWebPubsubHub_withMultipleEventListenerSettingsUpdate (1793.76s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/signalr       1794.542s


Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for switching that over to a List @xiaxyi, I left a few more comments which I only noticed after looking into this deeper, once those are fixed up this should be good to go!

website/docs/r/web_pubsub_hub.html.markdown Outdated Show resolved Hide resolved
website/docs/r/web_pubsub_hub.html.markdown Outdated Show resolved Hide resolved
website/docs/r/web_pubsub_hub.html.markdown Outdated Show resolved Hide resolved
@xiaxyi
Copy link
Contributor Author

xiaxyi commented Apr 4, 2023

Thanks @stephybun , updated the docs.

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @xiaxyi LGTM 🍜

@stephybun stephybun merged commit 06f82d9 into hashicorp:main Apr 5, 2023
@github-actions github-actions bot added this to the v3.51.0 milestone Apr 5, 2023
stephybun added a commit that referenced this pull request Apr 5, 2023
@github-actions
Copy link

This functionality has been released in v3.51.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants