Only set RequiresReplace if value has changed and config is non-null #203
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The following conditions must now be met in order for the RequiresReplace helpers to set RequiresReplace on the attribute: - The planned value must not be equal to the current state. - The config value must not be null. This more closely mirrors the ForceNew behaviour in SDKv2, and corrects unexpected behaviour in which Computed attributes with null config values would force resource recreation whenever updated by the provider.
paddycarver
added a commit
that referenced
this pull request
Oct 27, 2021
Fixes #187, supercedes #203. This changes the logic on our RequiresReplace function in the following ways: * if the attribute being modified has not changed, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when _any_ attribute changes, limiting it only to the attribute with RequiresReplace set on it. * if the attribute being modified is computed and is null in the config, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when the provider returns an unknown value in the plan, or even a new, provider-controlled value, because it's unlikely the practitioners expect their resource to be destroyed and recreated when an attribute out of their control changes. This has the unfortunate side-effect that practitioners removing an Optional+Computed field from their config will not prompt the default RequiresReplace behavior, but providers can specify their own, special plan modifier that has a better understanding of practitioner intent in that case. * if the resource is being created or destroyed, RequiresReplace isn't changed. This prevents the resource from being marked for destruction and recreation when it's just now being created or destroyed, which would be nonsensical. RequiresReplaceIf gets all these changes and an additional change that it will only set RequiresReplace to true, never to false, as its Modify method's GoDoc indicated. This means that RequiresReplaceIf functions can be chained, and that RequiresReplaceIf won't overwrite previous plan modifiers' determinations on whether or not the resource should be destroyed and recreated. Tests were added for RequiresReplace and RequiresReplaceIf to test these invariants.
|
Superseded by #213. |
paddycarver
added a commit
that referenced
this pull request
Oct 28, 2021
Fixes #187, supercedes #203. This changes the logic on our RequiresReplace function in the following ways: * if the attribute being modified has not changed, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when _any_ attribute changes, limiting it only to the attribute with RequiresReplace set on it. * if the attribute being modified is computed and is null in the config, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when the provider returns an unknown value in the plan, or even a new, provider-controlled value, because it's unlikely the practitioners expect their resource to be destroyed and recreated when an attribute out of their control changes. This has the unfortunate side-effect that practitioners removing an Optional+Computed field from their config will not prompt the default RequiresReplace behavior, but providers can specify their own, special plan modifier that has a better understanding of practitioner intent in that case. * if the resource is being created or destroyed, RequiresReplace isn't changed. This prevents the resource from being marked for destruction and recreation when it's just now being created or destroyed, which would be nonsensical. RequiresReplaceIf gets all these changes and an additional change that it will only set RequiresReplace to true, never to false, as its Modify method's GoDoc indicated. This means that RequiresReplaceIf functions can be chained, and that RequiresReplaceIf won't overwrite previous plan modifiers' determinations on whether or not the resource should be destroyed and recreated. Tests were added for RequiresReplace and RequiresReplaceIf to test these invariants.
paddycarver
added a commit
that referenced
this pull request
Nov 3, 2021
Fixes #187, supercedes #203. This changes the logic on our RequiresReplace function in the following ways: * if the attribute being modified has not changed, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when _any_ attribute changes, limiting it only to the attribute with RequiresReplace set on it. * if the attribute being modified is computed and is null in the config, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when the provider returns an unknown value in the plan, or even a new, provider-controlled value, because it's unlikely the practitioners expect their resource to be destroyed and recreated when an attribute out of their control changes. This has the unfortunate side-effect that practitioners removing an Optional+Computed field from their config will not prompt the default RequiresReplace behavior, but providers can specify their own, special plan modifier that has a better understanding of practitioner intent in that case. * if the resource is being created or destroyed, RequiresReplace isn't changed. This prevents the resource from being marked for destruction and recreation when it's just now being created or destroyed, which would be nonsensical. RequiresReplaceIf gets all these changes and an additional change that it will only set RequiresReplace to true, never to false, as its Modify method's GoDoc indicated. This means that RequiresReplaceIf functions can be chained, and that RequiresReplaceIf won't overwrite previous plan modifiers' determinations on whether or not the resource should be destroyed and recreated. Tests were added for RequiresReplace and RequiresReplaceIf to test these invariants.
paddycarver
added a commit
that referenced
this pull request
Nov 3, 2021
Fixes #187, supercedes #203. This changes the logic on our RequiresReplace function in the following ways: * if the attribute being modified has not changed, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when _any_ attribute changes, limiting it only to the attribute with RequiresReplace set on it. * if the attribute being modified is computed and is null in the config, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when the provider returns an unknown value in the plan, or even a new, provider-controlled value, because it's unlikely the practitioners expect their resource to be destroyed and recreated when an attribute out of their control changes. This has the unfortunate side-effect that practitioners removing an Optional+Computed field from their config will not prompt the default RequiresReplace behavior, but providers can specify their own, special plan modifier that has a better understanding of practitioner intent in that case. * if the resource is being created or destroyed, RequiresReplace isn't changed. This prevents the resource from being marked for destruction and recreation when it's just now being created or destroyed, which would be nonsensical. RequiresReplaceIf gets all these changes and an additional change that it will only set RequiresReplace to true, never to false, as its Modify method's GoDoc indicated. This means that RequiresReplaceIf functions can be chained, and that RequiresReplaceIf won't overwrite previous plan modifiers' determinations on whether or not the resource should be destroyed and recreated. Tests were added for RequiresReplace and RequiresReplaceIf to test these invariants.
paddycarver
added a commit
that referenced
this pull request
Nov 3, 2021
Fixes #187, supercedes #203. This changes the logic on our RequiresReplace function in the following ways: * if the attribute being modified has not changed, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when _any_ attribute changes, limiting it only to the attribute with RequiresReplace set on it. * if the attribute being modified is computed and is null in the config, RequiresReplace isn't changed. This prevents the resource from being destroyed and recreated when the provider returns an unknown value in the plan, or even a new, provider-controlled value, because it's unlikely the practitioners expect their resource to be destroyed and recreated when an attribute out of their control changes. This has the unfortunate side-effect that practitioners removing an Optional+Computed field from their config will not prompt the default RequiresReplace behavior, but providers can specify their own, special plan modifier that has a better understanding of practitioner intent in that case. * if the resource is being created or destroyed, RequiresReplace isn't changed. This prevents the resource from being marked for destruction and recreation when it's just now being created or destroyed, which would be nonsensical. RequiresReplaceIf gets all these changes and an additional change that it will only set RequiresReplace to true, never to false, as its Modify method's GoDoc indicated. This means that RequiresReplaceIf functions can be chained, and that RequiresReplaceIf won't overwrite previous plan modifiers' determinations on whether or not the resource should be destroyed and recreated. Tests were added for RequiresReplace and RequiresReplaceIf to test these invariants. Co-authored-by: Brian Flad <bflad417@gmail.com>
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following conditions must now be met in order for the RequiresReplace helpers to set RequiresReplace on the attribute:
This more closely mirrors the ForceNew behaviour in SDKv2, and corrects unexpected behaviour in which Computed attributes with null config values would force resource recreation whenever updated by the provider.
Should fix #187.
Needs careful testing.