From 68ed50616e0e70525f0b301b22288d9133a3b6de Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 18 Aug 2021 13:17:03 -0400 Subject: [PATCH] handle null and unknown values in attr diffs The code adopted from block diffs was not set to handle null and unknown values, as those are not allowed for blocks. We also revert the change to formatting nested object types as single attributes, because the attribute formatter cannot handle sensitive values from the schema. This presents some awkward syntax for diffs for now, but should suffice until the entire formatter can be refactored to better handle these new nested types. --- internal/command/format/diff.go | 63 ++++---- internal/command/format/diff_test.go | 212 ++++++++++++++++++++++++++- 2 files changed, 247 insertions(+), 28 deletions(-) diff --git a/internal/command/format/diff.go b/internal/command/format/diff.go index 686b6e6f0..258c979a5 100644 --- a/internal/command/format/diff.go +++ b/internal/command/format/diff.go @@ -471,28 +471,8 @@ func (p *blockBodyDiffPrinter) writeAttrDiff(name string, attrS *configschema.At } if attrS.NestedType != nil { - renderNested := true - - // If the collection values are empty or null, we render them as single attributes - switch attrS.NestedType.Nesting { - case configschema.NestingList, configschema.NestingSet, configschema.NestingMap: - var oldLen, newLen int - if !old.IsNull() && old.IsKnown() { - oldLen = old.LengthInt() - } - if !new.IsNull() && new.IsKnown() { - newLen = new.LengthInt() - } - - if oldLen+newLen == 0 { - renderNested = false - } - } - - if renderNested { - p.writeNestedAttrDiff(name, attrS.NestedType, old, new, nameLen, indent, path, action, showJustNew) - return false - } + p.writeNestedAttrDiff(name, attrS.NestedType, old, new, nameLen, indent, path, action, showJustNew) + return false } p.buf.WriteString("\n") @@ -626,15 +606,24 @@ func (p *blockBodyDiffPrinter) writeNestedAttrDiff( p.buf.WriteString(strings.Repeat(" ", indent+2)) p.buf.WriteString("]") + if !new.IsKnown() { + p.buf.WriteString(" -> (known after apply)") + } + case configschema.NestingSet: oldItems := ctyCollectionValues(old) newItems := ctyCollectionValues(new) - allItems := make([]cty.Value, 0, len(oldItems)+len(newItems)) - allItems = append(allItems, oldItems...) - allItems = append(allItems, newItems...) + var all cty.Value + if len(oldItems)+len(newItems) > 0 { + allItems := make([]cty.Value, 0, len(oldItems)+len(newItems)) + allItems = append(allItems, oldItems...) + allItems = append(allItems, newItems...) - all := cty.SetVal(allItems) + all = cty.SetVal(allItems) + } else { + all = cty.SetValEmpty(old.Type().ElementType()) + } p.buf.WriteString(" = [") @@ -646,6 +635,13 @@ func (p *blockBodyDiffPrinter) writeNestedAttrDiff( case !val.IsKnown(): action = plans.Update newValue = val + case !new.IsKnown(): + action = plans.Delete + // the value must have come from the old set + oldValue = val + // Mark the new val as null, but the entire set will be + // displayed as "(unknown after apply)" + newValue = cty.NullVal(val.Type()) case old.IsNull() || !old.HasElement(val).True(): action = plans.Create oldValue = cty.NullVal(val.Type()) @@ -680,6 +676,10 @@ func (p *blockBodyDiffPrinter) writeNestedAttrDiff( p.buf.WriteString(strings.Repeat(" ", indent+2)) p.buf.WriteString("]") + if !new.IsKnown() { + p.buf.WriteString(" -> (known after apply)") + } + case configschema.NestingMap: // For the sake of handling nested blocks, we'll treat a null map // the same as an empty map since the config language doesn't @@ -688,7 +688,12 @@ func (p *blockBodyDiffPrinter) writeNestedAttrDiff( new = ctyNullBlockMapAsEmpty(new) oldItems := old.AsValueMap() - newItems := new.AsValueMap() + + newItems := map[string]cty.Value{} + + if new.IsKnown() { + newItems = new.AsValueMap() + } allKeys := make(map[string]bool) for k := range oldItems { @@ -710,6 +715,7 @@ func (p *blockBodyDiffPrinter) writeNestedAttrDiff( for _, k := range allKeysOrder { var action plans.Action oldValue := oldItems[k] + newValue := newItems[k] switch { case oldValue == cty.NilVal: @@ -745,6 +751,9 @@ func (p *blockBodyDiffPrinter) writeNestedAttrDiff( p.writeSkippedElems(unchanged, indent+4) p.buf.WriteString(strings.Repeat(" ", indent+2)) p.buf.WriteString("}") + if !new.IsKnown() { + p.buf.WriteString(" -> (known after apply)") + } } return diff --git a/internal/command/format/diff_test.go b/internal/command/format/diff_test.go index fa6558f89..31cb62d7d 100644 --- a/internal/command/format/diff_test.go +++ b/internal/command/format/diff_test.go @@ -2633,6 +2633,56 @@ func TestResourceChange_nestedList(t *testing.T) { ~ attr = "y" -> "z" } } +`, + }, + "in-place update - unknown": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + "disks": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "mount_point": cty.StringVal("/var/diska"), + "size": cty.StringVal("50GB"), + }), + }), + "root_block_device": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "volume_type": cty.StringVal("gp2"), + "new_field": cty.StringVal("new_value"), + }), + }), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-AFTER"), + "disks": cty.UnknownVal(cty.List(cty.Object(map[string]cty.Type{ + "mount_point": cty.String, + "size": cty.String, + }))), + "root_block_device": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "volume_type": cty.StringVal("gp2"), + "new_field": cty.StringVal("new_value"), + }), + }), + }), + RequiredReplace: cty.NewPathSet(), + Schema: testSchemaPlus(configschema.NestingList), + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + ~ ami = "ami-BEFORE" -> "ami-AFTER" + ~ disks = [ + ~ { + - mount_point = "/var/diska" -> null + - size = "50GB" -> null + }, + ] -> (known after apply) + id = "i-02ae66f368e8518a9" + + # (1 unchanged block hidden) + } `, }, } @@ -2893,7 +2943,8 @@ func TestResourceChange_nestedSet(t *testing.T) { ExpectedOutput: ` # test_instance.example will be updated in-place ~ resource "test_instance" "example" { ~ ami = "ami-BEFORE" -> "ami-AFTER" - + disks = [] + + disks = [ + ] id = "i-02ae66f368e8518a9" } `, @@ -2952,6 +3003,56 @@ func TestResourceChange_nestedSet(t *testing.T) { - volume_type = "gp2" -> null } } +`, + }, + "in-place update - unknown": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + "disks": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "mount_point": cty.StringVal("/var/diska"), + "size": cty.StringVal("50GB"), + }), + }), + "root_block_device": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "volume_type": cty.StringVal("gp2"), + "new_field": cty.StringVal("new_value"), + }), + }), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-AFTER"), + "disks": cty.UnknownVal(cty.Set(cty.Object(map[string]cty.Type{ + "mount_point": cty.String, + "size": cty.String, + }))), + "root_block_device": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "volume_type": cty.StringVal("gp2"), + "new_field": cty.StringVal("new_value"), + }), + }), + }), + RequiredReplace: cty.NewPathSet(), + Schema: testSchemaPlus(configschema.NestingSet), + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + ~ ami = "ami-BEFORE" -> "ami-AFTER" + ~ disks = [ + - { + - mount_point = "/var/diska" -> null + - size = "50GB" -> null + }, + ] -> (known after apply) + id = "i-02ae66f368e8518a9" + + # (1 unchanged block hidden) + } `, }, } @@ -3288,6 +3389,115 @@ func TestResourceChange_nestedMap(t *testing.T) { - volume_type = "gp2" -> null } } +`, + }, + "in-place update - unknown": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + "disks": cty.MapVal(map[string]cty.Value{ + "disk_a": cty.ObjectVal(map[string]cty.Value{ + "mount_point": cty.StringVal("/var/diska"), + "size": cty.StringVal("50GB"), + }), + }), + "root_block_device": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "volume_type": cty.StringVal("gp2"), + "new_field": cty.StringVal("new_value"), + }), + }), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-AFTER"), + "disks": cty.UnknownVal(cty.Map(cty.Object(map[string]cty.Type{ + "mount_point": cty.String, + "size": cty.String, + }))), + "root_block_device": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "volume_type": cty.StringVal("gp2"), + "new_field": cty.StringVal("new_value"), + }), + }), + }), + RequiredReplace: cty.NewPathSet(), + Schema: testSchemaPlus(configschema.NestingMap), + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + ~ ami = "ami-BEFORE" -> "ami-AFTER" + ~ disks = { + - "disk_a" = { + - mount_point = "/var/diska" -> null + - size = "50GB" -> null + }, + } -> (known after apply) + id = "i-02ae66f368e8518a9" + + # (1 unchanged block hidden) + } +`, + }, + "in-place update - insertion sensitive": { + Action: plans.Update, + Mode: addrs.ManagedResourceMode, + Before: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-BEFORE"), + "disks": cty.MapValEmpty(cty.Object(map[string]cty.Type{ + "mount_point": cty.String, + "size": cty.String, + })), + "root_block_device": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "volume_type": cty.StringVal("gp2"), + "new_field": cty.StringVal("new_value"), + }), + }), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("i-02ae66f368e8518a9"), + "ami": cty.StringVal("ami-AFTER"), + "disks": cty.MapVal(map[string]cty.Value{ + "disk_a": cty.ObjectVal(map[string]cty.Value{ + "mount_point": cty.StringVal("/var/diska"), + "size": cty.StringVal("50GB"), + }), + }), + "root_block_device": cty.MapVal(map[string]cty.Value{ + "a": cty.ObjectVal(map[string]cty.Value{ + "volume_type": cty.StringVal("gp2"), + "new_field": cty.StringVal("new_value"), + }), + }), + }), + AfterValMarks: []cty.PathValueMarks{ + { + Path: cty.Path{cty.GetAttrStep{Name: "disks"}, + cty.IndexStep{Key: cty.StringVal("disk_a")}, + cty.GetAttrStep{Name: "mount_point"}, + }, + Marks: cty.NewValueMarks(marks.Sensitive), + }, + }, + RequiredReplace: cty.NewPathSet(), + Schema: testSchemaPlus(configschema.NestingMap), + ExpectedOutput: ` # test_instance.example will be updated in-place + ~ resource "test_instance" "example" { + ~ ami = "ami-BEFORE" -> "ami-AFTER" + ~ disks = { + + "disk_a" = { + + mount_point = (sensitive) + + size = "50GB" + }, + } + id = "i-02ae66f368e8518a9" + + # (1 unchanged block hidden) + } `, }, }