move setId until after possible errors #5821

megan07 · 2022-03-15T20:29:36Z

Fixes hashicorp/terraform-provider-google#11266

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

logging: fixed bug where `google_logging_project_bucket_config` would erroneously write to state after it errored out and wasn't actually created.

modular-magician · 2022-03-15T20:43:45Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 2 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 2 insertions(+), 2 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

shuyama1 · 2022-03-16T04:41:45Z

/gcbrun

modular-magician · 2022-03-16T04:56:07Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 2 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 1 file changed, 2 insertions(+), 2 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-03-16T07:27:31Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceSqlDatabaseInstance_basic|TestAccApikeysKey_AndroidKey|TestAccApikeysKey_BasicKey|TestAccApikeysKey_IosKey|TestAccApikeysKey_MinimalKey|TestAccApikeysKey_ServerKey|TestAccArtifactRegistryRepository_create_mvn_snapshot|TestAccArtifactRegistryRepository_create_mvn_release|TestAccBigqueryReservationAssignment_BasicHandWritten|TestAccCGCSnippet_sqlMysqlInstanceBackupExample|TestAccCGCSnippet_sqlPostgresInstanceBackupExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupExample|TestAccCGCSnippet_sqlMysqlInstanceAuthorizedNetworkExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupLocationExample|TestAccCloudBuildTrigger_cloudbuildTriggerPubsubConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerWebhookConfigExample|TestAccCloudBuildTrigger_cloudbuildTriggerManualExample|TestAccCloudBuildTrigger_cloudbuildTriggerFilenameExample|TestAccCloudBuildTrigger_cloudbuildTriggerBuildExample|TestAccCloudBuildTrigger_basic|TestAccCloudBuildTrigger_available_secrets_config|TestAccCloudBuildTrigger_cloudbuildTriggerServiceAccountExample|TestAccCloudBuildTrigger_disable|TestAccCloudBuildTrigger_fullStep|TestAccCloudFunctionsFunction_secretEnvVar|TestAccComputeForwardingRule_update|TestAccContainerAwsCluster_BasicHandWritten|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccContainerNodePool_gvnic|TestAccDataprocCluster_nonPreemptibleSecondary|TestAccDataprocCluster_updatable|TestAccDataprocCluster_withConfigOverrides|TestAccLoggingBucketConfigProject_basic|TestAccLoggingBucketConfig_CreateBuckets_withCustomId|TestAccLoggingLogView_BasicHandWritten|TestAccNetworkServicesEdgeCacheOrigin_networkServicesEdgeCacheOriginAdvancedExample|TestAccNetworkServicesEdgeCacheOrigin_updateAndImport|TestAccNetworkServicesEdgeCacheService_networkServicesEdgeCacheServiceAdvancedExample|TestAccNetworkServicesEdgeCacheService_updateAndImport You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=269060

shuyama1 · 2022-03-16T18:49:18Z

Looks like failed tests TestAccLoggingBucketConfigProject_basic and
TestAccLoggingBucketConfig_CreateBuckets_withCustomId are related

shuyama1 · 2022-03-16T18:55:11Z

mmv1/third_party/terraform/resources/resource_logging_bucket_config.go

@@ -136,6 +134,8 @@ func resourceLoggingBucketConfigAcquireOrCreate(parentType string, iDFunc loggin
 			}


We probably need to use id instead of d.Id() here, as setId is moved after.

Yes! Good catch - so odd, this is passing locally for me? But changing now. Thanks!

modular-magician · 2022-03-16T20:38:27Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 5 insertions(+), 5 deletions(-))
Terraform Beta: Diff ( 1 file changed, 5 insertions(+), 5 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

modular-magician · 2022-03-16T22:45:19Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccCGCSnippet_sqlMysqlInstanceBackupExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupLocationExample|TestAccComputeForwardingRule_update|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccDataprocCluster_updatable|TestAccDataprocCluster_withConfigOverrides|TestAccLoggingBucketConfig_CreateBuckets_withCustomId|TestAccLoggingLogView_BasicHandWritten|TestAccNetworkServicesEdgeCacheService_updateAndImport You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=269100

modular-magician · 2022-03-16T23:06:58Z

Tests failed during RECORDING mode: TestAccLoggingBucketConfig_CreateBuckets_withCustomId|TestAccContainerCluster_withAuthenticatorGroupsConfig Please fix these to complete your PR

shuyama1 · 2022-03-17T17:48:33Z

It seems TestAccLoggingBucketConfig_CreateBuckets_withCustomId still failed

shuyama1 · 2022-03-18T20:51:45Z

mmv1/third_party/terraform/resources/resource_logging_bucket_config.go

 			if err != nil {
 				return err
 			}

 			res, _ := sendRequest(config, "GET", "", url, userAgent, nil)
 			if res == nil {
-				log.Printf("[DEGUG] Loggin Bucket not exist %s", d.Id())
+				log.Printf("[DEGUG] Loggin Bucket not exist %s", id)
 				return resourceLoggingBucketConfigCreate(d, meta)


Looks like d.id never got set in resourceLoggingBucketConfigCreate, and that's why the tests are failing, as the request url does not got passed correctly in resourceLoggingBucketConfigRead.

modular-magician · 2022-03-18T21:11:44Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 1 file changed, 11 insertions(+), 7 deletions(-))
Terraform Beta: Diff ( 1 file changed, 11 insertions(+), 7 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

shuyama1

All previously failed tests passed: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloudBeta_ProviderGoogleCloudBetaMmUpstream/270087?buildTab=tests&status=passed

modular-magician · 2022-03-18T23:19:25Z

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccCGCSnippet_sqlSqlserverInstanceBackupRetentionExample|TestAccCGCSnippet_sqlSqlserverInstanceBackupLocationExample|TestAccContainerAwsNodePool_BasicHandWritten|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccFirebaserulesRelease_BasicRelease|TestAccFirebaserulesRelease_MinimalRelease|TestAccFirebaserulesRuleset_BasicRuleset|TestAccFirebaserulesRuleset_MinimalRuleset|TestAccServiceNetworkingPeeredDNSDomain_basic|TestAccMonitoringUptimeCheckConfig_uptimeCheckConfigHttpExample|TestAccOSConfigPatchDeployment_osConfigPatchDeploymentDailyMidnightExample You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=270091

modular-magician · 2022-03-18T23:34:39Z

Tests failed during RECORDING mode: TestAccContainerCluster_withAuthenticatorGroupsConfig Please fix these to complete your PR

* move setId until after possible errors * use id instead of d.Id * update set id in create

megan07 requested a review from shuyama1 March 15, 2022 21:13

shuyama1 requested changes Mar 16, 2022

View reviewed changes

shuyama1 reviewed Mar 18, 2022

View reviewed changes

megan07 force-pushed the megan_11266 branch from 0fb1cbe to 1507cf9 Compare March 18, 2022 20:56

megan07 added 3 commits March 18, 2022 15:57

move setId until after possible errors

eca1531

use id instead of d.Id

53d8b1a

update set id in create

f9b6014

megan07 force-pushed the megan_11266 branch from 1507cf9 to f9b6014 Compare March 18, 2022 20:58

shuyama1 approved these changes Mar 18, 2022

View reviewed changes

megan07 merged commit 7866a9e into GoogleCloudPlatform:main Mar 21, 2022

betsy-lichtenberg pushed a commit to betsy-lichtenberg/magic-modules that referenced this pull request Apr 25, 2022

move setId until after possible errors (GoogleCloudPlatform#5821)

9d23520

* move setId until after possible errors * use id instead of d.Id * update set id in create

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move setId until after possible errors #5821

move setId until after possible errors #5821

megan07 commented Mar 15, 2022

modular-magician commented Mar 15, 2022

shuyama1 commented Mar 16, 2022

modular-magician commented Mar 16, 2022

modular-magician commented Mar 16, 2022

shuyama1 commented Mar 16, 2022

shuyama1 Mar 16, 2022

megan07 Mar 16, 2022

modular-magician commented Mar 16, 2022

modular-magician commented Mar 16, 2022

modular-magician commented Mar 16, 2022

shuyama1 commented Mar 17, 2022

shuyama1 Mar 18, 2022 •

edited

modular-magician commented Mar 18, 2022

shuyama1 left a comment

modular-magician commented Mar 18, 2022

modular-magician commented Mar 18, 2022

		@@ -136,6 +134,8 @@ func resourceLoggingBucketConfigAcquireOrCreate(parentType string, iDFunc loggin
		}

move setId until after possible errors #5821

move setId until after possible errors #5821

Conversation

megan07 commented Mar 15, 2022

modular-magician commented Mar 15, 2022

Diff report:

shuyama1 commented Mar 16, 2022

modular-magician commented Mar 16, 2022

Diff report:

modular-magician commented Mar 16, 2022

shuyama1 commented Mar 16, 2022

shuyama1 Mar 16, 2022

Choose a reason for hiding this comment

megan07 Mar 16, 2022

Choose a reason for hiding this comment

modular-magician commented Mar 16, 2022

Diff report:

modular-magician commented Mar 16, 2022

modular-magician commented Mar 16, 2022

shuyama1 commented Mar 17, 2022

shuyama1 Mar 18, 2022 • edited

Choose a reason for hiding this comment

modular-magician commented Mar 18, 2022

Diff report:

shuyama1 left a comment

Choose a reason for hiding this comment

modular-magician commented Mar 18, 2022

modular-magician commented Mar 18, 2022

shuyama1 Mar 18, 2022 •

edited