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

New Resource - azurerm_signalr_service_custom_domain, azurerm_web_pubsub_custom_domain #21153

Merged
merged 31 commits into from
May 23, 2023

Conversation

xiaxyi
Copy link
Contributor

@xiaxyi xiaxyi commented Mar 28, 2023

The acc test requires some prerequisite:

  1. we need to provide an owned domain
  2. we need to add the custom certificate first -> depends on pr:New Resource - azurerm_web_pubsub_custom_certificate #21114 and New Resource - azurerm_signalr_service_custom_certificate #21112
  3. we need to add the cname record for the signalr service in the owned domain as mentioned in this docs:https://learn.microsoft.com/en-us/azure/azure-web-pubsub/howto-custom-domain?tabs=vault-access-policy%2Cazure-powershell#create-a-custom-domain-cname

any of the prerequisite mentioned above not satisfied will cause the acc test failure as the polling function is checking the resource provisioning status.

@xiaxyi
Copy link
Contributor Author

xiaxyi commented Mar 29, 2023

acc tests:

--- PASS: TestAccSignalrServiceCustomDomainResource_requiresImport (581.07s)
--- PASS: TestAccSignalrServiceCustomDomainResource_basic (619.66s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/signalr       620.573s
--- PASS: TestAccWebPubsubCustomDomainResource_requiresImport (239.91s)
--- PASS: TestAccWebPubsubCustomDomainResource_basic (314.16s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/signalr       314.911s

@xiaxyi xiaxyi changed the title New Resource - azurerm_signalr_custom_domain, azurerm_web_pubsub_custom_domain New Resource - azurerm_signalr_service_custom_domain, azurerm_web_pubsub_custom_domain Mar 29, 2023
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.

tests are failing:

------- Stdout: -------
=== RUN   TestAccSignalrServiceCustomDomainResource_basic
=== PAUSE TestAccSignalrServiceCustomDomainResource_basic
=== CONT  TestAccSignalrServiceCustomDomainResource_basic
    testcase.go:110: Step 1/2 error: Error running pre-apply refresh: exit status 1
        
        Error: Reference to undeclared resource
        
          on terraform_plugin_test.tf line 65, in resource "azurerm_key_vault" "test":
          65:     object_id = data.azurerm_signalr_service.test.identity[0].principal_id
        
        A data resource "azurerm_signalr_service" "test" has not been declared in the
        root module.
        
        Error: Invalid resource type
        
          on terraform_plugin_test.tf line 100, in resource "azurerm_signalr_custom_domain" "test":
         100: resource "azurerm_signalr_custom_domain" "test" {
        
        The provider hashicorp/azurerm does not support resource type
        "azurerm_signalr_custom_domain".
--- FAIL: TestAccSignalrServiceCustomDomainResource_basic (2.48s)
FAIL

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 this PR @xiaxyi. Overall this is looking good, I left some comments on the resources as well as regarding casing and consistency in the docs. Once those are resolved we can take another look through and get this merged!

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

xiaxyi commented Apr 26, 2023

Thanks @katbyte for the comment, the operation to update the custom domain of SignalR service is actually updating the SignalR service's property. The two APIs are not totally independent, so we need to ensure SignalR update is finished before moving forward to another operation.

@stephybun
Copy link
Member

@xiaxyi - coming back to my initial comment and suggestion if creating/updating the custom domain updates the parent resource then we should have the logic that checks the status in the create/update and only return once it's ready instead of waiting for it in the delete. Does that make sense?

@xiaxyi
Copy link
Contributor Author

xiaxyi commented Apr 28, 2023

@stephybun thanks for replying back, as we discussed offline. The reason to add the signalR waiter here is to ensure the delete can be started when siganlR service is not being updated by another operation since the operation on custom domain is actually updating the signalR service. can you let me know the concerns of this waiting function?

@stephybun
Copy link
Member

@xiaxyi after discussion with @tombuildsstuff the following needs to be done:

  • Needing additional wait logic in the delete indicates an eventual consistency issue/bug in the API - removing this waiting logic in the delete will highlight where the issue is and where we will need to add locks to these resources and associated resources e.g. azurerm_signalr_service to account for that
  • We will also need to add a custom poller to check the provisioning state in the create/update of this resource and associated resources

Ensuring the resource is in a ready state before deletion is outside of the scope for terraform - one of the core tenants is that when we've finished a create or update of something it is truly ready and fully provisioned.

If you can make the changes outlined above we can take another look through this.

@xiaxyi
Copy link
Contributor Author

xiaxyi commented May 4, 2023

Thanks @stephybun for the comment, I updated the code, can you take a look and let me know if anything needed.

@xiaxyi
Copy link
Contributor Author

xiaxyi commented May 15, 2023

Add API issue link:Azure/azure-rest-api-specs#23986

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 🌵


deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("context had no deadline")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("context had no deadline")
return fmt.Errorf("internal-error: context had no deadline")


deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("context had no deadline")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("context had no deadline")
return fmt.Errorf("internal-error: context had no deadline")


deadline, ok := ctx.Deadline()
if !ok {
return fmt.Errorf("context had no deadline")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("context had no deadline")
return fmt.Errorf("internal-error: context had no deadline")

@katbyte katbyte merged commit 11c45a1 into hashicorp:main May 23, 2023
@github-actions github-actions bot added this to the v3.58.0 milestone May 23, 2023
@github-actions
Copy link

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

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 May 29, 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.

5 participants