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_sql_managed_instance_active_directory_administrator #14104

Merged

Conversation

aristosvo
Copy link
Collaborator

@aristosvo aristosvo commented Nov 9, 2021

Fixes #13957

Needs azuread v2 via #14105
Needs merge of #14052

Acceptance Tests

TF_ACC=1 go test -v ./internal/services/sql -run=TestAccSqlMiAdministrator_basic -timeout 1800m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccSqlMiAdministrator_basic
=== PAUSE TestAccSqlMiAdministrator_basic
=== CONT  TestAccSqlMiAdministrator_basic
--- PASS: TestAccSqlMiAdministrator_basic (17391.73s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/sql	17391.752s

@aristosvo aristosvo force-pushed the sql/managed-instance-administrator branch 2 times, most recently from 378e9b7 to 92ce45a Compare November 10, 2021 08:48
@aristosvo aristosvo added service/mssql Microsoft SQL Server new-resource new-virtual-resource Resources which are split out to enhance the user experience and removed new-resource labels Nov 10, 2021
@aristosvo aristosvo force-pushed the sql/managed-instance-administrator branch from 92ce45a to a654c1e Compare November 12, 2021 07:26
@aristosvo aristosvo marked this pull request as ready for review November 12, 2021 07:27
@aristosvo aristosvo force-pushed the sql/managed-instance-administrator branch 2 times, most recently from 9ca37ad to 164b0a9 Compare November 18, 2021 07:58
@katbyte
Copy link
Collaborator

katbyte commented Nov 19, 2021

Hmm, looks like we need to increase the timeouts even more?

------- Stdout: -------
=== RUN   TestAccAzureRMSqlMiServer_basic
=== PAUSE TestAccAzureRMSqlMiServer_basic
=== CONT  TestAccAzureRMSqlMiServer_basic
    testcase.go:110: Step 1/2 error: Error running apply: exit status 1
        
        Error: sql.ManagedInstancesClient#CreateOrUpdate: Failure sending request: StatusCode=504 -- Original Error: Code="GatewayTimeout" Message="The gateway did not receive a response from 'Microsoft.Sql' within the specified time period."
        
          with azurerm_sql_managed_instance.test,
          on terraform_plugin_test.tf line 1142, in resource "azurerm_sql_managed_instance" "test":
        1142: resource "azurerm_sql_managed_instance" "test" {
        
--- FAIL: TestAccAzureRMSqlMiServer_basic (1289.87s)
FAIL

@aristosvo
Copy link
Collaborator Author

aristosvo commented Nov 19, 2021

@katbyte which timeouts can we possibly change here? This seems like a timeout of the Azure API, not a resource creation timeout AFAIK.

The resource create timeout for a Managed Instance is 24hrs, which is much more than current the 20 minutes timeout we're facing..? This looks like an Azure API instability issue

@aristosvo
Copy link
Collaborator Author

aristosvo commented Nov 22, 2021

Currently waiting for response on Azure/azure-rest-api-specs#16838

Edit: found a workaround, first tests are promising:

// azurerm_sql_managed_instance is working againTF_ACC=1 go test -v ./internal/services/sql -run=TestAccAzureRMSqlMiServer_basic -timeout 1800m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMSqlMiServer_basic
=== PAUSE TestAccAzureRMSqlMiServer_basic
=== CONT  TestAccAzureRMSqlMiServer_basic
    testcase.go:110: Step 1/2 is completed for TestAccAzureRMSqlMiServer_basic
    testcase.go:110: Step 2/2 is completed for TestAccAzureRMSqlMiServer_basic
--- PASS: TestAccAzureRMSqlMiServer_basic (16103.77s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/sql   16106.193s
// Identity works as expectedTF_ACC=1 go test -v ./internal/services/sql -run=TestAccAzureRMSqlMiServer_identity -timeout 1800m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMSqlMiServer_identity
=== PAUSE TestAccAzureRMSqlMiServer_identity
=== CONT  TestAccAzureRMSqlMiServer_identity
--- PASS: TestAccAzureRMSqlMiServer_identity (18194.80s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/sql	18194.833s

@aristosvo aristosvo added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Nov 22, 2021
@aristosvo aristosvo changed the title azurerm_sql_managed_identity_active_directory_administrator: New resource azurerm_sql_managed_instance_active_directory_administrator: New resource Nov 22, 2021
@katbyte katbyte added this to the Blocked milestone Nov 22, 2021
@aristosvo aristosvo removed the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Nov 23, 2021
Copy link
Collaborator Author

@aristosvo aristosvo left a comment

Choose a reason for hiding this comment

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

Another error on import:

--- PASS: TestAccSqlMiAdministrator_basic (17654.32s)
=== CONT  TestAccSqlMiAdministrator_requiresImport
    testcase.go:110: Step 3/3, expected an error with pattern, no match on: Error running pre-apply refresh: exit status 1
        
        Error: Reference to undeclared resource

@aristosvo aristosvo force-pushed the sql/managed-instance-administrator branch 3 times, most recently from d410e8a to ebaa249 Compare November 30, 2021 18:39
@aristosvo
Copy link
Collaborator Author

And last test fixed:

--- PASS: TestAccSqlMiAdministrator_requiresImport (15679.28s)

@aristosvo aristosvo removed this from the Blocked milestone Dec 1, 2021
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @aristosvo! This PR looks good with just a couple callouts

@aristosvo aristosvo force-pushed the sql/managed-instance-administrator branch from ebaa249 to c7a3d91 Compare December 5, 2021 11:41
@aristosvo
Copy link
Collaborator Author

Rebased and squashed!

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @aristosvo

@mbfrahry mbfrahry added this to the v2.89.0 milestone Dec 6, 2021
@mbfrahry mbfrahry changed the title azurerm_sql_managed_instance_active_directory_administrator: New resource New Resource: azurerm_sql_managed_instance_active_directory_administrator Dec 6, 2021
@mbfrahry mbfrahry merged commit ed48322 into hashicorp:main Dec 6, 2021
mbfrahry added a commit that referenced this pull request Dec 6, 2021
@github-actions
Copy link

This functionality has been released in v2.89.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 Jan 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation new-virtual-resource Resources which are split out to enhance the user experience service/mssql Microsoft SQL Server size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Active Directory admin of a SQL Managed Instance
3 participants