Persist state from the API for google_sql_database_instance regardless of what attributes the user has set

[danawillow]

Fixes #114.

Currently a WIP because tests are failing. Debug logs for TestAccGoogleSqlDatabaseInstance_slave at https://gist.github.com/danawillow/f5647fc7a78417eb99c536599742870b. Tests are no longer failing, this is now ready for review.

[catsby]

Does reveal any hidden issues with our current acceptance tests, by way of drift of attributes that weren't previously being read?

[catsby]

We generally recommend checking for errors from d.Set when setting non scalar values like slices and maps.

if err := d.Set("settings", flattenSettings(instance.Settings)); err != nil {
  log.Printf("[WARN] failed to set <...>")
}

Generally you want to log a WARN and not actually error out here

[danawillow]

Done

[rileykarson]

Generally looks good - I'm just curious why so much is computed, and have a line that needs to be moved in Create while you're here.

When running this interactively, it works for Postgresql, but we should also prove it using a test (or even better, both a normal + import test) since it looks like our documentation states that we support it. It's kind of unrelated to these changes, so I have no problem with that as a followup/issue if you don't want to clutter up this PR.

[rileykarson]

While you're in this file, can you move the d.SetId() in Create to below the actual insert request? (~line 540 right now)

[danawillow]

Done

[rileykarson]

It looks like we're setting a lot of values to Computed, which worries me a bit because of what happens when you remove a value in config to get the default behaviour; where the resource will keep the last-set value for the field. How come we've had to make so much of this computed?

[danawillow]

A lot of the fields have defaults that come back from the API or have defaults that differ between first and second generation instances

[rileykarson]

Thanks for clarifying! 👍

[danawillow]

@catsby not really, but I think that's a good thing for a refactoring change like this. I did notice that we had no ability to remove blocks in an update so I made that change, although because of the bug with Computed+Optional (see the discussion in #committers-terraform from yesterday) they don't really work the way they're supposed to anyway :-/

@rileykarson Yeah let's take that into a followup since this is big enough as it is.

[rileykarson]

Following up with Postgres tests SGTM

LGTM, merge at will 👍

TF_ACC=1 go test ./google -v -run=TestAccGoogleSqlDatabaseInstance_ -timeout 120m
=== RUN   TestAccGoogleSqlDatabaseInstance_importBasic
--- PASS: TestAccGoogleSqlDatabaseInstance_importBasic (49.47s)
=== RUN   TestAccGoogleSqlDatabaseInstance_importBasic3
--- PASS: TestAccGoogleSqlDatabaseInstance_importBasic3 (370.99s)
=== RUN   TestAccGoogleSqlDatabaseInstance_basic
--- PASS: TestAccGoogleSqlDatabaseInstance_basic (57.92s)
=== RUN   TestAccGoogleSqlDatabaseInstance_basic2
--- PASS: TestAccGoogleSqlDatabaseInstance_basic2 (57.79s)
=== RUN   TestAccGoogleSqlDatabaseInstance_basic3
--- PASS: TestAccGoogleSqlDatabaseInstance_basic3 (373.19s)
=== RUN   TestAccGoogleSqlDatabaseInstance_settings_basic
--- PASS: TestAccGoogleSqlDatabaseInstance_settings_basic (37.22s)
=== RUN   TestAccGoogleSqlDatabaseInstance_slave
--- PASS: TestAccGoogleSqlDatabaseInstance_slave (806.62s)
=== RUN   TestAccGoogleSqlDatabaseInstance_diskspecs
--- PASS: TestAccGoogleSqlDatabaseInstance_diskspecs (361.09s)
=== RUN   TestAccGoogleSqlDatabaseInstance_maintenance
--- PASS: TestAccGoogleSqlDatabaseInstance_maintenance (390.77s)
=== RUN   TestAccGoogleSqlDatabaseInstance_settings_upgrade
--- PASS: TestAccGoogleSqlDatabaseInstance_settings_upgrade (54.81s)
=== RUN   TestAccGoogleSqlDatabaseInstance_settings_downgrade
--- PASS: TestAccGoogleSqlDatabaseInstance_settings_downgrade (44.18s)
=== RUN   TestAccGoogleSqlDatabaseInstance_authNets
--- PASS: TestAccGoogleSqlDatabaseInstance_authNets (69.94s)
=== RUN   TestAccGoogleSqlDatabaseInstance_multipleOperations
--- PASS: TestAccGoogleSqlDatabaseInstance_multipleOperations (81.35s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	2755.451s

[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 👉 [email protected]. Thanks!