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

provider/google: Ensure we don't assert on nil #9370

Merged
merged 1 commit into from
Oct 14, 2016
Merged

provider/google: Ensure we don't assert on nil #9370

merged 1 commit into from
Oct 14, 2016

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Oct 14, 2016

This commit tests whether an interface is nil before type asserting it to string - this should fix the panic reported in #8609 and #9359.

We also clean up the schema definition to the newer style without redundant type declarations.

Fixes #8609, fixes #9359.

Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

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

you can make the change if you want, but otherwise LGTM

@@ -91,7 +91,12 @@ func resourceComputeTargetPool() *schema.Resource {
func convertStringArr(ifaceArr []interface{}) []string {
arr := make([]string, len(ifaceArr))
for i, v := range ifaceArr {
arr[i] = v.(string)
Copy link
Member

Choose a reason for hiding this comment

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

this is a rare case where ignoring the ok is useful: arr[i], _ = v.(string) replaces the if-else

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 that's much nicer.

This commit tests whether an interface is nil before type asserting it
to string - this should fix the panic reported in #8609.

We also clean up the schema definition to the newer style without
redundant type declarations.
@jbardin
Copy link
Member

jbardin commented Oct 14, 2016

LGTM

@caseylang
Copy link

Thanks for the work @jen20!

@jen20 jen20 merged commit f19990a into master Oct 14, 2016
@jen20 jen20 deleted the b-crash-8609 branch October 14, 2016 20:13
@ghost
Copy link

ghost commented Apr 21, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
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.

Terraform panics when running plan Terraform panics when running plan after upgrading from 0.6.16 -> 0.7.2
3 participants