Unify upgradeResourceState between provider and provider2
#2005
Conversation
The driving motivation for this change is to reduce complexity and allow both upgraders to handle large JSON numbers.
| return newState, newMeta, nil | ||
| } | ||
|
|
||
| func extractSchemaVersion(meta map[string]any) (int64, bool, error) { |
There was a problem hiding this comment.
This needed to be extracted out since we use it in multiple places:
- We use it in
upgradeResourceStateGRPCto send the current schema version to the provider server. - We use it in
upgradeResourceStateto find the correct type to hydrate the instanceState.Attributes into.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2005 +/- ##
==========================================
+ Coverage 60.87% 62.80% +1.92%
==========================================
Files 332 332
Lines 44756 37294 -7462
==========================================
- Hits 27247 23421 -3826
+ Misses 15987 12352 -3635
+ Partials 1522 1521 -1 ☔ View full report in Codecov by Sentry. |
| @@ -148,7 +148,7 @@ func (p v2Provider) Refresh( | |||
| return nil, fmt.Errorf("unknown resource %v", t) | |||
| } | |||
|
|
|||
| state, err := upgradeResourceState(ctx, p.tf, r, stateFromShim(s)) | |||
| state, err := upgradeResourceState(ctx, t, p.tf, r, stateFromShim(s)) | |||
There was a problem hiding this comment.
Should this use GRPC version? Possibly later?
There was a problem hiding this comment.
It does. upgradeResourceState shims from the representation used by v2Provider to cty.Value, invokes upgradeResourceStateGRPC and then shims back.
| // Copy the original ID and meta to the new state and stamp in the new version. | ||
| newState.RawConfig = instanceState.RawConfig | ||
| newState.RawState = v | ||
| newState.Meta = newMeta | ||
| newState.ID = instanceState.ID | ||
|
|
||
| // If state upgraders have modified the ID, respect the modification. |
There was a problem hiding this comment.
I vaguely recall working on the fix for this, would be interesting to see if the gRPC version just handles it right.
There was a problem hiding this comment.
It doesn't, since the gRPC version returns a (cty.Value, map[string]any, error). This is still needed to bridge back to *terraform.InstanceState.
There was a problem hiding this comment.
Brilliant. We can connect this to #1895 - I believe if we do this right and transition to PlanResourceChange cty.Value is all that is needed.
This PR extracts the non provider2 specific part of
planResourceChangeImpl.upgradeStateinto it's own function:upgradeResourceStateGRPC. This part is a pure refactor.The PR then re-implements
upgradeResourceStatein terms ofupgradeResourceStateGRPC.The driving motivation for this change is to reduce complexity and allow both upgraders to handle large JSON numbers.
Replaces #1735