Handle read failure after create failure #3219
Conversation
If a read fails when trying to return a partial result, ensure we still return a partial result with the resource identifier so the operation can be completed using refresh at a later point.
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3219 +/- ##
==========================================
+ Coverage 55.25% 55.28% +0.03%
==========================================
Files 66 66
Lines 9908 9917 +9
==========================================
+ Hits 5475 5483 +8
- Misses 4001 4002 +1
Partials 432 432 ☔ View full report in Codecov by Sentry. |
| @@ -201,7 +201,8 @@ func (r *resourceCrudClient) HandleErrorWithCheckpoint(ctx context.Context, err | |||
| // Try reading its state by ID and return a partial error if succeeded. | |||
| checkpoint, getErr := r.currentResourceStateCheckpoint(ctx, id, inputs) | |||
| if getErr != nil { | |||
| return azure.AzureError(errors.Wrapf(err, "resource updated but read failed %s", getErr)) | |||
| // If reading the state failed, combine the errors but still return the partial state. | |||
| err = azure.AzureError(errors.Wrapf(err, "resource created but read failed %s", getErr)) | |||
There was a problem hiding this comment.
This doesn't look right to me... If getErr is not nil, then checkpoint is nil and that's what we return in the partialError. Are we going to save empty state? Will this work at all? Can you please describe how you tested this change?
There was a problem hiding this comment.
Yup, tested this locally and it works fine. Checkpoint being nil is fine - just means we have no inputs or outputs recorded. Most importantly, we have an ID recorded which lets us run refresh if the resource was actually created correctly - otherwise it'll just show up as an update on the next deployment.
There was a problem hiding this comment.
Arguably, we could make this slightly better by still creating some kind of checkpoint object if the read fails - but perhaps missing just the final outputs.
There was a problem hiding this comment.
We could also get here from Update operation, right? (In which case I'm not sure that the change in the error message makes it better)
Writing an empty snapshot in this case looks even worse? What will Refresh behavior look like?
If a read fails when trying to return a partial result, ensure we still return a partial result with the resource identifier so the operation can be completed using refresh at a later point.
Fixes #3200 - where the partial state failed to be written resulting in the resource being created but lost from state.