Investigate and fix flaky sql user test.

[nat-henderson]

This test fails for a few reasons: one, there are persistent quota
and service-unavailable errors with the second-gen SQL instances.
Two, the issue mentioned in #1184, that there's unspecified behavior
in tests where two resources have the same ID. This changes the ID
to be user/host/instance instead of user/instance, and adds 429/503
handling to the sqladminOperationWait().

[danawillow]

Can you also confirm for me that if you have a sql user in your config from before this change, that adding this change (as if it were an update to terraform) doesn't break things? I don't think you need a migration for this but it doesn't hurt to check.

[danawillow]

I don't think we need to add a new loop inside the loop we already have (the refreshfunc itself, which will get called with backoff from the WaitForState call). Maybe just treat 429/503s as PENDING?

[nat-henderson]

You're right, that's way better.

[danawillow]

just curious, for second gen, what does currentUser.Host return? If it's the empty string then you could leave this as is since d.Get("host").(string) would be the empty string

[nat-henderson]

Done.

[nat-henderson]

I'm checking that now. I think it should be okay, but let's be sure.

[nat-henderson]

Here's what I did to be sure:

(first, write a config which uses a 2nd-gen sql user)

git checkout master
make
terraform init
terraform apply
git checkout sql-user-flaky-test
make
terraform init
terraform state show google_sql_user.user
<result shows id = admin/testinstance>
terraform refresh
terraform state show google_sql_user.user
<result shows id = admin//testinstance>

I think that proves that it's not a problem. What do you think?

[danawillow]

This could also be simplified with retry, right? That already retries (with backoff) on 429s and 503s. The only obstacle I'd see is the users == nil case- was that one that came up in testing or were you including it just-in-case?

err = retry(func() {
  users, err = config.clientSqlAdmin.Users.List(project, instance).Do()
  return err
})
if err != nil {
  return handleNotFoundError(...)
}

[nat-henderson]

I was using that as an entry condition. Yours is smarter, changing it.

[nat-henderson]

Done, that's better, thank you.

TF_ACC=1 go test ./google -v -run=TestAccSqlUser -timeout 120m
=== RUN   TestAccSqlUser_firstGen
=== RUN   TestAccSqlUser_secondGen
--- PASS: TestAccSqlUser_firstGen (90.28s)
--- PASS: TestAccSqlUser_secondGen (651.36s)
PASS
ok      github.com/terraform-providers/terraform-provider-google/google 651.380s

[ghost]

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!