Merge pull request #453 from overmindtech/known-after-apply

Use the text (known after apply) instead of removing

cmd/changes_submit_plan.go

@@ -144,7 +144,7 @@ func itemDiffFromResourceChange(resourceChange ResourceChange) (*sdp.ItemDiff, e
 		return nil, fmt.Errorf("failed to parse after attributes: %w", err)
 	}
 
-	err = removeKnownAfterApply(beforeAttributes, afterAttributes, resourceChange.Change.AfterUnknown)
+	err = handleKnownAfterApply(beforeAttributes, afterAttributes, resourceChange.Change.AfterUnknown)
 	if err != nil {
 		return nil, fmt.Errorf("failed to remove known after apply fields: %w", err)
 	}
@@ -204,10 +204,9 @@ func itemDiffFromResourceChange(resourceChange ResourceChange) (*sdp.ItemDiff, e
 	return result, nil
 }
 
-// Removes fields from the `before` and `after` attributes that are known after
-// apply. This is because these fields are not "real" changes and we don't want
-// to show them in the UI
-func removeKnownAfterApply(before, after *sdp.ItemAttributes, afterUnknown json.RawMessage) error {
+// Finds fields from the `before` and `after` attributes that are known after
+// apply and replaces the "after" value with the string "(known after apply)"
+func handleKnownAfterApply(before, after *sdp.ItemAttributes, afterUnknown json.RawMessage) error {
 	var afterUnknownInterface interface{}
 	err := json.Unmarshal(afterUnknown, &afterUnknownInterface)
 	if err != nil {
@@ -228,7 +227,7 @@ func removeKnownAfterApply(before, after *sdp.ItemAttributes, afterUnknown json.
 		},
 	}
 
-	err = removeUnknownFields(&beforeValue, &afterValue, afterUnknownInterface)
+	err = insertKnownAfterApply(&beforeValue, &afterValue, afterUnknownInterface)
 
 	if err != nil {
 		return fmt.Errorf("failed to remove known after apply fields: %w", err)
@@ -237,27 +236,34 @@ func removeKnownAfterApply(before, after *sdp.ItemAttributes, afterUnknown json.
 	return nil
 }
 
-// Recursively remove fields from the before and after values that are known
-// after apply. This is done by comparing the afterUnknown interface to the
-// before and after values and removing the fields that are true.
-//
-// AfterUnknown is an object value with similar structure to After, but with all
-// unknown leaf values replaced with true, and all known leaf values omitted.
-// This can be combined with After to reconstruct a full value after the action,
-// including values which will only be known after apply.
-func removeUnknownFields(before, after *structpb.Value, afterUnknown interface{}) error {
+const KnownAfterApply = `(known after apply)`
+
+// Inserts the text "(known after apply)" in place of null values in the planned
+// "after" values for fields that are known after apply. By default these are
+// `null` which produces a bad diff, so we replace them with (known after apply)
+// to more accurately mirror what Terraform does in the CLI
+func insertKnownAfterApply(before, after *structpb.Value, afterUnknown interface{}) error {
 	switch afterUnknown.(type) {
 	case map[string]interface{}:
 		for k, v := range afterUnknown.(map[string]interface{}) {
 			if v == true {
-				delete(before.GetStructValue().GetFields(), k)
-				delete(after.GetStructValue().GetFields(), k)
+				if afterFields := after.GetStructValue().GetFields(); afterFields != nil {
+					// Insert this in the after fields even if it doesn't exist.
+					// This is because sometimes you will get a plan that only
+					// has a before value for a know after apply field, so we
+					// want to still make sure it shows up
+					afterFields[k] = &structpb.Value{
+						Kind: &structpb.Value_StringValue{
+							StringValue: KnownAfterApply,
+						},
+					}
+				}
 			} else if v == false {
 				// Do nothing
 				continue
 			} else {
 				// Recurse into the nested fields
-				err := removeUnknownFields(before.GetStructValue().GetFields()[k], after.GetStructValue().GetFields()[k], v)
+				err := insertKnownAfterApply(before.GetStructValue().GetFields()[k], after.GetStructValue().GetFields()[k], v)
 				if err != nil {
 					return err
 				}
@@ -266,13 +272,14 @@ func removeUnknownFields(before, after *structpb.Value, afterUnknown interface{}
 	case []interface{}:
 		for i, v := range afterUnknown.([]interface{}) {
 			if v == true {
-				// If this value in a slice is true, remove the corresponding
-				// values from the before and after
-				if before.GetListValue() != nil && len(before.GetListValue().GetValues()) > i {
-					before.GetListValue().Values = append(before.GetListValue().GetValues()[:i], before.GetListValue().GetValues()[i+1:]...)
-				}
+				// If this value in a slice is true, set the corresponding value
+				// in after to (know after apply)
 				if after.GetListValue() != nil && len(after.GetListValue().GetValues()) > i {
-					after.GetListValue().Values = append(after.GetListValue().GetValues()[:i], after.GetListValue().GetValues()[i+1:]...)
+					after.GetListValue().Values[i] = &structpb.Value{
+						Kind: &structpb.Value_StringValue{
+							StringValue: KnownAfterApply,
+						},
+					}
 				}
 			} else if v == false {
 				// Do nothing
@@ -295,7 +302,7 @@ func removeUnknownFields(before, after *structpb.Value, afterUnknown interface{}
 					nestedAfterValue = afterListValues[i]
 				}
 
-				err := removeUnknownFields(nestedBeforeValue, nestedAfterValue, v)
+				err := insertKnownAfterApply(nestedBeforeValue, nestedAfterValue, v)
 				if err != nil {
 					return err
 				}

cmd/changes_submit_plan_test.go

@@ -3,6 +3,7 @@ package cmd
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"testing"
 
 	"github.com/overmindtech/sdp-go"
@@ -147,12 +148,13 @@ func TestExtractProviderNameFromConfigKey(t *testing.T) {
 	}
 }
 
-func TestRemoveKnownAfterApply(t *testing.T) {
+func TestHandleKnownAfterApply(t *testing.T) {
 	before, err := sdp.ToAttributes(map[string]interface{}{
 		"string_value": "foo",
 		"int_value":    42,
 		"bool_value":   true,
 		"float_value":  3.14,
+		"data":         "secret", // Known after apply but doesn't exist in the "after" map, this happens sometimes
 		"list_value": []interface{}{
 			"foo",
 			"bar",
@@ -210,6 +212,7 @@ func TestRemoveKnownAfterApply(t *testing.T) {
 		"int_value": true,
 		"bool_value": true,
 		"float_value": false,
+		"data": true,
 		"list_value": [
 			false,
 			false,
@@ -233,42 +236,47 @@ func TestRemoveKnownAfterApply(t *testing.T) {
 		]
 	}`)
 
-	err = removeKnownAfterApply(before, after, afterUnknown)
+	err = handleKnownAfterApply(before, after, afterUnknown)
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	if _, err := before.Get("int_value"); err == nil {
-		t.Errorf("Expected int_value to be removed from the before, but it's still there")
+	beforeJSON, err := json.MarshalIndent(before, "", "  ")
+	if err != nil {
+		t.Fatal(err)
 	}
-
-	if _, err := before.Get("bool_value"); err == nil {
-		t.Errorf("Expected bool_value to be removed from the before, but it's still there")
+	afterJSON, err := json.MarshalIndent(after, "", "  ")
+	if err != nil {
+		t.Fatal(err)
 	}
 
-	if _, err := after.Get("int_value"); err == nil {
-		t.Errorf("Expected int_value to be removed from the after, but it's still there")
-	}
+	fmt.Println("BEFORE:")
+	fmt.Println(string(beforeJSON))
+	fmt.Println("\n\nAFTER:")
+	fmt.Println(string(afterJSON))
 
-	if _, err := after.Get("bool_value"); err == nil {
-		t.Errorf("Expected bool_value to be removed from the after, but it's still there")
+	if val, _ := after.Get("int_value"); val != KnownAfterApply {
+		t.Errorf("expected int_value to be %v, got %v", KnownAfterApply, val)
 	}
 
-	if list, err := before.Get("list_value"); err != nil {
-		t.Errorf("Expected list_value to be there, but it's not: %v", err)
-	} else {
+	if val, _ := after.Get("bool_value"); val != KnownAfterApply {
+		t.Errorf("expected bool_value to be %v, got %v", KnownAfterApply, val)
+	}
 
-		if len(list.([]interface{})) != 2 {
-			t.Error("Expected list_value to have 2 elements")
-		}
+	i, err := after.Get("list_value")
+	if err != nil {
+		t.Error(err)
 	}
 
-	if list, err := after.Get("list_value"); err != nil {
-		t.Errorf("Expected list_value to be there, but it's not: %v", err)
-	} else {
-		if len(list.([]interface{})) != 2 {
-			t.Error("Expected list_value to have 2 elements")
+	if list, ok := i.([]interface{}); ok {
+		if list[2] != KnownAfterApply {
+			t.Errorf("expected third string_value to be %v, got %v", KnownAfterApply, list[2])
 		}
+	} else {
+		t.Error("list_value is not a string slice")
 	}
 
+	if val, _ := after.Get("data"); val != KnownAfterApply {
+		t.Errorf("expected data to be %v, got %v", KnownAfterApply, val)
+	}
 }

cmd/plan_mapper_test.go

@@ -164,15 +164,10 @@ func TestMappedItemDiffsFromPlan(t *testing.T) {
 	}
 
 	// In a secret the "data" field is known after apply, but we don't *know*
-	// that it's definitely going to change, so this shouldn't be part of the
-	// final diff
-	_, err = secret.GetItem().GetBefore().GetAttributes().Get("data")
-	if err == nil {
-		t.Errorf("Expected secret before to not have 'data' field, but it does")
-	}
-	_, err = secret.GetItem().GetAfter().GetAttributes().Get("data")
-	if err == nil {
-		t.Errorf("Expected secret after to not have 'data' field, but it does")
+	// that it's definitely going to change, so this should be (known after apply)
+	dataVal, _ := secret.GetItem().GetAfter().GetAttributes().Get("data")
+	if dataVal != KnownAfterApply {
+		t.Errorf("Expected secret data to be known after apply, got '%v'", dataVal)
 	}
 }
 

go.mod

@@ -19,7 +19,7 @@ require (
 	github.com/muesli/reflow v0.3.0
 	github.com/muesli/termenv v0.15.2
 	github.com/overmindtech/aws-source v0.0.0-20240705081859-20822e068693
-	github.com/overmindtech/sdp-go v0.77.0
+	github.com/overmindtech/sdp-go v0.78.0
 	github.com/overmindtech/stdlib-source v0.0.0-20240705081932-90a0076c3451
 	github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c
 	github.com/sirupsen/logrus v1.9.3

go.sum

@@ -236,8 +236,8 @@ github.com/overmindtech/aws-source v0.0.0-20240705081859-20822e068693 h1:sj9r6tc
 github.com/overmindtech/aws-source v0.0.0-20240705081859-20822e068693/go.mod h1:nwmDpkH8uGks1a2zeytcJ33fPbaez+Lcjwq2oMRsKP8=
 github.com/overmindtech/discovery v0.27.6 h1:p+xMEIST0fk6HfbejXR+Ea59+JA5zjCO4zvRqelRqwE=
 github.com/overmindtech/discovery v0.27.6/go.mod h1:A3wvNM6VTo7qfGExWQ+fgh+NfLh35nuPJ7wF4pLYEQI=
-github.com/overmindtech/sdp-go v0.77.0 h1:0GaTm6LKW1fCeyUM0qphj1daMdirkTdyKEwMFsNrd+Y=
-github.com/overmindtech/sdp-go v0.77.0/go.mod h1:X+RPg2bxJfhI674wM4B9AoYzYMO6u7hmSYplA59rKZ4=
+github.com/overmindtech/sdp-go v0.78.0 h1:vhDWFKssBzaHKMu/He44eTEJDGNzivVMFja8r/Kbfzs=
+github.com/overmindtech/sdp-go v0.78.0/go.mod h1:F0jughyQ72jT5NSBLZH+fosT98yci02fsL4KVtL66qw=
 github.com/overmindtech/sdpcache v1.6.4 h1:MJoYBDqDE3s8FrRzZ0RPgFiH39HWI/Mv2ImH1NdLT8k=
 github.com/overmindtech/sdpcache v1.6.4/go.mod h1:/F9XStVdntRJEQjlZ86BPuB1Y7VPo1PFcsCNiU1IoGE=
 github.com/overmindtech/stdlib-source v0.0.0-20240705081932-90a0076c3451 h1:9owcI2o+cIY0WO9W2xXeupv7B0hX3i+T43TAuiVox0I=