-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 zone #9318
fix zone #9318
Conversation
Hello! I am a robot. It looks like you are a: @rileykarson, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
@@ -38,7 +38,6 @@ func ResourceComputeAttachedDisk() *schema.Resource { | |||
|
|||
CustomizeDiff: customdiff.All( | |||
tpgresource.DefaultProviderProject, | |||
tpgresource.DefaultProviderZone, |
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.
Rather than remove this outright, we likely want a resource-specific version that handles pulling the value out of the instance id. @NickElliot in case you have any thoughts.
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.
Yeah, it seems like a pretty straight forward change I could go ahead and takeover if you want @edwardmedia
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.
@rileykarson @NickElliot the issue might be at here https://github.com/hashicorp/terraform-provider-google/blob/a3205a9f9416ac58a0c8e9898a063c7e3181923f/google/tpgresource/utils.go#L793. The value of zone
is actually "", not nil in this case during the plan.
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 think that line is just checking the resource has a field called zone since ResourceData/ResourceDiff methods will only return nil if a field does not appear in a resource schema! @NickElliot can you confirm? (also, we may want to comment those lines if that's the case since it's nonobvious)
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.
correct, if it doesnt exist in schema it returns nil, if it does exist in schema it will return the type-equivalent of nothing aka "" for strings, 0 for ints.
I'll add in the comments with my fix for this
Tests analyticsTotal tests: Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBigQueryDataTable_bigtable|TestAccDataSourceGoogleServiceAccountIdToken_impersonation |
Rerun these tests in REPLAYING mode to catch issues
|
Fixes hashicorp/terraform-provider-google#16265
Release Note Template for Downstream PRs (will be copied)