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

Add timezone field to Virtual Machine (Windows Configuration) #1265

Merged
merged 14 commits into from
Jun 26, 2018

Conversation

JunyiYi
Copy link

@JunyiYi JunyiYi commented May 19, 2018

This pull request adds the timezone field to the os_profile_windows_config of azurerm_virtual_machine. It also introduces a validation method to verify Azure supported timezone. The timezone list is retrieved from this blog.

Issue #554 will be resolved after this PR is merged.

(fixes #554)

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

Thank you for opening this PR. In addition to the comments I have left inline I've noticed you've not included any tests for the new property.

Look forward to getting this merged for you once some tests are added and the inline questions are answered.

@@ -1291,6 +1300,11 @@ func expandAzureRmVirtualMachineOsProfileWindowsConfig(d *schema.ResourceData) (
config.EnableAutomaticUpdates = &update
}

if v := osProfileConfig["timezone"]; v != nil {
provision := v.(string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor but this could be a single line:

config.TimeZone = utils.String(v.(string))

Copy link
Contributor

@metacpp metacpp May 21, 2018

Choose a reason for hiding this comment

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

2 lines of code is better for debugging, while 1 line of code is simpler @JunyiYi you can make decision.

Copy link
Contributor

Choose a reason for hiding this comment

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

there's no need to split this out - we can make this a single line; there's no value in debugging a pointer reference/dereference

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.


func validateAzureTimeZone() schema.SchemaValidateFunc {
return validation.StringInSlice([]string{
"Dateline Standard Time",
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 also sort this list alphabetically?

Copy link
Author

Choose a reason for hiding this comment

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

This is sorted by time offset (From UTC-12 to UTC+14). For your reference: http://jackstromberg.com/2017/01/list-of-time-zones-consumed-by-azure/

Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize that, but can we sort it alphabetically? Most things in the provider are sorted alphabetically. Sorting by UTC adds a level of unnecessary complexity for maintainers and users alike.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

},
{
// Wrong casing
Value: "china standard time",
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, does case actually matter?

Copy link
Author

@JunyiYi JunyiYi May 22, 2018

Choose a reason for hiding this comment

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

I think a more restricted rule will benefit us unless Azure returns inconsistent values.

Copy link
Contributor

Choose a reason for hiding this comment

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

whilst in principal I'd agree here - the Azure CLI / ARM Templates / PowerShell are case insensitive here - thus we should match the users expectations and make this case insensitive

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

"Tonga Standard Time",
"Samoa Standard Time",
"Line Islands Standard Time",
}, false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you sure the API is case sensitive?

Copy link
Author

Choose a reason for hiding this comment

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

Explained below.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

},
{
// Invalid UTC time zone
Value: "UTC-10",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if azure won't accept it this we probably should not use it as a failure case because it could be confusing.

Copy link
Author

Choose a reason for hiding this comment

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

Let me see whether Azure actually accepts it or not. If so, I will update the validation rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

given Azure could possibly support this as a value - it seems odd to use it as invalid data for test purposes? can we update this to UTC-48 which should never be a valid timezone

Copy link
Author

Choose a reason for hiding this comment

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

Tried it with Azure, and this value is not supported. Only the UTC time offsets listed in my list is accepted by Azure.

@@ -128,3 +128,42 @@ func TestValidateIso8601Duration(t *testing.T) {
}
}
}

func TestValidateAzureTimeZone(t *testing.T) {
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.

minor I would question the need for this test as we are just doing a "is the string in this array" check using the built in validator.

@@ -74,3 +75,114 @@ func validateIso8601Duration() schema.SchemaValidateFunc {
return
}
}

func validateAzureTimeZone() schema.SchemaValidateFunc {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we rename this to validateAzureVmTimeZone()? other APIs might accept different values (the automation one accepts UTC+10 for instance)

Copy link
Author

@JunyiYi JunyiYi May 22, 2018

Choose a reason for hiding this comment

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

I'm trying to make it a general validation rule which could also be used in other timezones (like the one here)

Copy link
Contributor

Choose a reason for hiding this comment

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

given the Azure API's vary so much there's no need to make this generic; in Go the general convention is to value duplication over a completely DRY codebase with extra complexities for switch statements

Copy link
Collaborator

Choose a reason for hiding this comment

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

The property you linked to accepts UTC+10 in it API as well as the portal sets it to an entirely different timezone format. For example PST is "America/Vancouver"

Copy link
Author

Choose a reason for hiding this comment

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

Changed.

@@ -74,3 +75,114 @@ func validateIso8601Duration() schema.SchemaValidateFunc {
return
}
}

func validateAzureTimeZone() schema.SchemaValidateFunc {
return validation.StringInSlice([]string{
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 please get a comment here linking to where this list is from? Are you sure it is accurate?

Type: schema.TypeString,
Optional: true,
ValidateFunc: validateAzureTimeZone(),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

this also needs a state migration to update the resourceArmVirtualMachineStorageOsProfileWindowsConfigHash function and a default value (UTC)

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Is there any samples? I found one here.

Copy link
Contributor

Choose a reason for hiding this comment

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

that'll do as an example, there's a few in the codebase - this value also needs to be added to the Hash function, otherwise this won't be detected as ForceNew for existing Virtual Machines (which is why the state migrations needed)

Copy link
Author

Choose a reason for hiding this comment

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

Added.

@@ -374,6 +374,11 @@ func resourceArmVirtualMachine() *schema.Resource {
Optional: true,
Default: false,
},
"timezone": {
Type: schema.TypeString,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

in the Azure API IIRC this defaults to UTC - we should default that here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

does this also need to be ForceNew, since IIRC this value is used as part of the initial post-sysprep?

Copy link
Contributor

Choose a reason for hiding this comment

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

if not - can we add an example for updating a VM's TimeZone from UTC -> PST?

Copy link
Author

Choose a reason for hiding this comment

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

Tried with Azure, it has to be ForceNew.

@JunyiYi JunyiYi self-assigned this May 22, 2018
@JunyiYi
Copy link
Author

JunyiYi commented May 23, 2018

Hi @katbyte , the test case has been added now.

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

Thank you for the updates. I've left one last comment inline that needs to be addressed before merge.

},
{
// Invalid UTC time zone
Value: "UTC-10",
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 an obviously wrong value like UTC-42 here?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@tombuildsstuff tombuildsstuff modified the milestones: 1.6.0, 1.7.0 May 24, 2018
@JunyiYi
Copy link
Author

JunyiYi commented May 24, 2018

hi @katbyte , the test case has been updated.

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 @JunyiYi

Thanks for pushing those updates - I've taken a look through and left some comments in-line. In particular the state migration needs to be updated to use the Schema Parser rather than trying to parse the schema out by the key prefix (which will break in a future version of Terraform). I've also identified a crash when running this migration - which I've highlighted in-line.

You can test this state migration by deploying a Virtual Machine using v1.6.0 of the AzureRM Provider, and then attempting to re-run terraform apply with this custom version; which will crash at the moment (due to the assumption the field will have a value/lack of nil-checking).

Thanks!

os_profile_windows_config {
timezone = "Pacific Standard Time"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make the spacing consistent here? we use spaces for indentation for terraform configurations

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@@ -76,6 +77,118 @@ func validateIso8601Duration() schema.SchemaValidateFunc {
}
}

func validateAzureVMTimeZone() schema.SchemaValidateFunc {
// Candidates are listed here: http://jackstromberg.com/2017/01/list-of-time-zones-consumed-by-azure/
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a Microsoft source for this information?

Copy link
Author

@JunyiYi JunyiYi May 26, 2018

Choose a reason for hiding this comment

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

Nope. I didn't find any official Azure list. But I tried that list, seems to be the correct one. And of course we don't need to provide the validation here, because the API call will return error.

So what is the best practice? Waiting the API to return error code or provide client-side validation?

@@ -1595,6 +1612,7 @@ func resourceArmVirtualMachineStorageOsProfileWindowsConfigHash(v interface{}) i
if v, ok := m["enable_automatic_upgrades"]; ok {
buf.WriteString(fmt.Sprintf("%t-", v.(bool)))
}
buf.WriteString(fmt.Sprintf("%s-", m["timezone"].(string)))
Copy link
Contributor

Choose a reason for hiding this comment

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

there's a crash where where this field doesn't exist - as such we need to nil-check this.

panic: interface conversion: interface {} is nil, not string
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm:
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: goroutine 32 [running]:
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: github.com/terraform-providers/terraform-provider-azurerm/azurerm.resourceArmVirtualMachineStorageOsProfileWindowsConfigHash(0x25b6c00, 0xc4201a73e0, 0xc420530ab8)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/Users/tharvey/code/src/github.com/terraform-providers/terraform-provider-azurerm/azurerm/resource_arm_virtual_machine.go:1615 +0x3d5
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema.(*Set).hash(0xc420530aa0, 0x25b6c00, 0xc4201a73e0, 0xc420530aa0, 0x0)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/Users/tharvey/code/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/set.go:205 +0x3d
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema.(*Set).add(0xc420530aa0, 0x25b6c00, 0xc4201a73e0, 0x268dc00, 0x1, 0xc420530aa0)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/Users/tharvey/code/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/set.go:192 +0x72
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema.(*Set).Add(0xc420530aa0, 0x25b6c00, 0xc4201a73e0)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/Users/tharvey/code/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/set.go:69 +0x44
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema.NewSet(0x28650d8, 0xc4206f2d40, 0x1, 0x1, 0xc4205309e0)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/Users/tharvey/code/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/set.go:56 +0x7a
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: github.com/terraform-providers/terraform-provider-azurerm/azurerm.resourceArmVirtualMachineRead(0xc4206b1ea0, 0x27afe20, 0xc420360000, 0xc4206b1ea0, 0x0)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/Users/tharvey/code/src/github.com/terraform-providers/terraform-provider-azurerm/azurerm/resource_arm_virtual_machine.go:712 +0x1166
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema.(*Resource).Refresh(0xc4202113b0, 0xc4203ac370, 0x27afe20, 0xc420360000, 0xc4203ea4a8, 0x10bc701, 0x24e1500)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/Users/tharvey/code/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/resource.go:354 +0x167
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema.(*Provider).Refresh(0xc4202205b0, 0xc4203ac320, 0xc4203ac370, 0x1, 0x10623c3, 0x27f0001)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/Users/tharvey/code/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/helper/schema/provider.go:308 +0x9a
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/plugin.(*ResourceProviderServer).Refresh(0xc4205644c0, 0xc4205348e0, 0xc4205349e0, 0x0, 0x0)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/Users/tharvey/code/src/github.com/terraform-providers/terraform-provider-azurerm/vendor/github.com/hashicorp/terraform/plugin/resource_provider.go:549 +0x4e
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: reflect.Value.call(0xc4200924e0, 0xc4201c2238, 0x13, 0x27ed6f9, 0x4, 0xc42069ef18, 0x3, 0x3, 0xc4200b3480, 0x0, ...)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/usr/local/Cellar/go/1.10.1/libexec/src/reflect/value.go:447 +0x969
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: reflect.Value.Call(0xc4200924e0, 0xc4201c2238, 0x13, 0xc420050718, 0x3, 0x3, 0x0, 0x0, 0x0)
2018-05-25T15:52:08.840+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/usr/local/Cellar/go/1.10.1/libexec/src/reflect/value.go:308 +0xa4
2018-05-25T15:52:08.841+0100 [DEBUG] plugin.terraform-provider-azurerm: net/rpc.(*service).call(0xc4203ae2c0, 0xc4206ea370, 0xc4200b0bf0, 0xc4200b0c00, 0xc4201be780, 0xc420564a60, 0x24e14c0, 0xc4205348e0, 0x16, 0x24e1500, ...)
2018-05-25T15:52:08.841+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/usr/local/Cellar/go/1.10.1/libexec/src/net/rpc/server.go:384 +0x14e
2018-05-25T15:52:08.841+0100 [DEBUG] plugin.terraform-provider-azurerm: created by net/rpc.(*Server).ServeCodec
2018-05-25T15:52:08.841+0100 [DEBUG] plugin.terraform-provider-azurerm: 	/usr/local/Cellar/go/1.10.1/libexec/src/net/rpc/server.go:480 +0x43a

Copy link
Author

@JunyiYi JunyiYi May 26, 2018

Choose a reason for hiding this comment

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

But it contains a default value. If the user didn't provide anything, will the default value be populated to the field?

What about the root field using d.Get()? How will Terraform handle the default value?

Copy link
Author

@JunyiYi JunyiYi May 31, 2018

Choose a reason for hiding this comment

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

It is caused by NewSet rather than any Get.

So what is the recommended way of handling default values in Read? Do we just leave it empty or set it to the field?

Copy link
Contributor

Choose a reason for hiding this comment

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

But it contains a default value. If the user didn't provide anything, will the default value be populated to the field?

That's not true for existing values in the state (or, older VM's which didn't expose this field) which won't have this value (and thus will crash) - but we can work around that by nil-checking it

Copy link
Contributor

Choose a reason for hiding this comment

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

given this field is case insensitive in the schema you'll also want to make this value consistent by either upper/lower-casing it; otherwise you'll get spurious diff's here

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

}
}
data["timezone"] = timezone
return resourceArmVirtualMachineStorageOsProfileWindowsConfigHash(data), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

The underlying data structures will diverge significantly in a future version of Terraform - as such direct manipulation of the state will cause hard to diagnose crashes later. Instead we need to use the schema parser (example) rather than assuming the format for keys, (as here)

This means we can simply assign the new value for the field, and allow Terraform Core to recompute the hash value; meaning the tests should stay the same, but all of this can disappear

Copy link
Author

@JunyiYi JunyiYi May 26, 2018

Choose a reason for hiding this comment

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

Nice suggestion. I think this should be mentioned in the guideline as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

(heads up that this needs to be resolved before we can merge this, which is why this is still "requested changes" 😄)

Just to add some additional context to testing this; you'll want to compare the state of a VM using the current provider release (currently v1.6.0) to the current development build you're working on; for instance:

provider "azurerm" {
  version = "=1.6.0"
}

resource "azurerm_resource_group" "test" {
  # ..
}

# ...
$ rm -rf .terraform/
$ terraform init
$ terraform apply

then un-pinning the provider version and re-initing/applying should show the state migration:

provider "azurerm" {
}

resource "azurerm_resource_group" "test" {
  # ..
}

# ...
$ rm -rf .terraform/
$ terraform init # to use the development version you've built via `make`
$ terraform plan|apply

In order to test this effectively unfortunately you'll want to destroy & recreate the VM for each iteration, which the taint command is quite helpful for:

$ terraform taint azurerm_virtual_machine.test
$ # switch back to "1.6.0"
$ terraform apply

Hope that helps :)

Copy link
Author

@JunyiYi JunyiYi Jun 1, 2018

Choose a reason for hiding this comment

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

Thanks for the explanations, and the test steps is really helpful here 👍 . So AFAIK, we can only do the manual test here for this kind of issues? Are there any unit-test infrastructure support it?

Copy link
Contributor

@tombuildsstuff tombuildsstuff Jun 1, 2018

Choose a reason for hiding this comment

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

So AFAIK, we can only do the manual test here for this kind of issues? Are there any unit-test infrastructure support it?

The associated unit test is checking the logic from a given version - this is more exploratory testing / validation (so no, unfortunately not afaik)

for k, v := range toAdd {
is.Attributes[k] = v
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

(which means this function can be removed)

@@ -473,6 +473,7 @@ output "principal_id" {

* `provision_vm_agent` - (Optional) This value defaults to false.
* `enable_automatic_upgrades` - (Optional) This value defaults to false.
* `timezone` - (Optional) Specifies the time zone of the virtual machine. e.g. "Pacific Standard Time".
Copy link
Contributor

Choose a reason for hiding this comment

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

We can suffix this with ". Defaults to UTC"

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@@ -182,3 +182,37 @@ func TestValidateIntBetweenDivisibleBy(t *testing.T) {
}
}
}

func TestValidateAzureVMTimeZone(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

VM -> VirtualMachine (we spell out the resource name for tests)

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

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.

Added some additional comments

@@ -76,6 +77,118 @@ func validateIso8601Duration() schema.SchemaValidateFunc {
}
}

func validateAzureVirtualMachineTimeZone() schema.SchemaValidateFunc {
// Candidates are listed here: http://jackstromberg.com/2017/01/list-of-time-zones-consumed-by-azure/
return validation.StringInSlice([]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

appears this comes from the .net method TimeZoneInfo.FindSystemTimeZoneById() - with this appearing to be a source from Microsoft for the information?

Copy link
Author

Choose a reason for hiding this comment

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

According to my test this is not the full list. For example, Azure also accepts "UTC-02" which is not in the list. And the weird thing is, Azure rejects "UTC-03".

Copy link
Contributor

@tombuildsstuff tombuildsstuff Jun 1, 2018

Choose a reason for hiding this comment

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

tbh that feels like a bug in the API - as if I saw UTC-02 on the list, as an operator I'd assume that UTC-3 or UTC-03 would work?

}
}
data["timezone"] = timezone
return resourceArmVirtualMachineStorageOsProfileWindowsConfigHash(data), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

(heads up that this needs to be resolved before we can merge this, which is why this is still "requested changes" 😄)

Just to add some additional context to testing this; you'll want to compare the state of a VM using the current provider release (currently v1.6.0) to the current development build you're working on; for instance:

provider "azurerm" {
  version = "=1.6.0"
}

resource "azurerm_resource_group" "test" {
  # ..
}

# ...
$ rm -rf .terraform/
$ terraform init
$ terraform apply

then un-pinning the provider version and re-initing/applying should show the state migration:

provider "azurerm" {
}

resource "azurerm_resource_group" "test" {
  # ..
}

# ...
$ rm -rf .terraform/
$ terraform init # to use the development version you've built via `make`
$ terraform plan|apply

In order to test this effectively unfortunately you'll want to destroy & recreate the VM for each iteration, which the taint command is quite helpful for:

$ terraform taint azurerm_virtual_machine.test
$ # switch back to "1.6.0"
$ terraform apply

Hope that helps :)

@@ -1595,6 +1612,7 @@ func resourceArmVirtualMachineStorageOsProfileWindowsConfigHash(v interface{}) i
if v, ok := m["enable_automatic_upgrades"]; ok {
buf.WriteString(fmt.Sprintf("%t-", v.(bool)))
}
buf.WriteString(fmt.Sprintf("%s-", m["timezone"].(string)))
Copy link
Contributor

Choose a reason for hiding this comment

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

given this field is case insensitive in the schema you'll also want to make this value consistent by either upper/lower-casing it; otherwise you'll get spurious diff's here

@@ -473,6 +473,7 @@ output "principal_id" {

* `provision_vm_agent` - (Optional) This value defaults to false.
* `enable_automatic_upgrades` - (Optional) This value defaults to false.
* `timezone` - (Optional) Specifies the time zone of the virtual machine. e.g. `Pacific Standard Time`. Defaults to `UTC`.
Copy link
Contributor

Choose a reason for hiding this comment

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

can we link to the MS page defined above here? e.g.

* `timezone` - (Optional) Specifies the time zone of the Virtual Machine, [the possible values of which are defined here](XXX). Defaults to `UTC`.

Copy link
Author

@JunyiYi JunyiYi Jun 1, 2018

Choose a reason for hiding this comment

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

The problem is I only found a 3-rd party blog which seems to be the correct list (I randomly tried the values in the list, and all are accepted. I also tried several reasonable item which is not list, and all are rejected). Do I still suppose to put it into the doc?

@JunyiYi
Copy link
Author

JunyiYi commented Jun 1, 2018

Hi @tombuildsstuff , I changed the default value of timezone from "UTC" to "" because the value is nil in Azure (not UTC) when you create a virtual machine using the old plugin. In that case, I also removed the state migration code because I tested it with the new plugin reading old state, and there are no changes detected.

The old state migration code actually doesn't work. Because it only changes the timezone in local state file to UTC, while in Azure it is still nil.

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

LGTM

@JunyiYi JunyiYi dismissed stale reviews from katbyte and tombuildsstuff June 8, 2018 21:59

Dismiss the stale review since the logic has been changed.Pineapple Fried Rice

@WodansSon
Copy link
Collaborator

Sound delicious!

@katbyte katbyte modified the milestones: 1.7.0, 1.8.0 Jun 15, 2018
```

$ acctests azurerm TestValidateAzureVirtualMachineTimeZone

=== RUN   TestValidateAzureVirtualMachineTimeZone
--- PASS: TestValidateAzureVirtualMachineTimeZone (0.00s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	0.025s
```
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.

otherwise LGTM - the migration path seems fine now 👍

@@ -76,6 +77,122 @@ func validateIso8601Duration() schema.SchemaValidateFunc {
}
}

func validateAzureVirtualMachineTimeZone(acceptEmpty bool) schema.SchemaValidateFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is only used in a single place, we can assume this is the case - I'll push a commit to remove this.

@tombuildsstuff
Copy link
Contributor

@JunyiYi running the tests for this it appears there's a test failure in one of the new tests:

screenshot 2018-06-22 at 15 27 14

Would you be able to take a look?

Thanks!

@JunyiYi
Copy link
Author

JunyiYi commented Jun 23, 2018

@tombuildsstuff yes I forgot to update the test case after changing the Hash Function. It is fixed in my last commit.

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.

LGTM 👍

@tombuildsstuff
Copy link
Contributor

Ignoring a transitive test failure, the tests pass:

screenshot 2018-06-26 at 16 02 09

@tombuildsstuff tombuildsstuff merged commit f1403fa into master Jun 26, 2018
@tombuildsstuff tombuildsstuff deleted the add_vm_timezone branch June 26, 2018 14:08
tombuildsstuff added a commit that referenced this pull request Jun 26, 2018
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Missing timeZone from os_profile_windows_config in azurerm_virtual_machine
5 participants