From 81ad18a16c4ba6b00288d1f9d61e19cd405c529a Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Tue, 9 Jul 2024 08:56:24 +0000 Subject: [PATCH 1/5] Use the text (known after apply) instead of removing --- cmd/changes_submit_plan.go | 56 ++++++++++++++++++--------------- cmd/changes_submit_plan_test.go | 50 +++++++++++++++-------------- 2 files changed, 57 insertions(+), 49 deletions(-) diff --git a/cmd/changes_submit_plan.go b/cmd/changes_submit_plan.go index 2a28a03b..c6dda7fb 100644 --- a/cmd/changes_submit_plan.go +++ b/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,33 @@ 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 { + if _, exists := afterFields[k]; exists { + 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 +271,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 +301,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 } diff --git a/cmd/changes_submit_plan_test.go b/cmd/changes_submit_plan_test.go index c0a4e2ed..96b78c83 100644 --- a/cmd/changes_submit_plan_test.go +++ b/cmd/changes_submit_plan_test.go @@ -3,6 +3,7 @@ package cmd import ( "context" "encoding/json" + "fmt" "testing" "github.com/overmindtech/sdp-go" @@ -147,7 +148,7 @@ 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, @@ -233,42 +234,43 @@ 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[3] != KnownAfterApply { + t.Errorf("expected third string_value to be %v, got %v", KnownAfterApply, list[3]) } + } else { + t.Error("list_value is not a string slice") } - } From fe45121e3b13867c17ca3cc27abf5659cbaf88e9 Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Tue, 9 Jul 2024 09:17:22 +0000 Subject: [PATCH 2/5] Updated sdp-go version --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 2eede8f1..d06887f3 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index e2726ff1..fea1211f 100644 --- a/go.sum +++ b/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= From 9b3de66fa3b3c0cc39724d80019204435204f995 Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Tue, 9 Jul 2024 09:19:58 +0000 Subject: [PATCH 3/5] Fix index --- cmd/changes_submit_plan_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/changes_submit_plan_test.go b/cmd/changes_submit_plan_test.go index 96b78c83..aa880ac0 100644 --- a/cmd/changes_submit_plan_test.go +++ b/cmd/changes_submit_plan_test.go @@ -267,8 +267,8 @@ func TestHandleKnownAfterApply(t *testing.T) { } if list, ok := i.([]interface{}); ok { - if list[3] != KnownAfterApply { - t.Errorf("expected third string_value to be %v, got %v", KnownAfterApply, list[3]) + 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") From e0236016bf1764730a9adbd04948b613fc5e4238 Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Tue, 9 Jul 2024 09:28:18 +0000 Subject: [PATCH 4/5] Fixed more tests --- cmd/plan_mapper_test.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/cmd/plan_mapper_test.go b/cmd/plan_mapper_test.go index af993d86..90714438 100644 --- a/cmd/plan_mapper_test.go +++ b/cmd/plan_mapper_test.go @@ -164,15 +164,11 @@ 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) + } } From fbe3a3cb7cf07e1468c0acf75c6973b0c9eb1829 Mon Sep 17 00:00:00 2001 From: Dylan Ratcliffe Date: Tue, 9 Jul 2024 10:17:17 +0000 Subject: [PATCH 5/5] Handle fields that don't exist in after --- cmd/changes_submit_plan.go | 15 ++++++++------- cmd/changes_submit_plan_test.go | 6 ++++++ cmd/plan_mapper_test.go | 1 - 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/cmd/changes_submit_plan.go b/cmd/changes_submit_plan.go index c6dda7fb..7055e78e 100644 --- a/cmd/changes_submit_plan.go +++ b/cmd/changes_submit_plan.go @@ -248,14 +248,15 @@ func insertKnownAfterApply(before, after *structpb.Value, afterUnknown interface for k, v := range afterUnknown.(map[string]interface{}) { if v == true { if afterFields := after.GetStructValue().GetFields(); afterFields != nil { - if _, exists := afterFields[k]; exists { - afterFields[k] = &structpb.Value{ - Kind: &structpb.Value_StringValue{ - StringValue: KnownAfterApply, - }, - } + // 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 diff --git a/cmd/changes_submit_plan_test.go b/cmd/changes_submit_plan_test.go index aa880ac0..3706e6a3 100644 --- a/cmd/changes_submit_plan_test.go +++ b/cmd/changes_submit_plan_test.go @@ -154,6 +154,7 @@ func TestHandleKnownAfterApply(t *testing.T) { "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", @@ -211,6 +212,7 @@ func TestHandleKnownAfterApply(t *testing.T) { "int_value": true, "bool_value": true, "float_value": false, + "data": true, "list_value": [ false, false, @@ -273,4 +275,8 @@ func TestHandleKnownAfterApply(t *testing.T) { } 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) + } } diff --git a/cmd/plan_mapper_test.go b/cmd/plan_mapper_test.go index 90714438..c7502ec0 100644 --- a/cmd/plan_mapper_test.go +++ b/cmd/plan_mapper_test.go @@ -168,7 +168,6 @@ func TestMappedItemDiffsFromPlan(t *testing.T) { dataVal, _ := secret.GetItem().GetAfter().GetAttributes().Get("data") if dataVal != KnownAfterApply { t.Errorf("Expected secret data to be known after apply, got '%v'", dataVal) - } }