Update to SDKv2 #3932
Update to SDKv2 #3932
Conversation
|
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 873 files changed, 4462 insertions(+), 2735 deletions(-)) |
|
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=143334" |
There was a problem hiding this comment.
I tried looking for something about TestAccProjectIamPolicy_emptyMembers but didn't find anything. I'd suggest adding logging around the field here + at HEAD.
The policy, err := unmarshalIamPolicy(d.Get("policy_data").(string)) line particularly, I'd be interested in whether d.Get("policy_data").(string) returns the same value between both. (sorry- laptop is slow today so I was browsing the code locally and not in GH, so no link)
| @@ -110,6 +110,9 @@ class Examples < Api::Object | |||
| # These properties will likely be custom code. | |||
| attr_reader :ignore_read_extra | |||
|
|
|||
| # Needs the random provider defined in ExternalProviders | |||
| attr_reader :needs_random | |||
There was a problem hiding this comment.
just curious: is there a downside to requiring it in every test?
| @@ -27,7 +27,9 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d T | |||
| } else { | |||
| // If no full topic given, we expand it to a full topic on the same project | |||
| fullTopic := fmt.Sprintf("projects/%s/topics/%s", project, topic) | |||
| d.Set("topic", fullTopic) | |||
| if err := d.Set("topic", fullTopic); err != nil { | |||
| return nil, fmt.Errorf("Error reading topic: %s", err) | |||
There was a problem hiding this comment.
Lots of these d.Set errors are "reading". Is that intentional & I'm missing something, or should it be writing/setting? (I've got no idea how we'd change them en-masse, but wouldn't want to make you to have to make that change manually)
There was a problem hiding this comment.
I spent the whole day Friday searching and replacing these, and about part way through I realized the error message that I copied said reading rather than writing, so I changed it for some of them. I can change these if you need me to, or at least as many as I can without spending too much time on it.
| @@ -27,7 +27,9 @@ func expand<%= prefix -%><%= titlelize_property(property) -%>(v interface{}, d T | |||
| } else { | |||
| // If no full topic given, we expand it to a full topic on the same project | |||
| fullTopic := fmt.Sprintf("projects/%s/topics/%s", project, topic) | |||
| d.Set("topic", fullTopic) | |||
| if err := d.Set("topic", fullTopic); err != nil { | |||
There was a problem hiding this comment.
Oh no! The SDK path change meant that the rule excluding d.Set from the errcheck lint broke: https://github.com/hashicorp/terraform-provider-google/blob/master/.golangci.yml#L26
This is an improvement to the code so I'd imagine it's worth keeping now that you've made it- sorry that you had to, though.
There was a problem hiding this comment.
oooooooh, that's what did it! oh no :( I was so confused why it was just showing up now, but now that I see it there it makes sense.
|
Mentioned this in the sync meeting but putting here for posterity: I think it's too difficult to review this as it is right now, and needs to be broken up into smaller, independent PRs. For example, the |
|
Closing to split this into a couple different PRs |
Fixes hashicorp/terraform-provider-google#6976
I'm struggling with one test still,
TestAccProjectIamPolicy_emptyMembers, but wanted to get eyes on this to see if someone can see where (if it's in our code) and also because it's a lot of code.There are still failures for:
TestAccBigtableInstance_allowDestroyTestAccOrganizationIamCustomRole_undeleteTestAccProjectIamCustomRole_undeleteTestAccStorageBucket_forceDestroyObjectDeleteErrorTestAccLoggingFolderExclusionI created a PR to fix the first 4 that @paddycarver is looking into.
The last one should be fixed with hashicorp/terraform-plugin-sdk#552
All 5 failures were failures in the testing framework only.
Generated code is at https://github.com/hashicorp/terraform-provider-google/tree/sdkv2 and https://github.com/hashicorp/terraform-provider-google-beta/tree/sdkv2. I'm thinking we'll likely want to merge them first and then do mm, to make sure we have
go.modandgo.sumupdated correctly.If this PR is for Terraform, I acknowledge that I have:
make testandmake lintto ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)