Overprovision should be default true. Terraform and the service back … #1322

VaijanathB · 2018-05-30T21:44:14Z

…end is considering this as default false when nothing is specified. I have run all the VMSS tests. Most of them pass with some unrelated failures.

TestAccAzureRMVirtualMachineScaleSet
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -run=TestAccAzureRMVirtualMachineScaleSet -timeout 180m
=== RUN   TestAccAzureRMVirtualMachineScaleSet_importBasic
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importBasic (355.53s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_importBasic_managedDisk
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importBasic_managedDisk (354.05s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_importBasic_managedDisk_withZones
--- FAIL: TestAccAzureRMVirtualMachineScaleSet_importBasic_managedDisk_withZones (101.70s)
	testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
		
		* azurerm_virtual_machine_scale_set.test: 1 error(s) occurred:
		
		* azurerm_virtual_machine_scale_set.test: compute.VirtualMachineScaleSetsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ResourceTypeNotSupportAvailabilityZones" Message="The resource type 'virtualMachineScaleSets' does not support availability zones at location 'southcentralus' and api-version '2017-12-01'."
=== RUN   TestAccAzureRMVirtualMachineScaleSet_importLinux
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importLinux (367.60s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_importLoadBalancer
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importLoadBalancer (365.93s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_importOverProvision
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importOverProvision (364.06s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_importExtension
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importExtension (367.29s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_importMultipleExtensions
--- PASS: TestAccAzureRMVirtualMachineScaleSet_importMultipleExtensions (364.91s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basic
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basic (360.84s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicPublicIP
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicPublicIP (354.82s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicAcceleratedNetworking
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicAcceleratedNetworking (356.50s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_bootDiagnostic
--- PASS: TestAccAzureRMVirtualMachineScaleSet_bootDiagnostic (353.95s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_networkSecurityGroup
--- PASS: TestAccAzureRMVirtualMachineScaleSet_networkSecurityGroup (363.09s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicWindows
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicWindows (401.86s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_singlePlacementGroupFalse
--- PASS: TestAccAzureRMVirtualMachineScaleSet_singlePlacementGroupFalse (367.48s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_linuxUpdated
--- PASS: TestAccAzureRMVirtualMachineScaleSet_linuxUpdated (522.11s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_customDataUpdated
--- PASS: TestAccAzureRMVirtualMachineScaleSet_customDataUpdated (461.82s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDisk (363.52s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk (470.57s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize (651.61s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDiskNoName
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux_managedDiskNoName (355.63s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears (426.61s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_planManagedDisk
--- FAIL: TestAccAzureRMVirtualMachineScaleSet_planManagedDisk (103.15s)
	testing.go:513: Step 0 error: Error applying: 1 error(s) occurred:
		
		* azurerm_virtual_machine_scale_set.test: 1 error(s) occurred:
		
		* azurerm_virtual_machine_scale_set.test: compute.VirtualMachineScaleSetsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ResourcePurchaseValidationFailed" Message="User failed validation to purchase resources. Error message: 'Legal terms have not been accepted for this item on this subscription. To accept legal terms using PowerShell, please use Get-AzureRmMarketplaceTerms and Set-AzureRmMarketplaceTerms API(https://go.microsoft.com/fwlink/?linkid=862451) or deploy via the Azure portal to accept the terms'"

…end is considering this as default false when nothing is specified.

VaijanathB · 2018-05-30T21:45:30Z

We have had discussion with VMSS team and they suggested that we should make this Default True.

tombuildsstuff

hey @VaijanathB

Thanks for this PR - I've taken a look through and left a comment inline, if we can fix this up it should be good to merge. In general we need to be a little careful with default changes; however in this particular case (since it's a top-level object outside a hash function) it should be fine, providing we message it appropriately in the changelog (which we'll handle post-merge).

Thanks!

tombuildsstuff · 2018-05-31T08:34:01Z

website/docs/r/virtual_machine_scale_set.html.markdown

@@ -245,7 +245,7 @@ The following arguments are supported:
 * `location` - (Required) Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created.
 * `sku` - (Required) A sku block as documented below.
 * `upgrade_policy_mode` - (Required) Specifies the mode of an upgrade to virtual machines in the scale set. Possible values, `Manual` or `Automatic`.
-* `overprovision` - (Optional) Specifies whether the virtual machine scale set should be overprovisioned.
+* `overprovision` - (Optional) Specifies whether the virtual machine scale set should be overprovisioned. Default set to true when not specified.


Can we make this "Defaults to true" to match the other docs?

tombuildsstuff · 2018-05-31T08:34:19Z

azurerm/resource_arm_virtual_machine_scale_set.go

@@ -106,6 +106,7 @@ func resourceArmVirtualMachineScaleSet() *schema.Resource {
 			"overprovision": {
 				Type:     schema.TypeBool,
 				Optional: true,
+				Default:  true,


bear in mind this will currently force a change for existing resources - so this is going to need to go out in a major version & we're going to want to call this out in the release notes (cc @katbyte)

VaijanathB · 2018-05-31T17:31:56Z

@tombuildsstuff Thanks for the review. I have updated the doc to change as requested. It should be fine if this goes in next major release.

tombuildsstuff · 2018-06-01T10:50:51Z

website/docs/r/virtual_machine_scale_set.html.markdown

-* `overprovision` - (Optional) Specifies whether the virtual machine scale set should be overprovisioned.
-* `single_placement_group` - (Optional) Specifies whether the scale set is limited to a single placement group with a maximum size of 100 virtual machines. If set to false, managed disks must be used. Default is true. Changing this forces a
+* `overprovision` - (Optional) Specifies whether the virtual machine scale set should be overprovisioned. Defaults to `true`.
+* `single_placement_group` - (Optional) Specifies whether the scale set is limited to a single placement group with a maximum size of 100 virtual machines. If set to false, managed disks must be used. Defaults to `true`. Changing this forces a


pushed a commit to quote both of these values

tombuildsstuff

LGTM - I'll add a message to the changelog about this 👍

ghost · 2020-03-31T14:53:02Z

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 👉 [email protected]. Thanks!

Overprovision should be default true. Terraform and the service back …

f6c616e

…end is considering this as default false when nothing is specified.

VaijanathB requested review from tombuildsstuff and JunyiYi May 30, 2018 21:44

Updating the documentation to reflect the change

f31927c

tombuildsstuff suggested changes May 31, 2018

View reviewed changes

tombuildsstuff added enhancement service/virtual-machine waiting-response labels May 31, 2018

tombuildsstuff added this to the Soon milestone May 31, 2018

Changing the sentence based on code review

c59756e

Quoting the true values

76fbee0

tombuildsstuff reviewed Jun 1, 2018

View reviewed changes

tombuildsstuff approved these changes Jun 1, 2018

View reviewed changes

tombuildsstuff removed the request for review from JunyiYi June 1, 2018 10:51

tombuildsstuff modified the milestones: Soon, 1.7.0 Jun 1, 2018

tombuildsstuff removed the waiting-response label Jun 1, 2018

tombuildsstuff merged commit 8de4693 into master Jun 1, 2018

tombuildsstuff deleted the vmssOverprovision branch June 1, 2018 10:54

tombuildsstuff added a commit that referenced this pull request Jun 1, 2018

Updating to include #1322

5bae6de

tombuildsstuff added a commit that referenced this pull request Jun 1, 2018

Updating to include #1322

def46aa

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overprovision should be default true. Terraform and the service back … #1322

Overprovision should be default true. Terraform and the service back … #1322

VaijanathB commented May 30, 2018 •

edited by tombuildsstuff

Loading

VaijanathB commented May 30, 2018

tombuildsstuff left a comment

tombuildsstuff May 31, 2018

VaijanathB May 31, 2018

tombuildsstuff May 31, 2018

VaijanathB commented May 31, 2018

tombuildsstuff Jun 1, 2018

tombuildsstuff left a comment

ghost commented Mar 31, 2020

Overprovision should be default true. Terraform and the service back … #1322

Overprovision should be default true. Terraform and the service back … #1322

Conversation

VaijanathB commented May 30, 2018 • edited by tombuildsstuff Loading

VaijanathB commented May 30, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff May 31, 2018

Choose a reason for hiding this comment

VaijanathB May 31, 2018

Choose a reason for hiding this comment

tombuildsstuff May 31, 2018

Choose a reason for hiding this comment

VaijanathB commented May 31, 2018

tombuildsstuff Jun 1, 2018

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

ghost commented Mar 31, 2020

VaijanathB commented May 30, 2018 •

edited by tombuildsstuff

Loading