-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass state back to the engine if Apply encountered an error 2 #2713
Conversation
This change is part of the following stack: Change managed by git-spice. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2713 +/- ##
==========================================
+ Coverage 69.53% 69.56% +0.03%
==========================================
Files 301 301
Lines 38655 38654 -1
==========================================
+ Hits 26878 26890 +12
+ Misses 10257 10244 -13
Partials 1520 1520 ☔ View full report in Codecov by Sentry. |
a8594a2
to
545a6f3
Compare
0b07184
to
2b822d0
Compare
545a6f3
to
f44bf76
Compare
2b822d0
to
89e62ea
Compare
89e62ea
to
0c78935
Compare
This PR has been shipped in release v3.98.0. |
In the SDKv2 bridge under PlanResourceChange we are not passing any state we receive during TF Apply back to the engine if we also received an error. This causes us to incorrectly miss any resources which were created but encountered errors during the creation process. The engine should see these as ResourceInitError, which allows the engine to attempt to update the partially created resource on the next up.
This PR fixes the issue by passing the state down to the engine in the case when we receive an error and a non-nil state from TF during Apply.
This is the second attempt at this. The first was #2695 but was reverted because it caused a different panic: #2706. We added a regression test for that in #2710
The reason for that panic was that we were now creating a non-nil
InstanceState
with a nilstateValue
which causes theID
function to panic. This PR fixes both issues by not allowing non-nil states with nilstateValue
s and by preventing the panic inID
.There was also a bit of fun with go nil interfaces along the way, which is the reason why
ApplyResourceChange
now returns ashim.InstanceState
interface instead of a*v2InstanceState2
. Otherwise we end up creating a non-nil interface with a nil value.related to pulumi/pulumi-gcp#2700
related to pulumi/pulumi-aws#4759
fixes #2696