Added best practices documentation for ForceNew fields #7127
Conversation
There was a problem hiding this comment.
The guidance on ForceNew looks reasonable. I think that the internal discussion has lost the forest for the trees a bit- the assumed default state of a resource is that every field is ForceNew, because the resource is immutable. Terraform Core should throw a validation error if that's not the case. A developer can then add an update method to allow removing it, although for each field where you've removed it it's assumed that by removing it you've handled updating the field in the update method.
I'm not sure that the guidance on mitigating data loss maps to our current practices- see comments. Namely, deletion_policy hasn't been quite that consistent (particularly around handling of the empty value) and deletion_protection hasn't fallen off like force_delete (which has also been used to express "delete child objects as well", such as GCS objects).
| The previous best practices were: | ||
|
|
||
| - A virtual field called `force_delete` that defaulted to `false` | ||
| - A virtual field called `deletion_protection` that defaulted to `true` |
There was a problem hiding this comment.
We have used deletion_protection recently, both it & deletion_policy have been in use for separate reasons for some time.
There was a problem hiding this comment.
I can see how they could be separate - deletion_protection is whether deletion is allowed, and deletion_policy is what happens when a resource is deleted. But if a resource had both - deletion_policy's value would never matter if deletion_protection was on. It seems to me like it would be simpler & still intuitive to allow deletion_policy to take a value that prevents deletion.
But I'm happy to also document them as separate fields with separate purposes.
Co-authored-by: Riley Karson <rileykarson@google.com>
Co-authored-by: Riley Karson <rileykarson@google.com>
melinath
force-pushed
the
b-260004873
branch
from
678e004 to
44b1507
Compare
|
|
||
| This is necessary and required for cases where a field can't be updated in-place, so that [Terraform's core workflow](https://developer.hashicorp.com/terraform/intro#how-does-terraform-work) of aligning real infrastructure with configuration can be achieved. If a field or resource can never be updated in-place and is not marked with `ForceNew`, that is considered a bug in the provider. | ||
|
|
||
| Some fields or resources may be possible to update in place, but only under specific conditions. In these cases, you can call `diff.ForceNew` inside a [`CustomizeDiff`](https://developer.hashicorp.com/terraform/plugin/sdkv2/resources/customizing-differences) function to force recreation. This is considered a good and useful enhancement in cases where it doesn't introduce undue complexity. Any `CustomizeDiff` function like this must be thoroughly unit tested. |
There was a problem hiding this comment.
nit: For "This is considered a good and useful enhancement in cases where it doesn't introduce undue complexity." should we explain which option to take between marking the field forcenew or updatable?
There was a problem hiding this comment.
took a stab at this - let me know what you think.
…atform#7127) * Added best practices documentation for ForceNew fields * Update docs/content/docs/best-practices/_index.md Co-authored-by: Riley Karson <rileykarson@google.com> * Update docs/content/docs/best-practices/_index.md Co-authored-by: Riley Karson <rileykarson@google.com> * Cleaned up and corrected docs * Addressed feedback regarding deletion_protection section * Cleaned up language related to ForceNew * Switched to active case --------- Co-authored-by: Riley Karson <rileykarson@google.com>
…atform#7127) * Added best practices documentation for ForceNew fields * Update docs/content/docs/best-practices/_index.md Co-authored-by: Riley Karson <rileykarson@google.com> * Update docs/content/docs/best-practices/_index.md Co-authored-by: Riley Karson <rileykarson@google.com> * Cleaned up and corrected docs * Addressed feedback regarding deletion_protection section * Cleaned up language related to ForceNew * Switched to active case --------- Co-authored-by: Riley Karson <rileykarson@google.com>
* main: (41 commits) update the test cases to resolve resourcename not found error Adds `grpc` field to `liveness_probe` and `startup_probe` to `google_cloud_run_v2_service` resource (GoogleCloudPlatform#6987) Upgrade DCL to v1.34 (GoogleCloudPlatform#7276) Add max_distance field to group placement policy (GoogleCloudPlatform#7354) Add stateful_ips to region_per_instance_config and per_instance_config (GoogleCloudPlatform#7316) Added support for workload-vulnerability-scanning and workload-config-audit (GoogleCloudPlatform#7310) datacatalog - bump Taxonomy and PolicyTag to ga (GoogleCloudPlatform#6989) Added best practices documentation for ForceNew fields (GoogleCloudPlatform#7127) Split resources in "B" products (GoogleCloudPlatform#7350) force recreate on master_config.num_instances (GoogleCloudPlatform#7349) Fix DataFusion instance versions used in tests (GoogleCloudPlatform#7343) remove duplicate word in Cluster.yaml (GoogleCloudPlatform#7347) Move more billing tests that require permissions beyond Billing User to master billing account (GoogleCloudPlatform#7344) Remove artifact repository beta URL, fixup handwritten tests (GoogleCloudPlatform#7345) Cloud Workstations - Workstation Config (GoogleCloudPlatform#7017) Add missing `type` argument to data source docs (GoogleCloudPlatform#7341) Fix caps in spanner resource schema accesses (GoogleCloudPlatform#7346) Downgrade Go to 1.18, modify comments (GoogleCloudPlatform#7339) feat: Add support for deletion_policy on shared vpc service project (GoogleCloudPlatform#7283) fixed virtual field update issues (GoogleCloudPlatform#7318) ...
…atform#7127) * Added best practices documentation for ForceNew fields * Update docs/content/docs/best-practices/_index.md Co-authored-by: Riley Karson <rileykarson@google.com> * Update docs/content/docs/best-practices/_index.md Co-authored-by: Riley Karson <rileykarson@google.com> * Cleaned up and corrected docs * Addressed feedback regarding deletion_protection section * Cleaned up language related to ForceNew * Switched to active case --------- Co-authored-by: Riley Karson <rileykarson@google.com>
Documented the best practices for ForceNew fields. b/260004873