-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Update azurerm_linux|windows_virtual_machine
and azurerm_linux|windows_virtual_machine_scale_set
- Support managed boot diagnostics
#8917
Conversation
This comment has been minimized.
This comment has been minimized.
Any update on this one? |
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 @ArcturusZhang,
I think what we could do is mark the boot_diagnostic.storage_account property as optional and computed, and then if the block is present but the storagr account missing switch it to "managed" rather then create a new "false" boolean flag? WDYT?
Hi @katbyte thanks for the review! I would love to do that but I am afraid in some circumstances it may not work.
in this way, the managed boot diagnostics usage should be this:
and unmanaged should be
But in this way we cannot change from a unmanaged boot diagnostics to managed - since the And also we cannot make the enabled as optional - diff will come up when the user is not assigning it explicitly but implying it by the presence of the block |
Hi @katbyte actually it works. We could use
for managed boot diagnostics and
for unmanaged. Thanks to the Azure compute team they do not return a value in |
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 @ArcturusZhang, tests are failing:
TestAccAzureRMWindowsVirtualMachineScaleSet_otherBootDiagnosticsMananged: testing.go:684: Step 0 error: After applying this step, the plan was not empty:
DIFF:
UPDATE: azurerm_windows_virtual_machine_scale_set.test
additional_capabilities.#: "0" => "0"
additional_unattend_content.#: "0" => "0"
admin_password: "P@ssword1234!" => "P@ssword1234!"
admin_username: "adminuser" => "adminuser"
automatic_instance_repair.#: "1" => "1"
automatic_instance_repair.0.enabled: "false" => "false"
automatic_instance_repair.0.grace_period: "PT30M" => "PT30M"
automatic_os_upgrade_policy.#: "0" => "0"
boot_diagnostics.#: "0" => "1"
computer_name_prefix: "acctvm20" => "acctvm20"
data_disk.#: "0" => "0"
do_not_run_extensions_on_overprovisioned_machines: "false" => "false"
enable_automatic_updates: "true" => "true"
encryption_at_host_enabled: "false" => "false"
eviction_policy: "" => ""
extension.#: "0" => "0"
health_probe_id: "" => ""
id: "/subscriptions/*******/resourceGroups/acctestRG-201103185031503663/providers/Microsoft.Compute/virtualMachineScaleSets/acctvm20" => "/subscriptions/*******/resourceGroups/acctestRG-201103185031503663/providers/Microsoft.Compute/virtualMachineScaleSets/acctvm20"
identity.#: "0" => "0"
instances: "1" => "1"
license_type: "" => ""
location: "westeurope" => "westeurope"
max_bid_price: "-1" => "-1"
name: "acctvm20" => "acctvm20"
network_interface.#: "1" => "1"
network_interface.0.dns_servers.#: "" => "0"
network_interface.0.enable_accelerated_networking: "false" => "false"
network_interface.0.enable_ip_forwarding: "false" => "false"
network_interface.0.ip_configuration.#: "1" => "1"
network_interface.0.ip_configuration.0.application_gateway_backend_address_pool_ids.#: "" => "0"
network_interface.0.ip_configuration.0.application_security_group_ids.#: "" => "0"
network_interface.0.ip_configuration.0.load_balancer_backend_address_pool_ids.#: "" => "0"
network_interface.0.ip_configuration.0.load_balancer_inbound_nat_rules_ids.#: "" => "0"
network_interface.0.ip_configuration.0.name: "internal" => "internal"
network_interface.0.ip_configuration.0.primary: "true" => "true"
network_interface.0.ip_configuration.0.public_ip_address.#: "0" => "0"
network_interface.0.ip_configuration.0.subnet_id: "/subscriptions/*******/resourceGroups/acctestRG-201103185031503663/providers/Microsoft.Network/virtualNetworks/acctestnw-201103185031503663/subnets/internal" => "/subscriptions/*******/resourceGroups/acctestRG-201103185031503663/providers/Microsoft.Network/virtualNetworks/acctestnw-201103185031503663/subnets/internal"
network_interface.0.ip_configuration.0.version: "IPv4" => "IPv4"
network_interface.0.name: "example" => "example"
network_interface.0.network_security_group_id: "" => ""
network_interface.0.primary: "true" => "true"
os_disk.#: "1" => "1"
os_disk.0.caching: "ReadWrite" => "ReadWrite"
os_disk.0.diff_disk_settings.#: "0" => "0"
os_disk.0.disk_encryption_set_id: "" => ""
os_disk.0.disk_size_gb: "127" => "127"
os_disk.0.storage_account_type: "Standard_LRS" => "Standard_LRS"
os_disk.0.write_accelerator_enabled: "false" => "false"
overprovision: "true" => "true"
plan.#: "0" => "0"
priority: "Regular" => "Regular"
provision_vm_agent: "true" => "true"
proximity_placement_group_id: "" => ""
resource_group_name: "acctestRG-201103185031503663" => "acctestRG-201103185031503663"
rolling_upgrade_policy.#: "0" => "0"
scale_in_policy: "Default" => "Default"
secret.#: "0" => "0"
single_placement_group: "true" => "true"
sku: "Standard_F2" => "Standard_F2"
source_image_id: "" => ""
source_image_reference.#: "1" => "1"
source_image_reference.0.offer: "WindowsServer" => "WindowsServer"
source_image_reference.0.publisher: "MicrosoftWindowsServer" => "MicrosoftWindowsServer"
source_image_reference.0.sku: "2019-Datacenter" => "2019-Datacenter"
source_image_reference.0.version: "latest" => "latest"
terminate_notification.#: "0" => "0"
timezone: "" => ""
unique_id: "d492158d-93c9-4e98-96e8-35eabaaf7695" => "d492158d-93c9-4e98-96e8-35eabaaf7695"
upgrade_mode: "Manual" => "Manual"
winrm_listener.#: "0" => "0"
zone_balance:
Hi @katbyte sorry for not fully tested... Now I have fixed the test, please have a look again, thanks. |
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 @ArcturusZhang - LGTM 👍
This has been released in version 2.36.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 = "~> 2.36.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! |
Fixes #8319
Since add a new required attribute in the existing
boot_diagnostics
block should be considered as a breaking change, I just implement this feature like this.Please let me know if we could accept the other way.
Update:
I have thought of two approaches of fixing this issue, one of them is the way implemented in this PR. The other is this:
which would introduce a breaking change (we add a required attribute). I am not quite sure whether this is allowed. But I personally prefer this way rather than the implementation in this PR, which is more elegant and aligned with the API.