-
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/google: Fix panic and state migration for google_compute_firewall #8390
Conversation
LGTM pending travis |
Whoops looks like some kind of issue there, I'll take a look shortly when I'm back. |
@@ -23,6 +23,7 @@ func resourceComputeFirewall() *schema.Resource { | |||
State: schema.ImportStatePassthrough, | |||
}, | |||
SchemaVersion: 1, | |||
MigrateState: migrateFirewallStateV0toV1, |
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.
I'm still learning go, so I apologize if I'm off here, but I think the reason this is breaking is the wrong function is being called. My guess is this should be resourceComputeFirewallMigrateState.
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.
Yep that's it! We'll get this sorted soon. Thanks @cblecker!
As part of Terraform 0.7.1 it was observed in issue #8345 that the state migration for google_compute_firewall did not appear to be running, causing a panic when an uninitialized member was read. This commit hooks up the state migration function (which _was_ independently unit tested but was not actually in place). There is currently no good test framework for this, I will address this issue in a future RFC.
This commit cleans up the google_compute_firewall resource to the Go 1.5+ style of not requiring map values to declare their type if they can be inferred.
9e50ee7
to
5c83ca7
Compare
Thanks @jen20! My fault for forgetting to hook in the state migration. Appreciate the fix |
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. |
This pull request fixes the issues reported against Terraform 0.7.1 whereby a state migration which never ran (because it wasn't hooked in) caused a panic because of uninitialized or incorrect structure use.
It also tidies up the code for
google_compute_firewall
to conform to the newer style without redundant type declarations.Fixes #8345, Fixes #8341.