-
Notifications
You must be signed in to change notification settings - Fork 9.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
provider/azurerm: add virtual_machine_extension resource #9962
Conversation
Hi @pmcatominey Looking at this right now - what are your thoughts on adding a test for each of the Extension types listed in this document? This would guarantee we can support the main types P. |
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 @pmcatominey
This LGTM pending a few small questions / thoughts inline. This is an amazing feature to have covered - thanks so much!
Let me know your thoughts.
Paul
d.Set("type_handler_version", resp.Properties.TypeHandlerVersion) | ||
d.Set("auto_upgrade_minor_version", resp.Properties.AutoUpgradeMinorVersion) | ||
|
||
settings, err := flattenArmVirtualMachineExtensionSettings(*resp.Properties.Settings) |
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 any change that Settings could be nil? If so, then we are going to cause a panic. Worth passing the Settings struct as a pointer to the flatten func and carrying out the necessary checks in there?
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.
Seems that settings
is optional as far as the API is concerned, I thought it was required, I'll guard against a panic in the read since it's the only settings map read back.
|
||
settings = <<SETTINGS | ||
{ | ||
"commandToExecute": "hostname" |
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'd love to see a more complex example and perhaps something with ProtectedSettings as well to make sure that it works
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.
Will take care of this by testing more extensions.
7fdd995
to
668dae8
Compare
Picked up from where hashicorp#6548 left off settings and protected_settings take JSON objects as strings to make extension generic TF_ACC=1 go test ./builtin/providers/azurerm -v -run "TestAccAzureRMVirtualMachineExtension" -timeout 120m === RUN TestAccAzureRMVirtualMachineExtension_importBasic --- PASS: TestAccAzureRMVirtualMachineExtension_importBasic (697.55s) === RUN TestAccAzureRMVirtualMachineExtension_basic --- PASS: TestAccAzureRMVirtualMachineExtension_basic (824.17s) === RUN TestAccAzureRMVirtualMachineExtension_concurrent --- PASS: TestAccAzureRMVirtualMachineExtension_concurrent (929.74s) === RUN TestAccAzureRMVirtualMachineExtension_linuxDiagnostics --- PASS: TestAccAzureRMVirtualMachineExtension_linuxDiagnostics (803.19s) PASS ok github.com/hashicorp/terraform/builtin/providers/azurerm 3254.663s
668dae8
to
cdbedcb
Compare
Updated the handling of settings as it is an optional field, added a test using the Linux Diagnostics extension to cover usage of protected_settings:
|
Thanks for the additions! This LGTM :) |
Picked up from where #6548 left off settings and protected_settings take JSON objects as strings to make extension generic TF_ACC=1 go test ./builtin/providers/azurerm -v -run "TestAccAzureRMVirtualMachineExtension" -timeout 120m === RUN TestAccAzureRMVirtualMachineExtension_importBasic --- PASS: TestAccAzureRMVirtualMachineExtension_importBasic (697.55s) === RUN TestAccAzureRMVirtualMachineExtension_basic --- PASS: TestAccAzureRMVirtualMachineExtension_basic (824.17s) === RUN TestAccAzureRMVirtualMachineExtension_concurrent --- PASS: TestAccAzureRMVirtualMachineExtension_concurrent (929.74s) === RUN TestAccAzureRMVirtualMachineExtension_linuxDiagnostics --- PASS: TestAccAzureRMVirtualMachineExtension_linuxDiagnostics (803.19s) PASS ok github.com/hashicorp/terraform/builtin/providers/azurerm 3254.663s
) Picked up from where hashicorp#6548 left off settings and protected_settings take JSON objects as strings to make extension generic TF_ACC=1 go test ./builtin/providers/azurerm -v -run "TestAccAzureRMVirtualMachineExtension" -timeout 120m === RUN TestAccAzureRMVirtualMachineExtension_importBasic --- PASS: TestAccAzureRMVirtualMachineExtension_importBasic (697.55s) === RUN TestAccAzureRMVirtualMachineExtension_basic --- PASS: TestAccAzureRMVirtualMachineExtension_basic (824.17s) === RUN TestAccAzureRMVirtualMachineExtension_concurrent --- PASS: TestAccAzureRMVirtualMachineExtension_concurrent (929.74s) === RUN TestAccAzureRMVirtualMachineExtension_linuxDiagnostics --- PASS: TestAccAzureRMVirtualMachineExtension_linuxDiagnostics (803.19s) PASS ok github.com/hashicorp/terraform/builtin/providers/azurerm 3254.663s
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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Picked up from where #6548 left off
Fixes #5820
settings and protected_settings take JSON objects as strings to make extension
generic