-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Fix potential nil panic in azure remote state #32821
Conversation
Thanks for this submission, I've notified the Azure provider team, who would be providing the review for this feature. |
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 @lonegunmanb
Thanks for this PR. Please see below for a suggestion on how this could be changed and kept more consistent with implementations in the AzureRM provider.
Additionally, a 404 should result in a non-nil value for err
here, did you look into why we are getting into this condition?
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 for the updates @lonegunmanb - This LGTM now 👍
Did you find out why we're not getting the err
when the response object has a 404
in it?
No, it's weird since the whole Reponse is |
Hi @jackofallops @manicminer is there anything I can do to get this pr be merged? Thanks! |
Hi @lonegunmanb, we appreciate the work on this, however I don't believe the proposed change will resolve the original error and as such I am not convinced this is worth changing in its current state? |
Hi @manicminer , the user is facing a core crash so the actual error cannot be displayed, as we should avoid any nil panic in any case, could we just merge this pr first, so the user could report the actual ereor to us? |
Hi @manicminer, a kindly ping, is there anything I can do to get this pr merged? Or do you have any suggestion to fix the panic? Thanks! |
Thank you @lonegunmanb for the work in this PR. I believe @magodo's PR #36258 resolved the panic that this PR is meaning to fix, as such I am going to close this PR. |
Adding a nil check before reading Http status code from response returned by sdk.
terraform-azurerm-provider hashicorp/terraform#32872
Waiting for user create a new issue in this repo.
Fixes #
Target Release
1.4.1
Draft CHANGELOG entry
BUG FIXES
Write a short description of the user-facing change. Examples:
terraform init
: Fixed crash when the storage account doesn't exist