Don't recreate container_cluster when maintenance_window changes

[michaelbannister]

Fixes #727

[michaelbannister]

I'm not sure if this extra step is worth it. I wanted to add something to prove that the cluster had not been torn down & recreated, but couldn't find a neat way to do this.

[michaelbannister]

I also tried to add code to testAccCheckContainerCluster to prove the absence of a maintenance window:

		if cluster.MaintenancePolicy == nil {
			clusterTests = append(clusterTests, clusterTestField{"maintenance_policy", []string{}})
		} else {
			clusterTests = append(clusterTests, clusterTestField{"maintenance_policy.0.daily_maintenance_window.0.start_time", cluster.MaintenancePolicy.Window.DailyMaintenanceWindow.StartTime})
			clusterTests = append(clusterTests, clusterTestField{"maintenance_policy.0.daily_maintenance_window.0.duration", cluster.MaintenancePolicy.Window.DailyMaintenanceWindow.Duration})
		}

but it turned out not really to prove anything: if I failed to correctly implement the "update MaintenanceWindow to nil" call, the state passed into the TestCheckFunc was the actual state (i.e. still with a non-nil MaintenanceWindow) so the test passed even though the apply hadn't done what I wanted.

[danawillow]

Yeah, our test infrastructure definitely leaves something to be desired. I do think having the test to check that you can remove a maintenance window is good to have. For the "check that it was a real update", https://github.com/hashicorp/terraform/issues/15126. And for proving the absence, you could use resource.TestCheckNoResourceAttr- then we have those sorts of checks making sure our state is what we expect it to be, and testAccCheckContainerCluster to (mostly) check that the state matches what GCP says.

[danawillow]

Wooooo thanks for this @michaelbannister!

[danawillow]

This is fine, but the pattern we tend to use is to just not set ForceNew at all if it's false.

[danawillow]

I won't block on this, but if you wanted you could add an expandMaintenancePolicy func that you could call from both here and Create

[danawillow]

Yeah, our test infrastructure definitely leaves something to be desired. I do think having the test to check that you can remove a maintenance window is good to have. For the "check that it was a real update", https://github.com/hashicorp/terraform/issues/15126. And for proving the absence, you could use resource.TestCheckNoResourceAttr- then we have those sorts of checks making sure our state is what we expect it to be, and testAccCheckContainerCluster to (mostly) check that the state matches what GCP says.

[michaelbannister]

Thanks @danawillow for the comments & suggestions…
Happy Christmas/Holidays/Hanukkah (belated)!

[danawillow]

Happy Holidays to you too!

=== RUN   TestAccContainerCluster_withMaintenanceWindow
--- PASS: TestAccContainerCluster_withMaintenanceWindow (309.10s)

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