Change authorized_networks field in sql instance to be a Set.

[rosbo]

Fixes #547

Anybody knows why the update method was so complicated?

I refactored the methods to use the expand/flatten pattern we are using for most resources.

[rosbo]

=== RUN   TestAccGoogleSqlDatabaseInstance_importBasic
--- PASS: TestAccGoogleSqlDatabaseInstance_importBasic (50.46s)
=== RUN   TestAccGoogleSqlDatabaseInstance_importBasic3
--- PASS: TestAccGoogleSqlDatabaseInstance_importBasic3 (372.75s)
=== RUN   TestAccGoogleSqlDatabaseInstance_basic
--- PASS: TestAccGoogleSqlDatabaseInstance_basic (61.45s)
=== RUN   TestAccGoogleSqlDatabaseInstance_basic2
--- PASS: TestAccGoogleSqlDatabaseInstance_basic2 (60.20s)
=== RUN   TestAccGoogleSqlDatabaseInstance_basic3
--- PASS: TestAccGoogleSqlDatabaseInstance_basic3 (352.58s)
=== RUN   TestAccGoogleSqlDatabaseInstance_dontDeleteDefaultUserOnReplica
--- PASS: TestAccGoogleSqlDatabaseInstance_dontDeleteDefaultUserOnReplica (694.33s)
=== RUN   TestAccGoogleSqlDatabaseInstance_settings_basic
--- PASS: TestAccGoogleSqlDatabaseInstance_settings_basic (55.86s)
=== RUN   TestAccGoogleSqlDatabaseInstance_slave
--- PASS: TestAccGoogleSqlDatabaseInstance_slave (1011.91s)
=== RUN   TestAccGoogleSqlDatabaseInstance_maintenance
--- PASS: TestAccGoogleSqlDatabaseInstance_maintenance (452.63s)
=== RUN   TestAccGoogleSqlDatabaseInstance_diskspecs
--- PASS: TestAccGoogleSqlDatabaseInstance_diskspecs (463.82s)
=== RUN   TestAccGoogleSqlDatabaseInstance_settings_upgrade
--- PASS: TestAccGoogleSqlDatabaseInstance_settings_upgrade (67.96s)
=== RUN   TestAccGoogleSqlDatabaseInstance_multipleOperations
--- PASS: TestAccGoogleSqlDatabaseInstance_multipleOperations (85.39s)
=== RUN   TestAccGoogleSqlDatabaseInstance_authNets
--- PASS: TestAccGoogleSqlDatabaseInstance_authNets (62.45s)
=== RUN   TestAccGoogleSqlDatabaseInstance_settings_downgrade
--- PASS: TestAccGoogleSqlDatabaseInstance_settings_downgrade (45.33s)

[danawillow]

Since you're changing it from a List to a Set, you'll also need to add a state migration for people who already have authorized_networks in their config.

[rosbo]

You actually don't need to. The read method is always called prior to do an terraform apply or a terraform plan and it initialize the sets properly with the right indexes.

[danawillow]

If this resource were ForceNew I think this (putting everything in the struct instead of checking if it's set first) would work, but since it's updatable you'll need ForceSendFields in order to be able to change values from non-empty to empty. But then by supplying ForceSendFields, you're insisting that each value has a default of empty, and I haven't checked whether that's actually true. If it is, can you add a comment as to why this is ok? (likewise with expandIpConfiguration)

[danawillow]

Ok the actual more important comment I meant to make here: If any of these aren't set this will panic because you can't type-assert nil to a string.

[danawillow]

Actually, check me on that. I'm not sure whether it'll be nil or the zero value.

[paddycarver]

From the looks of it--please correct me if I'm wrong--this appears to be a breaking change.

1. Any configs referencing the authorized_networks fields by index would break
2. It appears that (I'd have to take a closer look to be sure) the old Update method was in the style of a few years ago, where it tried as hard as possible to preserve changes that happened outside of Terraform, instead of overwriting them. This is not what we want to do now, and should be changed, but changing it is a surprising change that we should probably consider backwards incompatible.

[danawillow]

1. That's true, though I think this is probably pretty low-risk.
2. Why is that backwards incompatible? We already changed the read function to be in this manner, and it wouldn't break any existing configs.

[paddycarver]

For 2, the breaking change is we're changing the semantics of the command; whereas before it was "I will ensure this subset of resources exist", it would now be "I will ensure these and only these resources exist". For users with version ranges set, that's an unexpected and destructive upgrade experience that may not be intentionally opted into.

(Unless I'm misreading this code, which is entirely possible.)

[danawillow]

Ah ok, I see it now. So the issue is if they had a config with authorized_networks A, B, and C but server-side they also had D. In the old version, adding E would just add E but in the new version, adding E would remove D, and that's not something you can tell by running terraform plan because terraform doesn't know about D. That makes sense.

[paddycarver]

I think it would work out that if you had a config with A, B, and C, and the API had A, B, C, and D, before the update Terraform would think this is fine and not do anything. After the update, even if the config isn't modified at all, plan would still show it's going to drop D, because D would now be added to the state? Maybe? Now I'm not sure, because that should've broken with Read being fixed. I'll have to take a closer look at the code.

[danawillow]

Oh right. Hmm. For that one specifically we didn't do it in a major release. We did add release notes, though for some reason it doesn't seem to have actually included all the fields that were changed: https://github.com/terraform-providers/terraform-provider-google/blob/master/CHANGELOG.md#012-july-20-2017

[paddycarver]

Hm. Well, from what I can see, Update's behaviour will definitely change, because it used to merge the list of authorized_networks from the API and the list of authorized_networks in the config, then just create new networks from the config not found in the API--but it would never remove an authorized_network from the API if it wasn't found in the config. And now that behaviour is being corrected, I believe.

As for the 0.1.2 release, that was OK because semver explicitly states that anything is fair game before 1.0.0--there is no meaning, and breaking changes can happen pretty much any time. But now that we hit 1.0.0, a breaking change should bump us to 2.0.0, because users have the expectation that a 1.X.0 release will just contain new features and bug fixes, and 1.1.Y release will just contain bug fixes, and they should be safe to upgrade to without any user interaction.

[danawillow]

So something we could do if we're not ready for a major release yet would just be to break this apart into two separate changes- one to make it a set (in order to fix the linked issue), and the other to simplify the logic.

[rosbo]

I can definitely split the change in two. @paddycarver, do you agree with this course of action?

[paddycarver]

I'm game 👍 I'm cool with the access-by-index being broken because I think it's incredibly unlikely anyone actually used that. I still feel we should call it out in the CHANGELOG, though. But yeah, I'm comfortable with this moving forward with the old, broken behaviour but now as a set, and doing new, fixed behaviour for 2.0.

[rosbo]

I just opened #733 changing only authorized_networks to a Set without changing the update behavior.

[rileykarson]

Closed by #2203

[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!