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

Adding shared private link resource for the Azure web pubsub resource #15550

Merged
merged 15 commits into from Jul 20, 2022

Conversation

xiaxyi
Copy link
Contributor

@xiaxyi xiaxyi commented Feb 22, 2022

ACC test:

--- PASS: TestAccWebPubsubPrivateLinkedService_basic (424.23s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/signalr       425.031s
--- PASS: TestAccWebPubsubSharedPrivateLinkResource_basic (567.02s)
--- PASS: TestAccWebPubsubSharedPrivateLinkResource_requiresImport (597.15s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/signalr       597.831s

},

"shared_private_link_resource_types": {
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.

a Data Source cannot contain a Set - this needs to be a List

furthermore: what's the intention of this data source? why isn't this part of the WebPubSub data source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This data source is intended to provide a reference of supported sub resource that can be linked by a web pubsub resource, just as the same as the purpose of azurerm_monitor_diagnostic_categories. I thought it would be more straight-forward to get this information separately since user may would only like to refer to such information?

Copy link
Contributor Author

@xiaxyi xiaxyi Mar 23, 2022

Choose a reason for hiding this comment

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

@tombuildsstuff may I know if you have any other concerns that I can help to address? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

In what instances would a user need this information? I searched the repo and I cannot find any other services with such a private link resource data source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My first thought was to provide a reference to the user, just in case they don't know what information to put in the propertysubresource_name. I can remove it if it looks like unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that is the entire resource thou? thjats why i am asking what is the use to a user for this resoruce?

Copy link
Contributor Author

@xiaxyi xiaxyi May 10, 2022

Choose a reason for hiding this comment

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

sorry I don't quite follow your concerns. I added this data source just in case the user doesn't know what value to put in the subresource_name when using the resource azurerm_web_pubsub_shared_private_link_resource

the supported subresource varies from different resources, for example, the output of this datasource is:

webpubsub_private_supported_resource = {
  "id" = "/subscriptions/733b9054-5ed8-4f10-a444-c32458359d40/resourceGroups/xiaxintest/providers/Microsoft.SignalRService/WebPubSub/acctestWebPubsub"
  "shared_private_link_resource_types" = tolist([
    {
      "description" = "Azure App Service can be used as an upstream"
      "subresource_name" = "sites"
    },
    {
      "description" = "Azure Key Vault can be used as credentials store"
      "subresource_name" = "vault"
    },
  ])

The user can put either site or vault in the property subresource_name in below tf block:

resource "azurerm_web_pubsub_shared_private_link_resource" "test" {
  name               = "acctest-%d"
  web_pubsub_id      = azurerm_web_pubsub.test.id
  subresource_name   = "vault"
  target_resource_id = azurerm_key_vault.test.id
}

@xiaxyi
Copy link
Contributor Author

xiaxyi commented May 7, 2022

replied inline

@@ -2,3 +2,4 @@ package signalr

//go:generate go run ../../tools/generator-resource-id/main.go -path=./ -name=WebPubsub -id=/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/resGroup1/providers/Microsoft.SignalRService/WebPubSub/Webpubsub1
//go:generate go run ../../tools/generator-resource-id/main.go -path=./ -name=WebPubsubHub -id=/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/resGroup1/providers/Microsoft.SignalRService/WebPubSub/Webpubsub1/hubs/Webpubsubhub1
//go:generate go run ../../tools/generator-resource-id/main.go -path=./ -name=WebPubsubSharedPrivateLinkResource -id=/subscriptions/12345678-1234-9876-4563-123456789012/resourcegroups/resGroup1/providers/Microsoft.SignalRService/WebPubSub/Webpubsub1/sharedPrivateLinkResources/resource1
Copy link
Member

Choose a reason for hiding this comment

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

the casing here is wrong, this needs to be:

Suggested change
//go:generate go run ../../tools/generator-resource-id/main.go -path=./ -name=WebPubsubSharedPrivateLinkResource -id=/subscriptions/12345678-1234-9876-4563-123456789012/resourcegroups/resGroup1/providers/Microsoft.SignalRService/WebPubSub/Webpubsub1/sharedPrivateLinkResources/resource1
//go:generate go run ../../tools/generator-resource-id/main.go -path=./ -name=WebPubsubSharedPrivateLinkResource -id=/subscriptions/12345678-1234-9876-4563-123456789012/resourceGroups/resGroup1/providers/Microsoft.SignalRService/webPubSub/Webpubsub1/sharedPrivateLinkResources/resource1

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 @tombuildsstuff, I will make resourceGroups as camel cased, however, for webPubSub oe, the service is returning the value as WebPubSub and this behavior is the same as its parent WebPubSub resource. Shall we keep it as capital cased?

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be correcting it to the proper case

@xiaxyi
Copy link
Contributor Author

xiaxyi commented Jul 19, 2022

@katbyte I updated the ID, feel free to let me know if everything looks good to you. :)

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

LGTM 🔝

@katbyte katbyte merged commit 971915d into hashicorp:main Jul 20, 2022
katbyte added a commit that referenced this pull request Jul 20, 2022
@github-actions github-actions bot added this to the v3.15.0 milestone Jul 20, 2022
@github-actions
Copy link

This functionality has been released in v3.15.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!

@xiaxyi xiaxyi deleted the wps/privateLink branch August 1, 2022 02:21
@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 Aug 31, 2022
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.

None yet

3 participants