Enable DiffStrategy PlanState by default #1976
Conversation
|
Tests uncovered a change in behavior, logging it in #1977 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1976 +/- ##
==========================================
+ Coverage 61.53% 61.57% +0.04%
==========================================
Files 334 334
Lines 45056 44955 -101
==========================================
- Hits 27725 27682 -43
+ Misses 15803 15750 -53
+ Partials 1528 1523 -5 ☔ View full report in Codecov by Sentry. |
|
Should we run this through downstream tests? Do we expect this to break anything? |
| @@ -21,6 +21,8 @@ import ( | |||
|
|
|||
| // Configures how the provider performs Diff. Since this is a sensitive method that can result in unexpected breaking | |||
| // changes, using a configurable DiffStrategy as a feature flag assists gradual rollout. | |||
| // | |||
| // Deprecated. | |||
There was a problem hiding this comment.
Let's make sure there is an issue to clean these up.
|
I can fire up downstream tests, good idea. |
|
python scripts/downstream_checks.py --hash 5b75c91 ~/code/pulumi-terraform-bridge |
|
aiven and cloudamqp are unrelated. Fastly is the same panic as in #1964 Just restarted f5bigip, the rest were flakes |
|
|
Excellent this is down to #1964 having a look. |
This has been rolled out to GCP and AWS providers for a considerable time and should be the new default behavior. Retaining the WithDiffStrategy and similar functions as deprecated to avoid breaking provider builds that set them.
t0yv0
force-pushed
the
t0yv0/remove-diff-strategy-flag
branch
from
5b75c91
to
c12f66f
Compare
|
This now fails --- FAIL: TestUnknowns/unknown_for_set_block_prop (0.00s) @VenelinMartinov interesting, having a look. |
| @@ -4738,7 +4742,8 @@ func TestUnknowns(t *testing.T) { | |||
| }, | |||
| "response": { | |||
| "properties":{ | |||
| "id":"" | |||
| "id":"", | |||
| "setBlockProps": [{"prop": ""}] | |||
There was a problem hiding this comment.
This looks like an improvement but still not quite what we want. We had a set of 1 element [unk] and now it shows back as indeed a set of one element, just the unknonwn got substituted by an empty value.
@VenelinMartinov points out this may be fixed/changed in #2060
There was a problem hiding this comment.
With 2060 this test flips back. I think it's not a blocker either way, I'll go ahead and pre-test this PR through downstream checks.
|
Another round of checking FAILURE pulumi/pulumi-fastly#553 |
|
Per @VenelinMartinov Fastly error goes away if the Fastly resource is enrolled in PlanResourceChange per #1964 investigation, recommend we do this and not block |
This has been rolled out to GCP and AWS providers for a considerable time and should be the new default behavior. Retaining the WithDiffStrategy and similar functions as deprecated to avoid breaking provider builds that set them.