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_mssql_virtual_machine #5263

Merged
merged 42 commits into from
Mar 27, 2020

Conversation

yupwei68
Copy link
Contributor

@yupwei68 yupwei68 commented Dec 27, 2019

add resource mssqlvm

fixes #3891

fixes Azure/terraform-azurerm-compute#63

mssqlvm test pass
2020.03.05 all test passed!

@katbyte katbyte changed the title add resource mssqlvm New Resource and Data Source: azurerm_mssql_virtual_machine Jan 8, 2020
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.

Hi @yupwei68, thank you for the new resource,

I've done an initial review and left comments inline that need to be addressed.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @yupwei68

Thanks for pushing those changes/rebasing this - I've taken a look through and left some comments in addition to the ones @katbyte's left - primarily around the schema design, but this is off to a good start; if we can fix those up then we should be able to take another look 👍

Thanks!

azurerm/internal/services/mssqlvm/registration.go Outdated Show resolved Hide resolved
website/docs/r/mssql_virtual_machine.html.markdown Outdated Show resolved Hide resolved
website/docs/r/mssql_virtual_machine.html.markdown Outdated Show resolved Hide resolved
website/docs/r/mssql_virtual_machine.html.markdown Outdated Show resolved Hide resolved
website/docs/r/mssql_virtual_machine.html.markdown Outdated Show resolved Hide resolved
website/docs/r/mssql_virtual_machine.html.markdown Outdated Show resolved Hide resolved
yupwei added 2 commits March 5, 2020 16:01
@yupwei68
Copy link
Contributor Author

yupwei68 commented Mar 9, 2020

hi @tombuildsstuff @katbyte ,I have updated code and restested with pass to fit azurerm 2.0. Please help to review.

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.

Thanks @yupwei68,

I've left some more comments inline that should be addressed before merge.

@yupwei68 yupwei68 changed the title New Resource: azurerm_mssql_virtual_machine [WIP]New Resource: azurerm_mssql_virtual_machine Mar 24, 2020
@katbyte katbyte changed the title [WIP]New Resource: azurerm_mssql_virtual_machine New Resource: azurerm_mssql_virtual_machine Mar 25, 2020
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.

Thanks for the change @yupwei68, just a couple more minor comments and it seems like the tests are failing in our CI:

Test Failed

------- Stdout: -------
=== RUN   TestAccAzureRMMsSqlVirtualMachine_basic
=== PAUSE TestAccAzureRMMsSqlVirtualMachine_basic
=== CONT  TestAccAzureRMMsSqlVirtualMachine_basic
--- FAIL: TestAccAzureRMMsSqlVirtualMachine_basic (150.75s)
    testing.go:640: Step 0 error: errors during apply:
        
        Error: compute.VirtualMachinesClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="SkuNotAvailable" Message="The requested size for resource '/subscriptions/1a6092a6-137e-4025-9a7c-ef77f76f2c02/resourceGroups/acctestRG-mssql-200325213822100701/providers/Microsoft.Compute/virtualMachines/acctest-VM-200325213822100701' is currently not available in location 'westeurope' zones '' for subscription '1a6092a6-137e-4025-9a7c-ef77f76f2c02'. Please try another size or deploy to a different location or zones. See https://aka.ms/azureskunotavailable for details."
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test082828343/main.tf line 90:
          (source code not available)

@jackofallops
Copy link
Member

Hi @yupwei68
Just adding to Kt's comment above on the test. It looks like the SKU in the test isn't available in our primary test location. Could you update the test to use a newer VM Resource that's supported in westeurope? (Suggest Standard_F2s?)

@Ugo-G
Copy link

Ugo-G commented Mar 26, 2020

Salut @ yupwei68 Ajout
juste au commentaire de Kt ci-dessus sur le test. Il semble que la référence du test ne soit pas disponible dans notre emplacement de test principal. Pourriez-vous mettre à jour le test pour utiliser une nouvelle ressource VM prise en charge dans westeurope? (Suggérer Standard_F2s?)

Hello,
Please note, I do not know if this subject is related to this problem, but most Azure servers are currently full in Europe.
Servers in France, the United Kingdom and Northern Europe are affected.
There is still a little space in Western Europe, but it is still unstable.

@ghost ghost removed the waiting-response label Mar 26, 2020
@jackofallops
Copy link
Member

Hello,
Please note, I do not know if this subject is related to this problem, but most Azure servers are currently full in Europe.
Servers in France, the United Kingdom and Northern Europe are affected.
There is still a little space in Western Europe, but it is still unstable.

Thanks @Ugo-G - we've seen evidence of this also. The test didn't need the size of instance it was using to check the functionality. I'm also reasonably sure that the SKU in the test isn't normally available in West Europe in normal conditions (though v2 variants are).

@yupwei68
Copy link
Contributor Author

@katbyte @jackofallops Sorry the delay. I have updated the comments' changes. But recently, the update autopatch functions seem to fail in service team. They're looking for solution.

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.

Thanks for the changes @yupwei68, LGTM now 👍

@katbyte katbyte merged commit ce55e57 into hashicorp:master Mar 27, 2020
katbyte added a commit that referenced this pull request Mar 27, 2020
@azjon
Copy link

azjon commented Mar 30, 2020

Hi,

Are there any plans to add support for spanned disks, I'm currently trying to add multiple disks for data, log, tempdb. In ARM this is configured in the SQL extension or SQL resource provider:

"StorageConfigurationSettings": {
"DiskConfigurationType": "NEW",
"StorageWorkloadType": "OLTP",
"SQLDataSettings": {
"LUNs": "[1,2]",
"DefaultFilePath": "S:\ExamplePath\Data"
},
"SQLLogSettings": {
"LUNs": "[3,4]",
"DefaultFilePath": "F:\ExamplePath\Data"
},
"SQLTempDbSettings": {
"LUNs": "[5,6]",
"DefaultFilePath": "G:\ExamplePath\TempDB"
}
}

@yupwei68
Copy link
Contributor Author

Hi @azjon , we shall wait till the service returns the storage configuration back.

@ghost
Copy link

ghost commented Apr 26, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants