hashicorp / terraform-provider-google

Terraform Provider for Google Cloud Platform
https://registry.terraform.io/providers/hashicorp/google/latest/docs
Mozilla Public License 2.0
2.25k stars 1.7k forks source link

Support delay_hours for google_vmwareengine_private_cloud deletion #18151

Open smcwhtdtmc opened 1 month ago

smcwhtdtmc commented 1 month ago

Community Note

Description

After the UniSuper event, I noticed that the provider hard-codes delay_hours to zero when deleting. This disables an important safety mechanism for every Terraform user.

Can you add a property like deletion_delay_hours on the resource, even though it never gets used until deletion? IMO it should be required, to force every user to think about delete-safety prior to creation. But even if it were optional, that would let users inherit the 3 hour default value from the API itself.

New or Affected Resource(s)

Potential Terraform Configuration

resource "google_vmwareengine_private_cloud" "foo" {
  // other properties unchanged
  deletion_delay_hours = 3
}

References

No response

b/341735228

melinath commented 1 month ago

Note from triage: we should add support for this field. It should default to 0 for backwards compatibility. Tests for this would need to omit the destroy producer check (since the resource would only be soft-deleted.)

smcwhtdtmc commented 1 month ago

Thanks! One quibble:

it should default to 0 for backwards compatibility.

I'm pessimistic about whether this will improve real-world customer experience, because I'm pessimistic about real-world customers. First-time users are unlikely to know about the field, so they're going to get the unsafe value for 'free' with no visible evidence. Sure, there's a 0 in some plan-text, but ~nobody reads plans thoroughly ;)

Is there a way to give people the safe value for free while remaining mostly backwards compatible? Dreaming, with no knowledge of any frameworks you use to maintain your providers or how they might make this hard:

Can you treat nil in the state as 0 for an existing resource, but set 3 (mirror the API's default) during creation if the prop is missing? This is technically backwards-incompatible (changing default behavior always is), but it gets novice users a little bit more safety.

This is truly a quibble. Having the field will be a step forward regardless of the default behavior.

melinath commented 1 month ago

Different customers use things differently; if we changed the default value we would almost certainly get complaints about it from customers who experienced unexpected behavior and/or development churn as a result. However, we would have the option of changing the default value in a future major release.

See https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes/ for more information on breaking changes.

bubbletroubles commented 1 month ago

@melinath I think the issue here is that the default has been hard coded and no option for customers to set their desired value.

Also as this is a destructive setting, I think there would be more complaints if customers have their infrastructure permanently removed.

Most customers would rather a breaking change which allows them to customise recovery times for their infrastructure, rather than their infrastructure getting deleted.