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.

Sign up for GitHub

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

Jump to bottom

Add Network ACL for the resource Web Publishing Subscription #14827

Merged
merged 41 commits into from
Feb 8, 2022
Merged

Add Network ACL for the resource Web Publishing Subscription #14827

merged 41 commits into from
Feb 8, 2022

Conversation

xiaxyi
Copy link
Contributor

@xiaxyi xiaxyi commented Jan 6, 2022 •
edited
Loading

Add Network ACL configuration support for Web Publishing Subscription

This PR depends on Web Publishing Subscription
ACC test:

--- PASS: TestAccWebPubsubNetworkACL_basic (642.98s)
--- PASS: TestAccWebPubsubNetworkACL_complete_withoutPrivateEndpoint (644.39s)
--- PASS: TestAccWebPubsubNetworkACL_complete (793.09s)
--- PASS: TestAccWebPubsubNetworkACL_update (1082.39s)
--- PASS: TestAccWebPubsubNetworkACL_updateMultiplePrivateEndpoints (1263.77s)
--- PASS: TestAccWebPubsubNetworkACL_requiresImport (669.89s)

@xiaxyi
new resource "Azure Web Pubsub"
e6c4e96
@xiaxyi
fix linter issue
5321430
@xiaxyi
fmt
df0a3ab
@xiaxyi
remove unnecessary note
c463cd5
@xiaxyi
make fmt
386cccd
@xiaxyi
make fmt
133aa71
@xiaxyi
rewrite the if-else
d7107f4
@xiaxyi
modified the code as commented
eaf0d06
@xiaxyi
fmt docs
abc4d9b
@xiaxyi
go generate
c46b016
@xiaxyi
go gen
639e845
@xiaxyi
modified the code per reviewer's suggestion
2a631e1
@xiaxyi
Merge branch 'main' of https://github.com/hashicorp/terraform-provide…
b458aba 
…r-azurerm into upstream/webpubsub
@xiaxyi
modified the code per reviewer's suggestion
afa846c
@xiaxyi
modified the code per reviewer's suggestion - v2
ee2836c
@xiaxyi
modified the code per reviewer's suggestion - v3
9205e25
@xiaxyi
modified the code per reviewer's suggestion - v4
47b7f3f
@xiaxyi
modified the code per reviewer's suggestion - v4
16e44cd
@xiaxyi
golint
8b75709
@xiaxyi
golint
ef5bd13
@xiaxyi
Merge branch 'main' of https://github.com/hashicorp/terraform-provide…
792a49a 
…r-azurerm into upstream/webpubsub
@xiaxyi
removenetworkACL
14935ab
@xiaxyi
removenetworkACL-docs
94525a2
@xiaxyi
update
7871cf6
@xiaxyi
test
1e86818
@xiaxyi
Merge branch 'main' of https://github.com/hashicorp/terraform-provide…
a2ae970 
…r-azurerm into upstream/webpubsub
@xiaxyi
gencheck
f5fcf59
@xiaxyi
grammer correction/ property rename
03a8df0
@xiaxyi
make generate
9a98e3b
@xiaxyi
add network acl feature for web pubsub
62c60da
@xiaxyi xiaxyi changed the title Add network ACL for the resource Web Publishing Subscription Add Network ACL for the resource Web Publishing Subscription Jan 6, 2022
tombuildsstuff
tombuildsstuff reviewed Jan 6, 2022
View reviewed changes
internal/services/webpubsub/webpubsub_resource_test.go Outdated
Comment on lines 309 to 319
category {
name = "MessagingLogs"
enabled = false
}
category {
name = "ConnectivityLogs"
enabled = true
}
category {
name = "HttpRequestLogs"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than exposing categories like this, we should be making these properties and booleans - as we've seen with SignalR doing this leads to issues unfortunately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tombuildsstuff Are you suggesting to modify the code as below?

live_trace {

"enabled": bool,
"MessagingLogs_enabled" :bool,
"ConnectivityLogs_enabled": bool,
"HttpRequestLogs_enabled":bool,
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, i believe that is what he is getting at

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the code in the corresponding PR: Web Publishing Subscription

katbyte
katbyte reviewed Jan 19, 2022
View reviewed changes
website/docs/r/web_pubsub_hub.html.markdown Outdated
Comment on lines 78 to 80
An `event_handler` block supports the following:

* `setting` - (Required) An `setting` block as defined below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this two blocks deep? would it not make sense to remove the settings block and make event_handler a list of the settings blocks at the top level?

Copy link
Contributor Author

@xiaxyi xiaxyi Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the suggestion, I changed the schema definition accordingly

--- PASS: TestAccWebPubsubHub_requiresImport (439.00s)
--- PASS: TestAccWebPubsubHub_basic (460.46s)
--- PASS: TestAccWebPubsubHub_complete (466.93s)
--- PASS: TestAccWebPubsubHub_withAuthUpdate (679.75s)
--- PASS: TestAccWebPubsubHub_withMultipleEventhandlerSettingsUpdate (683.83s)
--- PASS: TestAccWebPubsubHub_withPropertyUpdate (704.96s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/webpubsub     705.663s

catriona-m
catriona-m approved these changes Feb 8, 2022
View reviewed changes
Copy link
Member

@catriona-m catriona-m 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 this @xiaxyi LGTM!

@catriona-m catriona-m merged commit 3e01e8a into hashicorp:main Feb 8, 2022
@github-actions github-actions bot added this to the v2.96.0 milestone Feb 8, 2022
catriona-m added a commit that referenced this pull request Feb 8, 2022
@catriona-m
Update changelog #14827
d399beb
@github-actions
Copy link

This functionality has been released in v2.96.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
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 14, 2022
@xiaxyi xiaxyi deleted the f/webpubsub/networkACL branch March 30, 2022 05:26
@xiaxyi xiaxyi restored the f/webpubsub/networkACL branch March 30, 2022 05:26
@xiaxyi xiaxyi deleted the f/webpubsub/networkACL branch August 14, 2022 00:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff left review comments

@katbyte katbyte katbyte left review comments

@catriona-m catriona-m catriona-m approved these changes

Assignees

@catriona-m catriona-m

Labels
documentation new-data-source new-resource service/application-insights service/signalr service/web-pub-sub size/XL
Projects
None yet
Milestone
v2.96.0
Development

Successfully merging this pull request may close these issues.
5 participants
@xiaxyi @tombuildsstuff @katbyte @catriona-m @jackofallops