Check for provider changes in mustWrite (#2805)

Recent changes to default provider semantics and the addition of
resource aliases allow a resource's provider reference to change even if
the resource itself is considered to have no diffs. `mustWrite` did not
expect this scenario, and indeed asserted against it. These changes
update `mustWrite` to detect such changes and require that the
checkpoint be written if and when they occur.

Fixes #2804.

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ### Improvements
 
+- Fixed a bug that caused an assertion when dealing with unchanged resources across version upgrades.
+
 ## 0.17.15 (Released June 5, 2019)
 
 ### Improvements

pkg/backend/snapshot.go

@@ -189,7 +189,12 @@ func (ssm *sameSnapshotMutation) mustWrite(old, new *resource.State) bool {
 	}
 
 	contract.Assert(old.ID == new.ID)
-	contract.Assert(old.Provider == new.Provider)
+
+	// If this resource's provider has changed, we must write the checkpoint. This can happen in scenarios involving
+	// aliased providers or upgrades to default providers.
+	if old.Provider != new.Provider {
+		return true
+	}
 
 	// If this resource's parent has changed, we must write the checkpoint.
 	if old.Parent != new.Parent {

pkg/backend/snapshot_test.go

@@ -256,6 +256,68 @@ func TestSamesWithOtherMeaningfulChanges(t *testing.T) {
 		err = manager.Close()
 		assert.NoError(t, err)
 	}
+
+	// Set up a second provider and change the resource's provider reference.
+	provider2 := NewResource("urn:pulumi:foo::bar::pulumi:providers:pkgA::provider2")
+	provider2.Custom, provider2.Type, provider2.ID = true, "pulumi:providers:pkgA", "id2"
+
+	resourceA.Custom, resourceA.ID, resourceA.Provider =
+		true, "id", "urn:pulumi:foo::bar::pulumi:providers:pkgA::provider::id"
+
+	snap = NewSnapshot([]*resource.State{
+		provider,
+		provider2,
+		resourceA,
+	})
+
+	changes = []*resource.State{NewResource(string(resourceA.URN))}
+	changes[0].Custom, changes[0].Provider = true, "urn:pulumi:foo::bar::pulumi:providers:pkgA::provider2::id2"
+
+	for _, c := range changes {
+		manager, sp := MockSetup(t, snap)
+
+		// Generate sames for the providers.
+		provUpdated := NewResource(string(provider.URN))
+		provUpdated.Custom, provUpdated.Type = true, provider.Type
+		provSame := deploy.NewSameStep(nil, nil, provider, provUpdated)
+		mutation, err := manager.BeginMutation(provSame)
+		assert.NoError(t, err)
+		_, _, err = provSame.Apply(false)
+		assert.NoError(t, err)
+		err = mutation.End(provSame, true)
+		assert.NoError(t, err)
+		assert.Empty(t, sp.SavedSnapshots)
+
+		// The engine generates a Same for p. This is not a meaningful change, so the snapshot is not written.
+		prov2Updated := NewResource(string(provider2.URN))
+		prov2Updated.Custom, prov2Updated.Type = true, provider.Type
+		prov2Same := deploy.NewSameStep(nil, nil, provider2, prov2Updated)
+		mutation, err = manager.BeginMutation(prov2Same)
+		assert.NoError(t, err)
+		_, _, err = prov2Same.Apply(false)
+		assert.NoError(t, err)
+		err = mutation.End(prov2Same, true)
+		assert.NoError(t, err)
+		assert.Empty(t, sp.SavedSnapshots)
+
+		// The engine generates a Same for a. Because this is a meaningful change, the snapshot is written:
+		aSame := deploy.NewSameStep(nil, nil, resourceA, c)
+		mutation, err = manager.BeginMutation(aSame)
+		assert.NoError(t, err)
+		_, _, err = aSame.Apply(false)
+		assert.NoError(t, err)
+		err = mutation.End(aSame, true)
+		assert.NoError(t, err)
+
+		assert.NotEmpty(t, sp.SavedSnapshots)
+		assert.NotEmpty(t, sp.SavedSnapshots[0].Resources)
+
+		inSnapshot := sp.SavedSnapshots[0].Resources[2]
+		assert.Equal(t, c, inSnapshot)
+
+		err = manager.Close()
+		assert.NoError(t, err)
+	}
 }
 
 // This test exercises the merge operation with a particularly vexing deployment