Remove custom code for computing planned state #1276

t0yv0 · 2023-07-06T21:13:29Z

Factors out ProposedNew into its own package so both Plugin Framework providers and SDKv2-based providers that opt into DiffStrategy=PlanState call the exact same logical implementation that is linked from TF CLI sources (objchange.ProposedNew).

  graph LR
      A[pfutils.ProposedNew] --> B[tfplan.ProposedNew]
      C[sdkv2.ProposedNew]   --> B[tfplan.ProposedNew]
      B --> D[objchange.ProposedNew]

Previously, PF-based providers used a homegrown implementation that was likely diverging from TF implementation in some of the edge cases. The PR trades off maintaining that implementation (which is no longer necessary) to maintaining some adapter boilerplate code.

github-actions · 2023-07-06T21:18:03Z

Diff for pulumi-random with merge commit 5cb3634

github-actions · 2023-07-06T21:18:29Z

Diff for pulumi-azuread with merge commit 5cb3634

t0yv0 · 2023-07-06T21:29:23Z

pf/tests/testdata/updateprogram/Pulumi.yaml

@@ -38,7 +38,7 @@ outputs:
  testOptionalString__expect: |-
    input2
  testSetOptionals__actual: ${set.setOptionals}
-  testSetOptionals__expect: [ "${r.requiredInputStringCopy}", a, b ]
+  testSetOptionals__expect: [ a, b, "${r.requiredInputStringCopy}" ]


This change caused set elements to be reordered in the list projection to Pulumi. While this should theoretically be alright, as the subsequent Diff recognizes that sets are sets and should not be sensitive to element order, perhaps there are some unintended consequences?

Should the bridge define some guarantees on the canonical set ordering when projecting elements to Pulumi array property values?

I think the bridge needs to define a constant ordering since we expose this data as a list. We shouldn't need to guarantee anything about the ordering, except consistency.

Its not unreasonable to assume that new Resource("one", opt=set[0]); new Resource("two", opt=set[1]) is safe between updates, where a re-order would break this.

I'm not sure I grok the example, could you expand.

Since bridged providers customize Diff method sending re-ordered versions of the set should result in a no-changes diff. But I have a hunch somewhere we run into trouble anyway. Would be good to file an issue figuring out where.

Perhaps with ignoreChanges paths is one place.

FWIW this PR seems to move us from "user-defined" ordering to "TF-defined" consistent ordering, indirectly, based on the example changes. There's some subtleties with this. It probably is affected by TF hashing functions and when they change upstream this is likely to reorder sets.

There are also some serious issues with sets on the SDKV2 based providers. I was recently looking at this one:

#1255

#1276 (review) has a better example. Basically, external users can rely on the ordering of pulumi lists (and tf sets) such that re-ordering will always cause problems, no matter what the bridge does.

If we believe that TF-defined sets are better (more consistent, easier to maintain) then I'm ok moving to them as long as we (1) recognize it is a breaking change and (2) put tests and documentation locking that ordering in.

github-actions · 2023-07-06T22:31:09Z

Diff for pulumi-random with merge commit 8124203

github-actions · 2023-07-06T22:33:09Z

Diff for pulumi-azuread with merge commit 8124203

t0yv0 · 2023-07-06T22:37:29Z

OK not ready for review quite yet, there's some more edge cases caught by the integration test suite.

github-actions · 2023-07-07T04:23:25Z

Diff for pulumi-random with merge commit 2bfc532

github-actions · 2023-07-07T04:23:34Z

Diff for pulumi-azuread with merge commit 2bfc532

iwahbe

In general this looks good. I'm scared of the set re-ordering though. That seams like a meaningful breaking change that could cause replaces. The obvious example is:

const resource = new Resource("parent")
resource.setProperty.apply(set => {
  // Create resources inside an apply, 
  // each resource is name associated with its ordering
  set.map((i, v) => new Child(`child-${i}`, {
    needsReplaceOnChanges: v,
  }))
}

Here a change in order would cause all children to be replaced.

iwahbe · 2023-07-07T08:11:36Z

pf/internal/pfutils/schema.go

@@ -45,32 +48,41 @@ type Schema interface {
 	//
 	// [State Upgrade]: https://developer.hashicorp.com/terraform/plugin/framework/resources/state-upgrade
 	ResourceSchemaVersion() int64
+
+	Proto() *tfprotov6.Schema


Suggested change

Proto() *tfprotov6.Schema

// The tfprotov6.Schema associated with `Schema`. If nil, then ...

Proto() *tfprotov6.Schema

iwahbe · 2023-07-07T08:12:23Z

pf/internal/pfutils/schema.go

 }

 func FromProviderSchema(x pschema.Schema) Schema {
 	attrs := convertMap(FromProviderAttribute, x.Attributes)
 	blocks := convertMap(FromProviderBlock, x.Blocks)
 	// Provider schemas cannot be versioned, see also x.GetVersion() always returning 0.
 	version := int64(0)
-	return newSchemaAdapter(x, x.Type(), x.DeprecationMessage, attrs, blocks, x.AttributeAtPath, version)
+	proto, err := schemav6.ProviderSchema(x)
+	contract.AssertNoError(err)


Can you explain why this is safe?

Suggested change

contract.AssertNoError(err)

contract.AssertNoErrorf(err, "Will not error because...")

iwahbe · 2023-07-07T10:03:44Z

pf/internal/schemav6/schemav6.go

+func (r *schemaExtractingResource) Create(context.Context, resource.CreateRequest, *resource.CreateResponse) {
+}
+
+func (r *schemaExtractingResource) Read(context.Context, resource.ReadRequest, *resource.ReadResponse) {
+}
+
+func (r *schemaExtractingResource) Update(context.Context, resource.UpdateRequest, *resource.UpdateResponse) {
+}
+
+func (r *schemaExtractingResource) Delete(context.Context, resource.DeleteRequest, *resource.DeleteResponse) {
+}


Can we assert that these methods will not be called (and then panic)?

I wonder if this would reduce branch coverage :)

iwahbe · 2023-07-07T10:05:09Z

pf/tests/internal/testprovider/testbridge_resource_compopt.go

@@ -0,0 +1,81 @@
+// Copyright 2016-2023, Pulumi Corporation.


This file is called testbridge_resource_compopt.go If compopt is "component options", can we just say that? Otherwise, can we say what it is?

iwahbe · 2023-07-07T10:09:27Z

pf/tests/testdata/updateprogram/Pulumi.yaml

@@ -38,7 +38,7 @@ outputs:
  testOptionalString__expect: |-
    input2
  testSetOptionals__actual: ${set.setOptionals}
-  testSetOptionals__expect: [ "${r.requiredInputStringCopy}", a, b ]
+  testSetOptionals__expect: [ a, b, "${r.requiredInputStringCopy}" ]


I think the bridge needs to define a constant ordering since we expose this data as a list. We shouldn't need to guarantee anything about the ordering, except consistency.

Its not unreasonable to assume that new Resource("one", opt=set[0]); new Resource("two", opt=set[1]) is safe between updates, where a re-order would break this.

iwahbe · 2023-07-07T10:10:55Z

pkg/tfshim/sdk-v2/internal/schemav6/schemav6.go

+
+	prov := &schema.Provider{
+		ResourcesMap: map[string]*schema.Resource{
+			"xres": res,


Suggested change

"xres": res,

resourceName: res,

iwahbe · 2023-07-07T10:14:15Z

pkg/tfplan/proposed_new.go

+	ctypack "github.com/zclconf/go-cty/cty/msgpack"
+)
+
+func ProposedNew(


This is the public entry point for this package. Can we add a comment explaining why it exists and what it does.

If we don't believe another repository could consume this function, we should mark it as unstable for consumption. We should be clear on when we adopt a backwards compatibility requirement.

t0yv0 · 2024-05-21T20:08:59Z

Obsolete now, the objective is met without this PR.

t0yv0 added 11 commits July 3, 2023 13:16

Remove homegrown ProposedNew implementation

8dd5c0d

Add a stub back

fa54a5c

Rewrite SDKv2 ProposedNew against tftypes

9e3fc9b

Populate tfplan

bfce61a

Small fixes

4e06e9b

Tidy up sdk-v2/proposed_new

fbedebd

Drop redundant borrowed files

2455d95

Toward a working version passing most tests; still got 2 failing tests

b50b890

go mod tidy

94a290a

Set element order has changed

5bf6b2b

Another set reordering

7c6cbaf

t0yv0 commented Jul 6, 2023

View reviewed changes

Move Computed Optional becoming unknown test to a higher level

496ed63

t0yv0 marked this pull request as ready for review July 6, 2023 22:29

t0yv0 requested a review from iwahbe July 6, 2023 22:29

t0yv0 mentioned this pull request Jul 6, 2023

Link ProposedNew from pulumi/terraform fork #929

Closed

t0yv0 removed the request for review from iwahbe July 6, 2023 22:37

t0yv0 added 3 commits July 6, 2023 22:46

schemav6 package for extracting PF schemas to protov6 form

dfd298d

Remove more home-grown converters

40abbc4

Remove more custom schema converters, fix regression tests

ae47181

iwahbe reviewed Jul 7, 2023

View reviewed changes

t0yv0 closed this May 21, 2024

mjeffryes assigned t0yv0 May 24, 2024

This was referenced Jul 16, 2024

PF update proposed new to pull recent upstream changes #2172

Closed

Use OpenTofu planner with Plugin Framework providers #2188

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove custom code for computing planned state #1276

Remove custom code for computing planned state #1276

t0yv0 commented Jul 6, 2023 •

edited

Loading

github-actions bot commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

t0yv0 Jul 6, 2023

iwahbe Jul 7, 2023

t0yv0 Jul 7, 2023

iwahbe Jul 7, 2023

iwahbe Jul 7, 2023

github-actions bot commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

t0yv0 commented Jul 6, 2023

github-actions bot commented Jul 7, 2023

github-actions bot commented Jul 7, 2023

iwahbe left a comment

iwahbe Jul 7, 2023

iwahbe Jul 7, 2023

iwahbe Jul 7, 2023

t0yv0 Jul 7, 2023

iwahbe Jul 7, 2023

iwahbe Jul 7, 2023

iwahbe Jul 7, 2023

iwahbe Jul 7, 2023

t0yv0 commented May 21, 2024

	Proto() *tfprotov6.Schema
	// The tfprotov6.Schema associated with `Schema`. If nil, then ...
	Proto() *tfprotov6.Schema

	contract.AssertNoError(err)
	contract.AssertNoErrorf(err, "Will not error because...")

Remove custom code for computing planned state #1276

Remove custom code for computing planned state #1276

Conversation

t0yv0 commented Jul 6, 2023 • edited Loading

github-actions bot commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

t0yv0 commented Jul 6, 2023

github-actions bot commented Jul 7, 2023

github-actions bot commented Jul 7, 2023

iwahbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented May 21, 2024

t0yv0 commented Jul 6, 2023 •

edited

Loading