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

azurerm_frontdoor, azurerm_linux_virtual_machine, azurerm_windows_virtual_machine, azurerm_ssh_public_key - Fix nil panic by throwing error when the resource was missing during update #21975

Merged
merged 2 commits into from
Jun 26, 2023

Conversation

lonegunmanb
Copy link
Contributor

@lonegunmanb lonegunmanb commented May 31, 2023

When executing update, if the resource was deleted out-of-band already, the current implementation would lead to a nil panic. This pr fix #21972 throwing the error even http status code is 404.

Another possible approach is like what we do in azurerm_linux_virtual_machine, just swallow the error and do nothing in this scenario.

I personally prefer throwing out the error since the real situation is different than the operator's assumption, the provider should notify the user about this drift explicitly.

@tombuildsstuff
Copy link
Contributor

hey @lonegunmanb

Thanks for this PR.

Taking a look through here the usage that you've called out within the Linux (and presumably Windows) Virtual Machine resources is actually a bug, so it'd be worth removing that as a part of this PR too - would you mind looking into fixing that? From Terraform's perspective to get into the Update function we'll have passed through the Read, where we'll have determined that the resource has gone (via d.SetID("")) - as such the update should be able to assume that this resource exists, and throw the error if it's not found?

Thanks!

@lonegunmanb
Copy link
Contributor Author

lonegunmanb commented May 31, 2023

Glad to do so @tombuildsstuff!


I've updated the pr and scanned compute package, more similar bugs have been fixed in the same way, I think compute and frontdoor package don't contain this bug any more, but maybe we need arrange a check on other package, or a static check tool based on go ast to detect such issue. @tombuildsstuff

@lonegunmanb lonegunmanb changed the title azurerm_frontdoor - Fix nil panic by throwing error when the resource was missing during update azurerm_frontdoor, azurerm_linux_virtual_machine, azurerm_windows_virtual_machine, azurerm_ssh_public_key - Fix nil panic by throwing error when the resource was missing during update May 31, 2023
@lonegunmanb
Copy link
Contributor Author

Hi @tombuildsstuff, a kindly ping, could we merge this pr? Thanks!

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks for this @lonegunmanb LGTM 🥝

@stephybun stephybun merged commit 5206eec into hashicorp:main Jun 26, 2023
@github-actions github-actions bot added this to the v3.63.0 milestone Jun 26, 2023
stephybun added a commit that referenced this pull request Jun 26, 2023
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2024
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.

Error: The terraform-provider-azurerm_v3.58.0_x5 plugin crashed!
3 participants