-
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
[Image] zone resilient property addition #3100
Conversation
This is not yet ready to merge. I am having issues running the integration tests seeing the following:
Seems to be something local to my branch, I am working on 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.
hey @Lucretius
Thanks for this PR :)
Taking a look through besides a couple of minor changes this otherwise LGTM, if we can get those fixed up then this should be good to merge 👍
Thanks!
azurerm/resource_arm_image_test.go
Outdated
|
||
// Region list pulled from https://docs.microsoft.com/en-us/azure/storage/common/storage-redundancy-zrs | ||
func supportsZRS(location string) bool { | ||
zrsRegions := []string{"westus2", "francecentral", "southeastasia", "westeurope", "northeurope", "japaneast", "uksouth", "eastus", "eastus2", "centralus"} |
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 only covers Azure Public (there's different Azure Regions which are supported e.g. Government, China, Germany) - as such I think we'd be best to remove this for the moment? (we're running the tests in some of these regions)
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, my concern had been that I did not know in which locations these tests were going to be run, and did not want to have an acceptance test failure simply due to the fact that ZRS storage is not supported in some locations.
@@ -40,6 +40,11 @@ func dataSourceArmImage() *schema.Resource { | |||
|
|||
"location": locationForDataSourceSchema(), | |||
|
|||
"zone_resilient": { |
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 set this field in the state here: https://github.com/terraform-providers/terraform-provider-azurerm/pull/3100/files#diff-21521ec4fe0d38353d87860312cd25a4R187
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.
Good catch thanks!
@@ -35,6 +35,13 @@ func resourceArmImage() *schema.Resource { | |||
|
|||
"resource_group_name": resourceGroupNameSchema(), | |||
|
|||
"zone_resilient": { |
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 Set this in the state here using the object returned from the API? https://github.com/terraform-providers/terraform-provider-azurerm/pull/3100/files#diff-95b4abb1c5040a8fd4f3c3674821b7b6R292
website/docs/r/image.html.markdown
Outdated
@@ -58,11 +58,14 @@ The following arguments are supported: | |||
the image. Changing this forces a new resource to be created. | |||
* `location` - (Required) Specified the supported Azure location where the resource exists. | |||
Changing this forces a new resource to be created. | |||
* `zone_resilient` - (Optional) Is zone resiliency enabled? Defaults to `false`. Changing this forces a new resource to be created. |
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're trying to keep these sorted Required -> Optional and then Alphabetically within the fields; could we move this to below Tags (and above the Note
?)
Co-Authored-By: Lucretius <rlippens39@gmail.com>
Ah nice catch @katbyte - I think I got mixed up looking at another resource which had the |
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.
hey @Lucretius
Thanks for pushing those changes - sorry for the delayed re-review here! Taking a look through this now LGTM 👍
Thanks!
dismissing since changes have been pushed
This has been released in version 1.24.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 1.24.0"
}
# ... other configuration ... |
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! |
Resolves #1581
Updated documentation. Note about where zone resiliency can be used comes from the Azure documentation
Added a property to an existing test function which specifies the storage type so that it could be reused for the
zone_redundant
test. Added a possible skip to the test, as not all zones support ZRS and did not want false positives interfering with the test.