-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Conversation
There was a problem hiding this 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) |
There was a problem hiding this comment.
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))
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
azurerm/validators.go
Outdated
|
||
func validateAzureTimeZone() schema.SchemaValidateFunc { | ||
return validation.StringInSlice([]string{ | ||
"Dateline Standard Time", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
azurerm/validators_test.go
Outdated
}, | ||
{ | ||
// Wrong casing | ||
Value: "china standard time", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
azurerm/validators.go
Outdated
"Tonga Standard Time", | ||
"Samoa Standard Time", | ||
"Line Islands Standard Time", | ||
}, false) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explained below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
azurerm/validators_test.go
Outdated
}, | ||
{ | ||
// Invalid UTC time zone | ||
Value: "UTC-10", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
azurerm/validators_test.go
Outdated
@@ -128,3 +128,42 @@ func TestValidateIso8601Duration(t *testing.T) { | |||
} | |||
} | |||
} | |||
|
|||
func TestValidateAzureTimeZone(t *testing.T) { |
There was a problem hiding this comment.
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.
azurerm/validators.go
Outdated
@@ -74,3 +75,114 @@ func validateIso8601Duration() schema.SchemaValidateFunc { | |||
return | |||
} | |||
} | |||
|
|||
func validateAzureTimeZone() schema.SchemaValidateFunc { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
azurerm/validators.go
Outdated
@@ -74,3 +75,114 @@ func validateIso8601Duration() schema.SchemaValidateFunc { | |||
return | |||
} | |||
} | |||
|
|||
func validateAzureTimeZone() schema.SchemaValidateFunc { | |||
return validation.StringInSlice([]string{ |
There was a problem hiding this comment.
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(), | ||
}, |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
Hi @katbyte , the test case has been added now. |
There was a problem hiding this 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.
azurerm/validators_test.go
Outdated
}, | ||
{ | ||
// Invalid UTC time zone | ||
Value: "UTC-10", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
hi @katbyte , the test case has been updated. |
There was a problem hiding this 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" | ||
} | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
azurerm/validators.go
Outdated
@@ -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/ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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))) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 | ||
} | ||
} |
There was a problem hiding this comment.
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". |
There was a problem hiding this comment.
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
"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
azurerm/validators_test.go
Outdated
@@ -182,3 +182,37 @@ func TestValidateIntBetweenDivisibleBy(t *testing.T) { | |||
} | |||
} | |||
} | |||
|
|||
func TestValidateAzureVMTimeZone(t *testing.T) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this 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
azurerm/validators.go
Outdated
@@ -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{ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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))) |
There was a problem hiding this comment.
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`. |
There was a problem hiding this comment.
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`.
There was a problem hiding this comment.
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?
Hi @tombuildsstuff , I changed the default value of The old state migration code actually doesn't work. Because it only changes the timezone in local state file to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Dismiss the stale review since the logic has been changed.Pineapple Fried Rice
Sound delicious! |
``` $ acctests azurerm TestValidateAzureVirtualMachineTimeZone === RUN TestValidateAzureVirtualMachineTimeZone --- PASS: TestValidateAzureVirtualMachineTimeZone (0.00s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 0.025s ```
There was a problem hiding this 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 👍
azurerm/validators.go
Outdated
@@ -76,6 +77,122 @@ func validateIso8601Duration() schema.SchemaValidateFunc { | |||
} | |||
} | |||
|
|||
func validateAzureVirtualMachineTimeZone(acceptEmpty bool) schema.SchemaValidateFunc { |
There was a problem hiding this comment.
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.
@JunyiYi running the tests for this it appears there's a test failure in one of the new tests: Would you be able to take a look? Thanks! |
@tombuildsstuff yes I forgot to update the test case after changing the Hash Function. It is fixed in my last commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
This pull request adds the
timezone
field to theos_profile_windows_config
ofazurerm_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)