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

Additional VMSS properties (issues #1210 & #1226 #1209

Merged
merged 2 commits into from
Jun 5, 2018

Conversation

djsly
Copy link
Contributor

@djsly djsly commented May 7, 2018

  1. Add the support for dns_servers when creating a VMScaleSet
  2. add the support for ip_forwarding when creating a VMScaleSet

Fix for #1210 and #1226

@WodansSon
Copy link
Collaborator

hey @djsly

Thanks for opening this PR :)

I've taken a look through and notice you haven't implemented any test cases for your changes. Can you please add some tests for the new properties you have exposed?

Thanks!

@katbyte katbyte added enhancement service/vmss Virtual Machine Scale Sets labels May 7, 2018
@katbyte katbyte changed the title Issue 901 Additional VMSS properties (issues #910 & #1210) May 7, 2018
@djsly
Copy link
Contributor Author

djsly commented May 7, 2018

@jeffreyCline: Definitely.

@djsly djsly changed the title Additional VMSS properties (issues #910 & #1210) Additional VMSS properties (issues #910, #1210 & #1226) May 11, 2018
@djsly
Copy link
Contributor Author

djsly commented May 11, 2018

@jeffreyCline unit test added.

@djsly
Copy link
Contributor Author

djsly commented May 11, 2018

/assign @tombuildsstuff

@djsly
Copy link
Contributor Author

djsly commented May 11, 2018

/retest

@katbyte
Copy link
Collaborator

katbyte commented May 11, 2018

Hello @djsly,

Thank you for this contribution, it seems that there currently are some issues with the tests:

azurerm/resource_arm_virtual_machine_scale_set_test.go:1292: Sprintf call needs 8 args but has 9 args
azurerm/resource_arm_virtual_machine_scale_set_test.go:1377: Sprintf call needs 8 args but has 9 args

@djsly
Copy link
Contributor Author

djsly commented May 11, 2018

Hi @katbyte thanks for reporting that! I think that I will need to review my IDE since it didn't give me the errors that you found...

Same for the travis-ci ob and the make test :(

The tests were updated accordingly.

Hopefully all is good now.

@djsly
Copy link
Contributor Author

djsly commented May 14, 2018

Hello @katbyte and @jeffreyCline , let me know if you guys need something else from my part. This is my first time I'm not fully up part with Harshicorp's PR process yet.

@katbyte katbyte self-requested a review May 14, 2018 23:20
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 @djsly,

Thank you for opening this PR. Generally it looks good but I've left some comments in line.

However with respect to the vm_hostnames, vm_statuses and vm_primary_private_ip_addresses properties we don't feel that the terraform resource is necessarily the right place to expose this data as they will constantly be changing as VMs in the scale set spin up and down.

@@ -583,6 +605,24 @@ func resourceArmVirtualMachineScaleSet() *schema.Resource {
},

"tags": tagsSchema(),

"vm_hostnames": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These properties do not make much sense to expose in a resource as they will always be changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where would you have them? Those are needed when not using the auto scaling feature of the scaleSet; therefore specifying a predefined number of VMs. We need to run remote-exec provisioner on the resulting VMs so the IP are important to us, as well as the hostnames since we pass the terraform output to a secondary script which consumes the hostnames.

Should we put this in a Datasource instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

starting a review as I need the feature (originally asked in the original issue #910)

Looking for a OK to implement this as a DataSource instead before starting the work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@katbyte can I proceed with the DataSource implementation instead? will that fly by on your side ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@katbyte do you know if a data source can reference a resource that Terraform is creating ? or it only supports preexisting resources not managed by terraform?

Copy link
Collaborator

@katbyte katbyte May 19, 2018

Choose a reason for hiding this comment

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

@djsly, a datasource can reference anything you would like, terraform created or not. However you just have to make sure the graph knows about the dependancy by linking back to the terraform resource:

resource "r" "example" {
  name = "name"
}

data "d" "example" {
  name = "${r.example.name}"
}

With respect to these additional attributes can we please remove them from this PR? That really is separate task so lets move the discussion to the existing issue (#901) so my colleagues can weigh in 🙂

Thanks!

@@ -1317,7 +1441,19 @@ func expandAzureRmVirtualMachineScaleSetNetworkProfile(d *schema.ResourceData) *
name := config["name"].(string)
primary := config["primary"].(bool)
acceleratedNetworking := config["accelerated_networking"].(bool)

ipForwarding := config["ip_forwarding"].(bool)
dns_settings := config["dns_settings"].(*schema.Set).List()[0].(map[string]interface{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the dns settings block doesn't exist this could fail. You should do a check:

if v := config["dns_settings"].(*schema.Set).List(); v.Len() > 0 {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch @katbyte ! I have the fix Already since I got hit by that exact same problem :). I will be pushing over my changes soon after testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested with and without the optional dns_settings block, all is working as expected now.

@@ -1409,6 +1545,8 @@ func expandAzureRmVirtualMachineScaleSetNetworkProfile(d *schema.ResourceData) *
Primary: &primary,
IPConfigurations: &ipConfigurations,
EnableAcceleratedNetworking: &acceleratedNetworking,
EnableIPForwarding: &ipForwarding,
DNSSettings: &dnsSettings,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we only set DNSSettings to an object when dns_servers is populated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will check if this is easily doable, but from the look of it, all other optional settings are always set: e.g. primary, acceleratedNetworking, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally we don't as some API's will break when empty objects are sent, its more a proactive choice to minimize that chance. It is easy enough by just creating the object above, and then only setting the field if required. However if the API is ok with an empty DNSSettings object then its not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested and the API support it , for now I left it the same.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

* `network_security_group_id` - (Optional) Specifies the identifier for the network security group.
* `accelerated_networking` - (Optional) Specifies whether to enable accelerated networking or not. Defaults to
`false`.
* `ip_forwarding` - (Optional) Specifies whether to enable ip forwarding or not. Defaults to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we move this up so the list is sorted alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the list is currently not in any alphabetical order, want me to reorder the full list ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ordered them in Required First, followed by Optional both following an alphabetical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in next

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks! We usually sort required -> optional -> computed and then alphabetically with name location and resource group at the top.

@@ -352,9 +352,12 @@ resource "azurerm_virtual_machine_scale_set" "test" {
* `name` - (Required) Specifies the name of the network interface configuration.
* `primary` - (Required) Indicates whether network interfaces created from the network interface configuration will be the primary NIC of the VM.
* `ip_configuration` - (Required) An ip_configuration block as documented below
* `dns_settings` - (Optional) An dns_settings block as documented below
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line should end with a .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in next

@@ -367,6 +370,10 @@ resource "azurerm_virtual_machine_scale_set" "test" {
* `public_ip_address_configuration` - (Optional) describes a virtual machines scale set IP Configuration's
PublicIPAddress configuration. The public_ip_address_configuration is documented below.

`dns_settings` supports the following:

* `dns_servers` - (Required) Specifies an array of dns servers
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line should end with a .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in next

testCheckAzureRMVirtualMachineScaleSetSinglePlacementGroup("azurerm_virtual_machine_scale_set.test", true),
testCheckAzureRMVirtualMachineScaleSetIPForwarding("azurerm_virtual_machine_scale_set.test", false),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unless there is a good reason not to, I think we should be just checking the state here:

resource.TestCheckResourceAttr(resourceName, "ip_forwarding", "false"),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For my personal knowledge, what is the difference between the current test VS checking for the state only? The current methods queries the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After review, indeed the diff is an API call on the remote resource VS what terraform stored in its state.

I don't mind converting to resource.TestCheckResourceAttr this will probably speed up the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in next

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!

The reason for this is we want to verify that the create/update and read functions. If we call out to the API we are only checking one direction, and not ensuring that the provider read it back (or imported) into the state correctly.

@@ -24,9 +25,12 @@ func TestAccAzureRMVirtualMachineScaleSet_basic(t *testing.T) {
Config: config,
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMVirtualMachineScaleSetExists("azurerm_virtual_machine_scale_set.test"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

an aside as this is the way it was, we should probably set resourceName to "azurerm_virtual_machine_scale_set.test" and pass that around instead of hardcoding it everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can clean that up np.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in next

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you!

Config: config,
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMVirtualMachineScaleSetExists(resource_name),
testCheckAzureRMVirtualMachineScaleSetIPForwarding(resource_name, true),
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above, unless there is a good reason to do this I think we should be just checking the state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reasoning as above, I will convert to state check to same on exec time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in next

Config: config,
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMVirtualMachineScaleSetExists(resource_name),
testCheckAzureRMVirtualMachineScaleSetDNSSettings(resource_name, []string{"8.8.8.8", "8.8.4.4"}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above, unless there is a good reason to do this I think we should be just checking the state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reasoning as above, I will convert to state check to same on exec time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in next

@djsly
Copy link
Contributor Author

djsly commented May 17, 2018

@katbyte I have pushed all of the changes Except for the migration of the scale set properties to a data source. Waiting on your approval for acceptance to start the work.

@katbyte
Copy link
Collaborator

katbyte commented May 19, 2018

In case you missed it I left a comment inline, in short lets remove the computed fields as mentioned from this PR and move the discussion on creating a datasource for the to #901, thanks!

@djsly djsly changed the title Additional VMSS properties (issues #910, #1210 & #1226) Additional VMSS properties (issues #1210, #1226 & #1249) May 22, 2018
@djsly
Copy link
Contributor Author

djsly commented May 22, 2018

@katbyte I removed the code change for #901 and added the support for priority

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 @djsly,

Thank you for moving those properties out! However I see you've added another ticket to the PR. It is best to not lump multiple issues into the same PR or change functionality during the review process unless necessary or the issues are tightly coupled.

The simpler the PRs can be the quicker we can review them 🙂

The priority property is already being added by another community PR (#1249) so could we please remove it in favour of that on? Thanks.

ipForwarding := config["ip_forwarding"].(bool)

dnsSettingsConfigs := config["dns_settings"].(*schema.Set).List()
//(*schema.Set).List()[0].(map[string]interface{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason this comment was left in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in next

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch.

@@ -115,6 +115,12 @@ func resourceArmVirtualMachineScaleSet() *schema.Resource {
ForceNew: true,
},

"priority": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There already exists a PR for this property predating this commit: #1250

Could we remove it in favour of that one please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, I saw it after I pushed. Since it didn't seem like the Original Author was responding I decided to keep it in here.
Removed in next.

@@ -12,6 +12,8 @@ import (
"github.com/hashicorp/terraform/terraform"
)

const scaleSetResourceName = "azurerm_virtual_machine_scale_set.test"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand the motive but as this ends up being a global constant we tend not to do this. Could we please revert this and use the existing pattern for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Katy, this was updated per your request. You wanted to move the strings out of the tests into a single var. Please comment on which you are favoring, duplication or const var.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the tests, they should be move self-content and unified now.

@@ -352,11 +354,18 @@ resource "azurerm_virtual_machine_scale_set" "test" {

* `name` - (Required) Specifies the name of the network interface configuration.
* `primary` - (Required) Indicates whether network interfaces created from the network interface configuration will be the primary NIC of the VM.
* `ip_configuration` - (Required) An ip_configuration block as documented below
* `ip_configuration` - (Required) An ip_configuration block as documented below.
* `accelerated_networking` - (Optional) Specifies whether to enable accelerated networking or not. Defaults to
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we get this on a single line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in next

* `ip_configuration` - (Required) An ip_configuration block as documented below.
* `accelerated_networking` - (Optional) Specifies whether to enable accelerated networking or not. Defaults to
`false`.
* `dns_settings` - (Optional) An dns_settings block as documented below.
* `network_security_group_id` - (Optional) Specifies the identifier for the network security group.
* `accelerated_networking` - (Optional) Specifies whether to enable accelerated networking or not. Defaults to
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to be duplicated above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in next

@djsly djsly changed the title Additional VMSS properties (issues #1210, #1226 & #1249) Additional VMSS properties (issues #1210 & #1226 May 23, 2018
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.

@djsly,

Thank you for the updates, This LGTM! Provided the test pass I'll merge it shortly :)

One thing however, we manually update the change-log ourselves. As such I have reverted that change to expedite getting this in for you.

@djsly
Copy link
Contributor Author

djsly commented May 23, 2018 via email

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.

Hey @djsly,

Running the tests showed some errors so i am going to have to request more changes. Here are the errors:

------- Stdout: -------
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basic
--- FAIL: TestAccAzureRMVirtualMachineScaleSet_basic (358.21s)
    testing.go:513: Step 0 error: Check failed: Check 2/6 error: azurerm_virtual_machine_scale_set.test: Attribute 'vm_hostnames' expected to be set
FAIL
------- Stdout: -------
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicDNSSettings
--- FAIL: TestAccAzureRMVirtualMachineScaleSet_basicDNSSettings (367.35s)
    testing.go:513: Step 0 error: Check failed: Check 2/3 error: azurerm_virtual_machine_scale_set.test: Attribute 'dns_settings.0.dns_servers.0' not found
FAIL
------- Stdout: -------
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicIPForwarding
--- FAIL: TestAccAzureRMVirtualMachineScaleSet_basicIPForwarding (423.10s)
    testing.go:513: Step 0 error: Check failed: Check 2/2 error: azurerm_virtual_machine_scale_set.test: Attribute 'ip_forwarding' not found
FAIL

I've left some comments inline as well highlighting what i think some of the failures are caused by.

// single placement group should default to true
testCheckAzureRMVirtualMachineScaleSetSinglePlacementGroup("azurerm_virtual_machine_scale_set.test", true),
testCheckAzureRMVirtualMachineScaleSetExists(resourceName),
resource.TestCheckResourceAttrSet(resourceName, "vm_hostnames"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are testing attributes you have removed and causing tests to fail.

Config: config,
Check: resource.ComposeTestCheckFunc(
testCheckAzureRMVirtualMachineScaleSetExists(resourceName),
resource.TestCheckResourceAttr(resourceName, "dns_settings.0.dns_servers.0", "8.8.8.8"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have dns_settings as a TypeSet so the .0. syntax will not work. You'll need to change to a TypeList

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @katbyte while this would help with the test, TypeSet is the right fit here since we can only have a single dns_settings block. Same logic as for the network_profile block.

While debugging I realized that using the

since the full string would be something like this, as you can see the dns_settings and the ip_forwarding are a child element of the network_profile with TypeSet, therefore I won't be able to use the resource.TestCheckResourceAttr logic unless you know of another method that supports sets

"network_profile.4108483151.ip_forwarding": "true",

Copy link
Collaborator

Choose a reason for hiding this comment

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

Typically unless there is a reason to, single element blocks like this are always a TypeList as diffs are simpler, easier to get at the attributes and state migrations are not required when adding/changing the fields.

Copy link
Contributor Author

@djsly djsly May 24, 2018

Choose a reason for hiding this comment

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

ok, while changing the dns_settings to TypeList will help, what should we do with network_profile ?

  1. convert to TypeList
  2. find a way to generate the hash in the test.

"network_security_group_id": {
Type: schema.TypeString,
Optional: true,
},

"dns_settings": {
Type: schema.TypeSet,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your tests are using the .0 list syntax while this is a TypeSet. I suggest changing this to a a TypeList

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using the same approach, I would need to convert the network_profile to a TypeList to allow the test to work. Is this acceptable? I feel that we are using the wrong data type for to be compatible with the test harness. Both dns_settings and network_profile should be Set with MaxItem = 1 in my opinion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep! I would prefer a TypeList here for the above reasons. A type set is only really useful when you have >1 item, order doesn't matter and duplicates are not allowed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with your current description of TypeSet, network_profile falls indeed into a TypeSet. Therefore I won't be able to resource.TestCheckResourceAttr(resourceName, "network_profile.<hashstring>.dns_settings.0.dns_servers.0", "8.8.8.8"),

unless I try to generate the right hash.

network_profile - (Required) A collection of network profile block as documented below.

network_profile supports the following:

name - (Required) Specifies the name of the network interface configuration.
primary - (Required) Indicates whether network interfaces created from the network interface configuration will be the primary NIC of the VM.
ip_configuration - (Required) An ip_configuration block as documented below
network_security_group_id - (Optional) Specifies the identifier for the network security group.
accelerated_networking - (Optional) Specifies whether to enable accelerated networking or not. Defaults to false.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh i didn't realize there could be multiple network_profile blocks, then yes in this case it should be a TypeSet. I believe you can turn debug on to see the state during the tests to grab the actual hash to do a proper check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, network_profile was kept a TypeSet while dns_settings was converted to a TypeList`, the ACCtests were updated accordingly

@djsly
Copy link
Contributor Author

djsly commented May 23, 2018 via email

@djsly
Copy link
Contributor Author

djsly commented May 29, 2018

@katbyte ok I think it should all be good now

17:39 $ make testacc TEST=./azurerm TESTARGS='-run=TestAccAzureRMVirtualMachineScaleSet_basicDNSSettings'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -run=TestAccAzureRMVirtualMachineScaleSet_basicDNSSettings -timeout 180m
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicDNSSettings
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicDNSSettings (433.41s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm       433.438s
17:33 $ make testacc TEST=./azurerm TESTARGS='-run=TestAccAzureRMVirtualMachineScaleSet_basicIPForwarding'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -run=TestAccAzureRMVirtualMachineScaleSet_basicIPForwarding -timeout 180m
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicIPForwarding
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicIPForwarding (346.28s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm       346.318s

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.

@djsly,

LGTM except taking one last look at the tests and noticed a strange failure. seems by switching to %[] format for some of the values in the format string and not for others introduced some bugs:

[14:09:29] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize -timeout 180m
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize
--- FAIL: TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize (114.51s)
	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="InvalidParameter" Message="The requested VM size eastus is not available in the current region. The sizes available in the current region are: Standard_B1ms,Standard_B1s,Standard_B2ms,Standard_B2s,Standard_B4ms,Standard_B8ms,Standard_DS1_v2,Standard_DS2_v2,Standard_DS3_v2,Standard_DS4_v2,Standard_DS5_v2,Standard_DS11-1_v2,Standard_DS11_v2,Standard_DS12-1_v2,Standard_DS12-2_v2,Standard_DS12_v2,Standard_DS13-2_v2,Standard_DS13-4_v2,Standard_DS13_v2,Standard_DS14-4_v2,Standard_DS14-8_v2,Standard_DS14_v2,Standard_DS15_v2,Standard_DS2_v2_Promo,Standard_DS3_v2_Promo,Standard_DS4_v2_Promo,Standard_DS5_v2_Promo,Standard_DS11_v2_Promo,Standard_DS12_v2_Promo,Standard_DS13_v2_Promo,Standard_DS14_v2_Promo,Standard_F1s,Standard_F2s,Standard_F4s,Standard_F8s,Standard_F16s,Standard_D2s_v3,Standard_D4s_v3,Standard_D8s_v3,Standard_D16s_v3,Standard_D32s_v3,Standard_A0,Standard_A1,Standard_A2,Standard_A3,Standard_A5,Standard_A4,Standard_A6,Standard_A7,Basic_A0,Basic_A1,Basic_A2,Basic_A3,Basic_A4,Standard_D1_v2,Standard_D2_v2,Standard_D3_v2,Standard_D4_v2,Standard_D5_v2,Standard_D11_v2,Standard_D12_v2,Standard_D13_v2,Standard_D14_v2,Standard_D15_v2,Standard_D2_v2_Promo,Standard_D3_v2_Promo,Standard_D4_v2_Promo,Standard_D5_v2_Promo,Standard_D11_v2_Promo,Standard_D12_v2_Promo,Standard_D13_v2_Promo,Standard_D14_v2_Promo,Standard_F1,Standard_F2,Standard_F4,Standard_F8,Standard_F16,Standard_A1_v2,Standard_A2m_v2,Standard_A2_v2,Standard_A4m_v2,Standard_A4_v2,Standard_A8m_v2,Standard_A8_v2,Standard_D2_v3,Standard_D4_v3,Standard_D8_v3,Standard_D16_v3,Standard_D32_v3,Standard_H8,Standard_H16,Standard_H8m,Standard_H16m,Standard_H16r,Standard_H16mr,Standard_D1,Standard_D2,Standard_D3,Standard_D4,Standard_D11,Standard_D12,Standard_D13,Standard_D14,Standard_NV6,Standard_NV12,Standard_NV24,Standard_NC6s_v2,Standard_NC12s_v2,Standard_NC24rs_v2,Standard_NC24s_v2,Standard_NC6,Standard_NC12,Standard_NC24,Standard_NC24r,Standard_F2s_v2,Standard_F4s_v2,Standard_F8s_v2,Standard_F16s_v2,Standard_F32s_v2,Standard_F64s_v2,Standard_F72s_v2,Standard_DS1,Standard_DS2,Standard_DS3,Standard_DS4,Standard_DS11,Standard_DS12,Standard_DS13,Standard_DS14,Standard_D64_v3,Standard_D64s_v3,Standard_E2_v3,Standard_E4_v3,Standard_E8_v3,Standard_E16_v3,Standard_E32_v3,Standard_E64i_v3,Standard_E64_v3,Standard_E2s_v3,Standard_E4-2s_v3,Standard_E4s_v3,Standard_E8-2s_v3,Standard_E8-4s_v3,Standard_E8s_v3,Standard_E16-4s_v3,Standard_E16-8s_v3,Standard_E16s_v3,Standard_E32-8s_v3,Standard_E32-16s_v3,Standard_E32s_v3,Standard_E64-16s_v3,Standard_E64-32s_v3,Standard_E64is_v3,Standard_E64s_v3,Standard_NC6s_v3,Standard_NC12s_v3,Standard_NC24rs_v3,Standard_NC24s_v3,Standard_L8s_v2,Standard_L16s_v2,Standard_ND6s,Standard_ND12s,Standard_ND24rs,Standard_ND24s,Standard_A8,Standard_A9,Standard_A10,Standard_A11,Standard_M8-2ms,Standard_M8-4ms,Standard_M8ms,Standard_M16-4ms,Standard_M16-8ms,Standard_M16ms,Standard_M32-8ms,Standard_M32-16ms,Standard_M32ls,Standard_M32ms,Standard_M32ts,Standard_M64-16ms,Standard_M64-32ms,Standard_M64ls,Standard_M64ms,Standard_M64s,Standard_M128-32ms,Standard_M128-64ms,Standard_M128ms,Standard_M128s,Standard_M64,Standard_M64m,Standard_M128,Standard_M128m.\r\nFind out more on the available VM sizes in each region at https://aka.ms/azure-regions." Target="sku.name"
FAIL
FAIL	github.com/terraform-providers/terraform-provider-azurerm/azurerm	114.543s
make: *** [testacc] Error 1

Changing the %ss to %[#]s seems to fix the issue:

[14:11:41] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$ make fmt; make testacc TEST=./azurerm TESTARGS=-test.run=TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize
gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize -timeout 180m
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize (581.52s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	581.554s
[16:30:49] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$

If your going to change all of them to the index format, could you place switch every placeholder to have the positional argument.

here is the change i made:

[16:50:42] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$ git diff
diff --git a/azurerm/resource_arm_virtual_machine_scale_set_test.go b/azurerm/resource_arm_virtual_machine_scale_set_test.go
index 54e017d8..0f196e51 100644
--- a/azurerm/resource_arm_virtual_machine_scale_set_test.go
+++ b/azurerm/resource_arm_virtual_machine_scale_set_test.go
@@ -2256,7 +2256,7 @@ func testAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk(rInt int, loc
        return fmt.Sprintf(`
 resource "azurerm_resource_group" "test" {
     name = "acctestRG-%[1]d"
-    location = "%s"
+    location = "%[2]s"
 }

 resource "azurerm_virtual_network" "test" {
@@ -2280,7 +2280,7 @@ resource "azurerm_virtual_machine_scale_set" "test" {
   upgrade_policy_mode = "Manual"

   sku {
-    name = "%s"
+    name = "%[3]s"
     tier = "Standard"
     capacity = 2
   }
[16:50:44] kt@snowbook:~/hashi/..3../terraform-providers/terraform-provider-azurerm$

@djsly
Copy link
Contributor Author

djsly commented Jun 5, 2018

Thanks @katbyte , it will teach me to read the DOC on more details when using new syntax that I'm not used too (copying other's people code can do that ...)

Explicit argument indexes:

In Printf, Sprintf, and Fprintf, the default behavior is for each formatting verb to format successive arguments passed in the call. However, the notation [n] immediately before the verb indicates that the nth one-indexed argument is to be formatted instead. The same notation before a '*' for a width or precision selects the argument index holding the value. After processing a bracketed expression [n], subsequent verbs will use arguments n+1, n+2, etc. unless otherwise directed.

@djsly
Copy link
Contributor Author

djsly commented Jun 5, 2018

08:11 $ make testacc TEST=./azurerm TESTARGS='-run=TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -run=TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk -timeout 180m
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk (533.03s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicWindows_managedDisk_resize (638.28s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm       1171.337s

and the following which seems to be a known issue.

07:35 $ make testacc TEST=./azurerm TESTARGS='-run=TestAccAzureRMVirtualMachineScaleSet_customImage'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -run=TestAccAzureRMVirtualMachineScaleSet_customImage -timeout 180m
=== RUN   TestAccAzureRMVirtualMachineScaleSet_customImage
--- FAIL: TestAccAzureRMVirtualMachineScaleSet_customImage (239.23s)
        testing.go:513: Step 1 error: Error refreshing: 1 error(s) occurred:

                * data.azurerm_image.test: 1 error(s) occurred:

                * data.azurerm_image.test: data.azurerm_image.test: Error: Image "accteste-6769746892753897977" (Resource Group "acctestRG-6769746892753897977") was not found
FAIL
exit status 1
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm       239.256s
make: *** [testacc] Error 1

@djsly
Copy link
Contributor Author

djsly commented Jun 5, 2018

@katbyte I pushed a fix for the test that was failling.

09:39 $ make testacc TF_LOG_PATH=./tf.log TF_LOG=DEBUG TEST=./azurerm TESTARGS='-run=TestAccAzureRMVirtualMachineScaleSet_customImage'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -run=TestAccAzureRMVirtualMachineScaleSet_customImage -timeout 180m
=== RUN   TestAccAzureRMVirtualMachineScaleSet_customImage
--- PASS: TestAccAzureRMVirtualMachineScaleSet_customImage (760.64s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm       760.673s

@djsly
Copy link
Contributor Author

djsly commented Jun 5, 2018

DataSource cannot be used with resources that are managed within the same run unless you specify a depends_on but even that isn't working properly due to some destroy problem in dependencies. I couldn't find a rationale to use a data source to extract the .id of the image since it is exposed at the resource level. For this reason, I removed the usage of the data source.

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.

@djsly,

Thank you for fixing that! LGTM now 👍

@katbyte
Copy link
Collaborator

katbyte commented Jun 5, 2018

Tests pass:

screen shot 2018-06-05 at 10 55 25

@katbyte katbyte added this to the 1.7.0 milestone Jun 5, 2018
@katbyte katbyte merged commit 4683093 into hashicorp:master Jun 5, 2018
@djsly djsly deleted the issue-901 branch June 5, 2018 22:58
katbyte added a commit that referenced this pull request Jun 5, 2018
@katbyte
Copy link
Collaborator

katbyte commented Jun 16, 2018

Hey @djsly,

Just a friendly heads up 1.7 including this PR has been released !

@ghost
Copy link

ghost commented Mar 30, 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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement service/vmss Virtual Machine Scale Sets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants