Moved check for op name out of create*Waiter

[melinath]

If there is a nested response but the operation is synchronous, there will not be a name - but we still need to run the rest of the logic in order to properly extract the response data.

This is an update to the change made in https://github.com/GoogleCloudPlatform/magic-modules/pull/4368/files#diff-0a9e906091ae6539868494efebdb18f41d945ac77f9dbf6d9be664f71f506028R33. At the time, I thought that the issue was that the operation wasn't being detected. However, the code comment was correct: the operation was synchronous:

Synchronous operation response with metadata (UserAccessBinding)

---[ REQUEST ]---------------------------------------
POST /v1/organizations/<orgId>/gcpUserAccessBindings?alt=json HTTP/1.1
Host: accesscontextmanager.googleapis.com
User-Agent: Terraform/0.12.29 (+https://www.terraform.io) Terraform-Plugin-SDK/2.4.0 terraform-provider-google-beta/dev
Content-Length: 128
Content-Type: application/json
Accept-Encoding: gzip

{
 "access_levels": [
  "accessPolicies/<policyid>/accessLevels/tf_test_chromeos_no_lockgr6qfoohnw"
 ],
 "group_key": "<groupKey>"
}

-----------------------------------------------------
2021/01/05 23:59:17 [DEBUG] Google API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/1.1 200 OK
Connection: close
Transfer-Encoding: chunked
Alt-Svc: h3-29=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-Q050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43"
Cache-Control: private
Content-Type: application/json; charset=UTF-8
Date: Tue, 05 Jan 2021 23:59:17 GMT
Server: ESF
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 0

214
{
  "metadata": {
    "@type": "type.googleapis.com/google.identity.accesscontextmanager.v1.GcpUserAccessBindingOperationMetadata"
  },
  "done": true,
  "response": {
    "@type": "type.googleapis.com/google.identity.accesscontextmanager.v1.GcpUserAccessBinding",
    "name": "organizations/<orgId>/gcpUserAccessBindings/aAQS-YRSPwf-1cEbnsJjq-s-dS6gFsCgWJYdBCp1nMVeRLdFW",
    "groupKey": "<groupKey>",
    "accessLevels": [
"accessPolicies/<policyid>/accessLevels/tf_test_chromeos_no_lockgr6qfoohnw"
    ]
  }
}

The impetus for this PR was that TagBindings also return a synchronous operation, but don't include a name for it, and also don't include a metadata key.

Synchronous operation response without metadata (TagBinding)

---[ REQUEST ]---------------------------------------
POST /v3/tagBindings?alt=json HTTP/1.1
Host: cloudresourcemanager.googleapis.com
User-Agent: Terraform/0.13.6 (+https://www.terraform.io) Terraform-Plugin-SDK/2.5.0 terraform-provider-google-beta/acc
Content-Length: 109
Content-Type: application/json
Accept-Encoding: gzip

{
 "parent": "//cloudresourcemanager.googleapis.com/projects/107041489576",
 "tagValue": "tagValues/815672295465"
}

-----------------------------------------------------
2021/04/02 10:09:54 [DEBUG] Google API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 200 OK
Alt-Svc: h3-29=":443"; ma=2592000,h3-T051=":443"; ma=2592000,h3-Q050=":443"; ma=2592000,h3-Q046=":443"; ma=2592000,h3-Q043=":443"; ma=2592000,quic=":443"; ma=2592000; v="46,43"
Cache-Control: private
Content-Type: application/json; charset=UTF-8
Date: Fri, 02 Apr 2021 17:09:54 GMT
Server: ESF
Vary: Origin
Vary: X-Origin
Vary: Referer
X-Content-Type-Options: nosniff
X-Frame-Options: SAMEORIGIN
X-Xss-Protection: 0

{
  "done": true,
  "response": {
    "@type": "type.googleapis.com/google.cloud.resourcemanager.v3.TagBinding",
    "name": "tagBindings/%2F%2Fcloudresourcemanager.googleapis.com%2Fprojects%2F107041489576/tagValues/815672295465",
    "parent": "//cloudresourcemanager.googleapis.com/projects/107041489576",
    "tagValue": "tagValues/815672295465"
  }
}

The problem is that for *WaitResponse, we need to process synchronous operations, even if they don't have name or metadata fields. I assume that checking for an operation name is necessary for non-*WaitResponse operation handling.

I ran into this while working on hashicorp/terraform-provider-google#8428. TagBinding resources also return a synchronous operation on creation but the operation doesn't contain a "metadata" key, just "done" and "response".

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)

[modular-magician]

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

Diff report:

Terraform GA: Diff ( 17 files changed, 102 insertions(+), 170 deletions(-))
Terraform Beta: Diff ( 28 files changed, 161 insertions(+), 267 deletions(-))

[modular-magician]

I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=180178"

[modular-magician]

I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccComputeNetworkPeeringRoutesConfig_networkPeeringRoutesConfigGkeExample|TestAccContainerCluster_withILBSubsetting|TestAccContainerCluster_withAuthenticatorGroupsConfig|TestAccContainerCluster_regionalWithNodePool|TestAccContainerCluster_withTpu|TestAccContainerCluster_withPrivateClusterConfig|TestAccContainerCluster_withIntraNodeVisibility|TestAccContainerNodePool_nodeLocations|TestAccContainerNodePool_maxPodsPerNode|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_regionalAutoscaling|TestAccContainerNodePool_resize|TestAccContainerNodePool_012_ConfigModeAttr|TestAccContainerNodePool_EmptyGuestAccelerator|TestAccDataprocClusterIamBinding|TestAccDataprocClusterIamMember|TestAccDataprocClusterIamPolicy|TestAccDataprocCluster_updatable|TestAccDataprocCluster_withStagingBucket|TestAccDataprocCluster_withTempBucket|TestAccDataprocCluster_withInitAction|TestAccDataprocCluster_withConfigOverrides|TestAccDataprocJobIamBinding|TestAccDataprocJobIamMember|TestAccDataprocCluster_withServiceAcc|TestAccDataprocJobIamPolicy|TestAccDataprocJob_PySpark|TestAccDataprocJob_updatable|TestAccDataprocJob_Spark|TestAccDataprocJob_Pig|TestAccDataprocJob_Hadoop|TestAccDataprocJob_Hive|TestAccDataprocJob_SparkSql|TestAccStorageHmacKey_update You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=180190"

[modular-magician]

Tests failed during RECORDING mode: TestAccContainerCluster_withPrivateClusterConfigMissingCidrBlock|TestAccDataprocCluster_withConfigOverrides Please fix these to complete your PR

[melinath]

TestAccDataprocCluster_withConfigOverrides looks like it's failing in nightly builds as well. TestAccContainerCluster_withPrivateClusterConfigMissingCidrBlock is not - but the failure doesn't look related to this change? It's also failing on this PR: #4650 (comment)

[rileykarson]

Hmm- I guess the API isn't breaking the definition at https://github.com/googleapis/api-common-protos/blob/master/google/longrunning/operations.proto#L129-L132 or https://google.aip.dev/151 but returning an operation without a name is surprising.

Can you post a debug log snippet including an operation response? In a similar vein to #1396 (comment), it can help capture context in case things change.

[rileykarson]

I think there are two checks we want to make. First we can see if done: true is set and return immediately if so. Next, we can check for a name and return as well, but debug logging a message- because we've probably misclassified an empty response (indicating a synchronous call) as an operation. If neither of those cases is true, we can wait on the operation.

I forget if MMv1 allows individual endpoints on a resource to have operations- I'd guess it does now, but didn't when we added #1396

[melinath]

So, this PR does not represent a net change to OperationWaitTime - it just moves the check for "name" out of create*Waiter. (It also removes the check for "metadata" – but that was originally added to improve the behavior of OperationWaitTimeWithResponse, which this PR improves even further by removing checks for OperationWaitTimeWithResponse altogether.)

I'm not clear on the behavior of Operations that use OperationWaitTime, so I am hesitant to make more changes here. I agree we could check if done is true and short-circuit (assuming that done: True is set on this kind of operation) but I think there wouldn't be much benefit, since OperationWait already short-circuits immediately if the Operation is already done.

[rileykarson]

SGTM- we don't need to add the done check. It'd be fine to do here if we wanted to, since it's already a dedicated change to this kind of behaviour (and only +- 16 LOC)

[melinath]

@rileykarson I've added debug logs to the PR description.