AzureStack Scale set Resource #15

thetonymaster · 2018-07-30T17:21:33Z

No description provided.

katbyte

In addition to the comments I have left inline:

Would we want to keep the unsupported properties & code for them in & commented out so when they are supported it is easy to implement them?
The documentation doesn't appear to have been updated with the new resource prefix: azurestack
one unsupported property is still active & in the docs
Some of the acceptance tests currently do not pass as required prerequisite resources are not implemented yet. Could we either skip() or remove them?

katbyte · 2018-07-30T20:17:27Z

azurestack/resource_arm_virtual_machine_scale_set.go

+
+			"resource_group_name": resourceGroupNameSchema(),
+
+			// "zones": zonesSchema(),


A comment explaining why this was commented out would be useful?

katbyte · 2018-07-31T15:26:14Z

azurestack/resource_arm_virtual_machine_scale_set.go

+	}
+	storageProfile.OsDisk = osDisk
+
+	// if _, ok := d.GetOk("storage_profile_data_disk"); ok {


Is there a reason this was commented out and not removed? an inline comment describe why would be useful

katbyte · 2018-07-31T15:48:15Z

website/docs/r/compute_virtualmachine_scale_set.html.markdown

@@ -0,0 +1,314 @@
+---
+layout: "azurerm"
+page_title: "Azure Resource Manager: azurerm_virtual_machine_scale_set"


These have not been updated to say azure stack.

Also this file has not been updated to include the resource.

katbyte · 2018-07-31T15:49:18Z

website/docs/r/compute_virtualmachine_scale_set.html.markdown

+  Create a Virtual Machine scale set.
+---
+
+# azurerm\_virtual\_machine\_scale\_set


Shouldn't this be azurestack?

Also the \s can be removed.

katbyte · 2018-07-31T15:49:31Z

website/docs/r/compute_virtualmachine_scale_set.html.markdown

+  container_access_type = "private"
+}
+
+resource "azurerm_virtual_machine_scale_set" "test" {


Shouldn't this also be azurestack

katbyte · 2018-07-31T15:52:43Z

azurestack/resource_arm_virtual_machine_scale_set.go

+				Set: resourceArmVirtualMachineScaleSetNetworkConfigurationHash,
+			},
+
+			"boot_diagnostics": {


According to a test comment this is not supported, could we comment it out with the reasoning why?

Hello, @katbyte I've opened an issue about this Azure/azure-sdk-for-go#2495. This field can be set according to https://docs.microsoft.com/en-us/rest/api/compute/virtualmachinescalesets/createorupdate#virtualmachinescalesetvmprofile documentation.

For now, we are going to comment on this argument and give this reason.

katbyte · 2018-07-31T15:55:35Z

website/docs/r/compute_virtualmachine_scale_set.html.markdown

+* `sku` - (Optional) Specifies the SKU of the image used to create the virtual machines.
+* `version` - (Optional) Specifies the version of the image used to create the virtual machines.
+
+`boot_diagnostics` supports the following:


Test comment says this is not supported, so it should probably be removed from here

This comes in the response of the request, but I can't send any configuration, will ask Walter if this is supported by AzureStack, if it is, will fill a bug for a missing argument, in the meanwhile I moved it to the argument reference.

katbyte · 2018-07-31T16:49:07Z

current test status:

thetonymaster · 2018-07-31T16:55:02Z

Hey @katbyte

About this

Hello @thetonymaster,

In addition to the comments I have left inline:

Would we want to keep the unsupported properties & code for them in & commented out so when they are supported it is easy to implement them?

I originally left them commented on the first release, but someone else removed them, so I started to remove them on the next PR's/

The documentation doesn't appear to have been updated with the new resource prefix: azurestack
one unsupported property is still active & in the docs

I knew I missed something, thanks for this

Some of the acceptance tests currently do not pass as required prerequisite resources are not implemented yet. Could we either skip() or remove them?

Had an outage yesterday on the env and for some reason I got pass (?), let me check those and skip the ones that will not pass

…er-azurestack into scale-set

marinsalinas · 2018-08-22T15:44:50Z

Hello @katbyte,

I skipped all the unsupported test and give comments on the reason why were skipped.

Could you check again, please?

katbyte

Hi @thetonymaster,

I have given this another review and left some comments inline. They are all very minor and are mostly about adding some validation to the schema.

Once these have been address I think we can get this merged.

katbyte · 2018-09-06T17:39:15Z

azurestack/resource_arm_virtual_machine_scale_set.go

+				Type:     schema.TypeString,
+				Required: true,
+				ForceNew: true,
+			},


Could we get a ValidateFunc: validation.NoZeroValues, here?

azurestack/resource_arm_virtual_machine_scale_set.go

katbyte · 2018-09-06T17:44:18Z

azurestack/resource_arm_virtual_machine_scale_set.go

+						"capacity": {
+							Type:     schema.TypeInt,
+							Required: true,
+						},


Could we get some validation here? ValidateFunc: validation.IntAtLeast(0), should do it as i think 0 is a valid capacity.

katbyte · 2018-09-06T17:45:25Z

azurestack/resource_arm_virtual_machine_scale_set.go

+			"upgrade_policy_mode": {
+				Type:     schema.TypeString,
+				Required: true,
+			},


Could we get some validation here:

ValidateFunc: validation.StringInSlice([]string{ string(compute.Automatic), string(compute.Manual), string(compute.Rolling), }, true),

katbyte · 2018-09-06T17:57:24Z

azurestack/resource_arm_virtual_machine_scale_set.go

+						"type": {
+							Type:             schema.TypeString,
+							Required:         true,
+							DiffSuppressFunc: ignoreCaseDiffSuppressFunc,


Could we use suppress.CaseDifference here?

katbyte · 2018-09-06T18:14:09Z

azurestack/resource_arm_virtual_machine_scale_set.go

+	}
+
+	err = future.WaitForCompletion(ctx, client.Client)
+	if err != nil {


This could be:

if err := future.WaitForCompletion(ctx, client.Client); err != nil { return err }

katbyte · 2018-09-06T18:14:59Z

azurestack/resource_arm_virtual_machine_scale_set.go

+	}
+
+	err = future.WaitForCompletion(ctx, client.Client)
+	if err != nil {


This could also be

if err := future.WaitForCompletion(ctx, client.Client); err != nil { return err }

katbyte · 2018-09-06T18:31:05Z

website/docs/r/virtual_machine_scale_set.html.markdown

+page_title: "Azure Resource Manager: azurestack_virtual_machine_scale_set"
+sidebar_current: "docs-azurestack-resource-compute-virtualmachine-scale-set"
+description: |-
+  Create a Virtual Machine scale set.


This should be Manages a

katbyte · 2018-09-06T18:31:10Z

website/docs/r/virtual_machine_scale_set.html.markdown

+
+# azurestack\_virtual\_machine\_scale\_set
+
+Create a virtual machine scale set.


This should be Manage a

katbyte · 2018-09-06T18:31:20Z

website/docs/r/virtual_machine_scale_set.html.markdown

+  Create a Virtual Machine scale set.
+---
+
+# azurestack\_virtual\_machine\_scale\_set


this \s are not required and can be removed

katbyte · 2018-09-06T18:42:16Z

Requested changes backported to AzureRM here

katbyte

hi @thetonymaster,

Thank you for the updates, the appear to address all my comments. However you seem to have included some files and packages unrelated to VMSS? specifically event-hub and service bus. I don't see any reason for them to be in this PR.

Also you have switched from the vended azurerm to the azurestack validate & suppress packages, could we swap back and not re-add them?

katbyte · 2018-09-07T18:00:51Z

azurestack/helpers/azure/datalake.go

@@ -0,0 +1,23 @@
+package azure


I'm not sure why this file was added?

katbyte · 2018-09-07T18:00:55Z

azurestack/helpers/azure/eventhub.go

@@ -0,0 +1,145 @@
+package azure


I'm not sure why this file was added?

katbyte · 2018-09-07T18:01:05Z

azurestack/helpers/azure/servicebus.go

@@ -0,0 +1,166 @@
+package azure


I'm not sure why this file was added?

katbyte · 2018-09-07T18:01:45Z

azurestack/helpers/suppress/string.go

@@ -0,0 +1,10 @@
+package suppress


These have been vended in from AzureRM and should not be re-added

katbyte · 2018-09-07T18:01:52Z

azurestack/helpers/suppress/string_test.go

@@ -0,0 +1,51 @@
+package suppress


These have been vended in from AzureRM and should not be re-added

katbyte · 2018-09-07T18:01:59Z

azurestack/helpers/suppress/time.go

@@ -0,0 +1,18 @@
+package suppress


These have been vended in from AzureRM and should not be re-added

katbyte · 2018-09-07T18:02:31Z

azurestack/resource_arm_loadbalancer.go

 	"github.com/terraform-providers/terraform-provider-azurestack/azurestack/helpers/azure"
+	"github.com/terraform-providers/terraform-provider-azurestack/azurestack/helpers/suppress"


These should reference azurerm not azurestack

katbyte · 2018-09-07T18:02:46Z

azurestack/resource_arm_virtual_machine_scale_set.go

@@ -11,6 +11,8 @@ import (
 	"github.com/hashicorp/terraform/helper/schema"
 	"github.com/hashicorp/terraform/helper/structure"
 	"github.com/hashicorp/terraform/helper/validation"
+	"github.com/terraform-providers/terraform-provider-azurestack/azurestack/helpers/azure"
+	"github.com/terraform-providers/terraform-provider-azurestack/azurestack/helpers/suppress"


These should reference azurerm not azurestack

katbyte · 2018-09-07T18:04:11Z

vendor/vendor.json

@@ -38,6 +38,12 @@
 			"revision": "cd93ccfe0395e70031704ca68f14606588eec120",
 			"revisionTime": "2018-06-08T21:25:51Z"
 		},
+		{
+			"checksumSHA1": "FF+JJRL14lbAx8c2s4pixvIulCA=",
+			"path": "github.com/Azure/azure-sdk-for-go/services/eventhub/mgmt/2017-04-01/eventhub",


Why are we updating/adding the event hub package in this PR?

katbyte · 2018-09-07T18:04:16Z

vendor/vendor.json

@@ -50,6 +56,12 @@
 			"revision": "cd93ccfe0395e70031704ca68f14606588eec120",
 			"revisionTime": "2018-06-08T21:25:51Z"
 		},
+		{
+			"checksumSHA1": "/zjoJNfxpxVj22QUxBdBjQcsbHY=",
+			"path": "github.com/Azure/azure-sdk-for-go/services/servicebus/mgmt/2017-04-01/servicebus",


Why are we updating/adding the Service package in this PR?

I vendored the azure package from AzureRM, since it uses (for now) only the generic functions and it keeps getting out of sync. both of the new dependencies come from that package.

katbyte · 2018-09-13T22:12:02Z

tests now pass:

ghost · 2019-06-27T14:54:17Z

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!

thetonymaster added 4 commits July 27, 2018 14:42

Add scale set resource to azurestack provider

8d0df37

Add support for scale set

d6e627a

Remove unsupported fields

c05aca6

Add documentation

338a427

katbyte requested changes Jul 31, 2018

View reviewed changes

katbyte added 2 commits July 31, 2018 11:02

Merge branch 'master' into scale-set

3dd0cea

Merge branch 'master' into scale-set

0f8e33e

katbyte added waiting-response new-resource labels Aug 1, 2018

thetonymaster added 3 commits August 1, 2018 13:18

Changed AzureRM references to AzureStack

78276c9

Merge branch 'scale-set' of github.com:thetonymaster/terraform-provid…

d81af0a

…er-azurestack into scale-set

Add scale set resource to website sidebar template

893b5fb

katbyte added waiting-response and removed waiting-response labels Aug 10, 2018

marinsalinas added 2 commits August 21, 2018 14:36

Merge branch 'master' into scale-set

b556185

Skip unsupported test

fe7b44d

Add import test step on virtual machine scale set resource

133ceee

ghost added the size/XXL label Aug 29, 2018

katbyte removed the new-resource label Sep 6, 2018

katbyte added this to the 0.4.0 milestone Sep 6, 2018

ghost added the size/XXL label Sep 6, 2018

Merge branch 'master' into scale-set

535c87e

katbyte force-pushed the scale-set branch from c7ac5ba to 535c87e Compare September 6, 2018 17:34

ghost added the size/XXL label Sep 6, 2018

katbyte requested changes Sep 6, 2018

View reviewed changes

Add new helpers from AzureRM

b9bde00

thetonymaster added 5 commits September 6, 2018 15:02

Add Autorest dependency and update azure-sdk-for-go

c9850af

Refactor and add validations

6187c29

Add azurestack instead of azurerm helpers

f1ddad3

Fix some issues with documentation for scale set

9a17466

Add comments for not defined enums

cca6574

katbyte requested changes Sep 7, 2018

View reviewed changes

Remove suppress and validate helpers and change reference to vendor

0b5f6f6

thetonymaster force-pushed the scale-set branch from 565fe3b to 0b5f6f6 Compare September 7, 2018 19:42

ghost added the size/XXL label Sep 7, 2018

thetonymaster and others added 5 commits September 7, 2018 14:44

Remove unused files from azure package

6810355

For some reason gofmt complained about this file

8f02d6b

Merge remote-tracking branch 'origin/master' into scale-set

88044d5

Fixed minor typo

cb9ff32

Merge branch 'master' into scale-set

af37227

katbyte force-pushed the scale-set branch from 54ad3ae to 0695f90 Compare September 12, 2018 03:28

ghost added the size/XXL label Sep 12, 2018

katbyte force-pushed the scale-set branch from 0695f90 to af37227 Compare September 12, 2018 04:35

ghost added the size/XXL label Sep 12, 2018

katbyte added 2 commits September 13, 2018 09:23

merge master into scale-set

491f24f

Fix small merge mistake

4f87932

katbyte merged commit d3a0077 into hashicorp:master Sep 13, 2018

katbyte added a commit that referenced this pull request Sep 13, 2018

Update CHANGELOG.md to include #15

a98e28d

ghost locked and limited conversation to collaborators Jun 27, 2019


		"resource_group_name": resourceGroupNameSchema(),

		// "zones": zonesSchema(),


		# azurestack\_virtual\_machine\_scale\_set

		Create a virtual machine scale set.

		"github.com/terraform-providers/terraform-provider-azurestack/azurestack/helpers/azure"
		"github.com/terraform-providers/terraform-provider-azurestack/azurestack/helpers/suppress"

AzureStack Scale set Resource #15

AzureStack Scale set Resource #15

Conversation

thetonymaster commented Jul 30, 2018

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Jul 31, 2018

thetonymaster commented Jul 31, 2018 • edited Loading

marinsalinas commented Aug 22, 2018

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Sep 6, 2018

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Sep 13, 2018

ghost commented Jun 27, 2019

katbyte Jul 31, 2018 •

edited

Loading

thetonymaster commented Jul 31, 2018 •

edited

Loading