Fix spurious ForcesProviderReplace activation #1958
Conversation
The ForceProviderReplace setting introduces cascading replaces when a certain Provider-level configuration setting changes. For example, changing the region in the AWS Provider is able to recreate all the affected resources in the new desired region with this setting. There was however a practical problem fixed with this PR that caused accidental re-creation of resources in the same region. Specifically, some of the resources states provisioned by older versions of Pulumi CLI may not have oldInputs sufficiently populated in the call to DetailedDiff. This caused ForceProviderReplace logic to assume that the region is changing from "" to "us-east-1" where it did not, in fact, change. To compensate for this, the logic no longer initiates cascading replace when the oldInputs map does not contain the value. See also: - pulumi/pulumi-aws#3826 - pulumi/pulumi-aws#3674
t0yv0
requested review from
iwahbe and
VenelinMartinov
and removed request for
iwahbe and
VenelinMartinov
|
This seems the most direct approach to the problem but feels a little odd, possibly does not generalize very well and will need to be revisited. If we know of other use cases or approaches let's consider here. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1958 +/- ##
==========================================
- Coverage 60.56% 60.55% -0.01%
==========================================
Files 331 331
Lines 44722 44726 +4
==========================================
+ Hits 27084 27085 +1
- Misses 16116 16118 +2
- Partials 1522 1523 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I'm fine with this change given the current design.
The need here does make me question the original design. Instead of setting a flag (bool), we should have allowed a diff function
ForceProviderReplace func(ctx context.Context, olds, news resource.PropertyValue) (bool, error)
There was a problem hiding this comment.
This looks like something we'd feature flag in the future. I doubt all provider parameters are not really changes when they move from unspecified to specified.
Should we make it opt-in (so needs a change now) or opt out (safe to merge like this, add a feature flag later)?
EDIT: never mind, this is already flagged, we can add another in the future if we find examples of the reverse behaviour. Happy with this.
|
Yes a callback would be the way to go. This currently has only 1 use case (AWS region) unfortunately. Let's leave as-is but if revisiting go with the callback and deprecate the flag. |
The ForceProviderReplace setting introduces cascading replaces when a certain Provider-level configuration setting changes. For example, changing the region in the AWS Provider is able to recreate all the affected resources in the new desired region with this setting. There was however a practical problem fixed with this PR that caused accidental re-creation of resources in the same region.
Specifically, some of the resources states provisioned by older versions of Pulumi CLI may not have oldInputs sufficiently populated in the call to DetailedDiff. This caused ForceProviderReplace logic to assume that the region is changing from "" to "us-east-1" where it did not, in fact, change. To compensate for this, the logic no longer initiates cascading replace when the oldInputs map does not contain the value.
See also: