-
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/openstack: Fix crash when access_network was not defined #4966
provider/openstack: Fix crash when access_network was not defined #4966
Conversation
8714bc9
to
240268c
Compare
@@ -998,7 +998,7 @@ func getInstanceAccessAddresses(d *schema.ResourceData, networks []map[string]in | |||
hostv6 = n["fixed_ip_v6"].(string) | |||
} | |||
|
|||
if n["access_network"].(bool) { | |||
if ok, an := n["access_network"].(bool); ok && an { |
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.
Should these be named the opposite way around for clarity? Appreciate that it doesn't actually matter here, but the "ok" is generally the second parameter of the type assertion?
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.
Yes indeed. I should really just step away from any keyboard for the rest of the day.
Hi @jtopjian! I've left a comment in line - the nature of the fix looks fine, with one minor nitpick. Thanks for opening the PR! |
240268c
to
7ccaaa1
Compare
Thanks for the review. Should be good now. |
provider/openstack: Fix crash when access_network was not defined
@phinze I was swamped at work and wanted to fix this bug ASAP. Now that the dust has settled, I've had some time to review and I'm utterly confused as to why this bug was triggered. Most specifically, I was only able to recreate this bug by compiling Terraform to a binary and then applying a fresh config. This bug didn't trigger during any acceptance tests nor on existing configurations when used as a binary. Any ideas? |
Can we soon get a 0.6.12 including this patch? :) |
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. |
Fixes #4954