Codecov Report

Attention: Patch coverage is 90.72165% with 9 lines in your changes missing coverage. Please review.

Project coverage is 62.73%. Comparing base (a1be35d) to head (749dfbb).
Report is 6 commits behind head on master.

@@            Coverage Diff             @@
##           master    #2496      +/-   ##
==========================================
+ Coverage   62.71%   62.73%   +0.02%     
==========================================
  Files         381      382       +1     
  Lines       51515    51545      +30     
==========================================
+ Hits        32307    32338      +31     
+ Misses      17395    17394       -1     
  Partials     1813     1813              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Reviewing the tests added in #2405, they look sufficient for nested forcenew in top level and singly nested cases. Can you point me to where multiple nesting levels are tested? I want to see a test that validates our behavior on a schema like this:

List{
  ForceNew: true,
  Elem: Object{
    Fields: {
       "f0": String,
    }
  },
}

I couldn't find tests for more then one level of nesting.

Reviewing the tests added in #2405, they look sufficient for nested forcenew in top level and singly nested cases. Can you point me to where multiple nesting levels are tested? I want to see a test that validates our behavior on a schema like this:

Here's the tests for ForceNew:

I believe I've covered all shapes and nestings of TF schemas there.

I've also added additional tests which focus on how forceNew interacts with Unknowns in #2515

Here's a case where it just returns an empty set of diffs (no changes) because "the shape mismatched". Why?

func TestAddingSecretArray(t *testing.T) {
	old := resource.PropertyMap{"x": resource.NewNullProperty()}
	new := resource.PropertyMap{"x": resource.MakeSecret(resource.NewArrayProperty([]resource.PropertyValue{
		resource.NewStringProperty("SECRET"),
	}))}
	tfs := shimv2.NewSchemaMap(map[string]*schema.Schema{
		"x": &schema.Schema{
			Type: schema.TypeList,
			Elem: &schema.Schema{
				Type: schema.TypeString,
			},
			Optional: true,
		},
	})
	runDetailedDiffTest(t, old, new, tfs, nil, map[string]*pulumirpc.PropertyDiff{
		"x": &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_ADD},
	})
}

@t0yv0 the test you posted seems to work fine. It returns an {x: ADD} which is the expected value.

I guess we might get it off in cases like these:

func TestMistypedArray(t *testing.T) {
	old := resource.PropertyMap{"x": resource.NewStringProperty("a")}
	new := resource.PropertyMap{"x": resource.NewArrayProperty([]resource.PropertyValue{
		resource.NewStringProperty("VAL"),
	})}
	tfs := shimv2.NewSchemaMap(map[string]*schema.Schema{
		"x": &schema.Schema{
			Type: schema.TypeList,
			Elem: &schema.Schema{
				Type: schema.TypeString,
			},
			Optional: true,
		},
	})
	runDetailedDiffTest(t, old, new, tfs, nil, map[string]*pulumirpc.PropertyDiff{
		"x[0]": &pulumirpc.PropertyDiff{Kind: pulumirpc.PropertyDiff_ADD},
	})
}

Here we present an ADD diff when it is really an UPDATE. I think this is enough of a corner case and the diff is still comprehensible that it's probably fine.

Additionally the behaviour was there before, I am fairly sure this PR does not change that.

Hmm, interesting, do I have a problem with local setup? The test fails for me.

=== RUN   TestAddingSecretArray
    detailed_diff_test.go:311: 
        	Error Trace:	/Users/anton/code/pulumi-terraform-bridge/pkg/tfbridge/detailed_diff_test.go:294
        	            				/Users/anton/code/pulumi-terraform-bridge/pkg/tfbridge/detailed_diff_test.go:311
        	Error:      	Not equal: 
        	            	expected: map[string]*pulumirpc.PropertyDiff{"x":(*pulumirpc.PropertyDiff)(0x140009b94a0)}
        	            	actual  : map[string]*pulumirpc.PropertyDiff{}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,17 +1,2 @@
        	            	-(map[string]*pulumirpc.PropertyDiff) (len=1) {
        	            	- (string) (len=1) "x": (*pulumirpc.PropertyDiff)({
        	            	-  state: (impl.MessageState) {
        	            	-   NoUnkeyedLiterals: (pragma.NoUnkeyedLiterals) {
        	            	-   },
        	            	-   DoNotCompare: (pragma.DoNotCompare) {
        	            	-   },
        	            	-   DoNotCopy: (pragma.DoNotCopy) {
        	            	-   },
        	            	-   atomicMessageInfo: (*impl.MessageInfo)(<nil>)
        	            	-  },
        	            	-  sizeCache: (int32) 0,
        	            	-  unknownFields: ([]uint8) <nil>,
        	            	-  Kind: (pulumirpc.PropertyDiff_Kind) 0,
        	            	-  InputDiff: (bool) false
        	            	- })
        	            	+(map[string]*pulumirpc.PropertyDiff) {
        	            	 }
        	Test:       	TestAddingSecretArray

Additionally the behaviour was there before, I am fairly sure this PR does not change that.

So we have a bug, yes?

If I replace code with this:

func isTypeShapeMismatched(val resource.PropertyValue, propType shim.ValueType) bool {
	return false
}

All the tests still pass.

I'll add some tests to make the behaviour clearer

I've pulled your latest changes, and now TestAddingSecretArray panics.

AFAIK the bridge doesn't accept secrets and the engine shouldn't send secrets, is that correct?

Yes, but, we will need to change that to support secrets properly, and at a minimum we need the work backlogged so we know there are additional bugs in diff code.

I think I'd like to see the code factored out and made a little simpler until we have confidence that we're not adding panics into the product or replacing diffs with nil diffs.

Your TestMistypedArray is another bug example. To reduce confidence further, I get a different result on my machine than claimed. IT produces an "x": ADD diff. It really an UPDATE as you point out.

What are the next steps here, are we fixing the code or logging known bugs?

Handling secrets is out of scope for the epic - what panics is an assert saying we don't support secrets in detailed diff.

I'll add an item to add additional tests for mistyped state so that we can decide if this is worth including in the epic. This is out of scope for this PR.

EDIT: I've changed the logic for typeMismatch to trigger an update instead of assume the property is Null and also added tests for this.

I've added #2525 that'd give some basic confidence that these asserts do not result in panics.

Discussed offline, will add Output handling in a separate PR - Outputs are not currently used and we can afford to completely strip them for the detailed diff algorithm as it doesn't need the dependency information or secrets.

EDIT: Opened #2526

This PR has been shipped in release v3.94.0.