command/format: improve list nested attribute rendering (hashicorp#29827

)

* command/format: fix list nested attr diff rendering

Previously, diffs only rendered correctly if all changed elements
appeared before all unchanged elements. Once an unchanged element was
found, all remaining elements were skipped. This usually led to the
output being an empty list with a weird amount of space between the
brackets.

* command/format: improve list nested attr rendering

This makes several changes that make diffs for lists clearer and more
consistent:

  * Separate items with brackets instead of only new lines. This better
    matches the input syntax and avoids confusion from the first and
    last brackets implying there is a single item.
  * Render an action symbol for each element of the list
  * Use the correct action symbol for new and deleted items
  * Fix the alignment of opening and closing brackets

I also refactored the structure so it is similar to the set and map
cases to minimize duplication of the new prints.

* Fix re-use of blockBodyDiffResult struct

internal/command/format/diff.go

@@ -473,9 +473,6 @@ func (p *blockBodyDiffPrinter) writeNestedAttrDiff(
  p.buf.WriteString(p.color.Color(forcesNewResourceCaption))
  }
  p.buf.WriteString("\n")
- p.buf.WriteString(strings.Repeat(" ", indent+4))
- p.writeActionSymbol(action)
- p.buf.WriteString("{")
 
  oldItems := ctyCollectionValues(old)
  newItems := ctyCollectionValues(new)
@@ -489,48 +486,65 @@ func (p *blockBodyDiffPrinter) writeNestedAttrDiff(
  // commonLen is number of elements that exist in both lists, which
  // will be presented as updates (~). Any additional items in one
  // of the lists will be presented as either creates (+) or deletes (-)
- // depending on which list they belong to.
+ // depending on which list they belong to. maxLen is the number of
+ // elements in that longer list.
  var commonLen int
+ var maxLen int
  // unchanged is the number of unchanged elements
  var unchanged int
 
  switch {
  case len(oldItems) < len(newItems):
  commonLen = len(oldItems)
+ maxLen = len(newItems)
  default:
  commonLen = len(newItems)
+ maxLen = len(oldItems)
  }
- for i := 0; i < commonLen; i++ {
+ for i := 0; i < maxLen; i++ {
  path := append(path, cty.IndexStep{Key: cty.NumberIntVal(int64(i))})
- oldItem := oldItems[i]
- newItem := newItems[i]
- if oldItem.RawEquals(newItem) {
+
+ var action plans.Action
+ var oldItem, newItem cty.Value
+ switch {
+ case i < commonLen:
+ oldItem = oldItems[i]
+ newItem = newItems[i]
+ if oldItem.RawEquals(newItem) {
+ action = plans.NoOp
+ unchanged++
+ } else {
+ action = plans.Update
+ }
+ case i < len(oldItems):
+ oldItem = oldItems[i]
+ newItem = cty.NullVal(oldItem.Type())
+ action = plans.Delete
+ case i < len(newItems):
+ newItem = newItems[i]
+ oldItem = cty.NullVal(newItem.Type())
+ action = plans.Create
+ default:
  action = plans.NoOp
- unchanged++
  }
+
  if action != plans.NoOp {
- p.writeAttrsDiff(objS.Attributes, oldItem, newItem, indent+6, path, result)
- p.writeSkippedAttr(result.skippedAttributes, indent+8)
+ p.buf.WriteString(strings.Repeat(" ", indent+4))
+ p.writeActionSymbol(action)
+ p.buf.WriteString("{")
+
+ result := &blockBodyDiffResult{}
+ p.writeAttrsDiff(objS.Attributes, oldItem, newItem, indent+8, path, result)
+ if action == plans.Update {
+ p.writeSkippedAttr(result.skippedAttributes, indent+10)
+ }
  p.buf.WriteString("\n")
+
+ p.buf.WriteString(strings.Repeat(" ", indent+6))
+ p.buf.WriteString("},\n")
  }
  }
- for i := commonLen; i < len(oldItems); i++ {
- path := append(path, cty.IndexStep{Key: cty.NumberIntVal(int64(i))})
- oldItem := oldItems[i]
- newItem := cty.NullVal(oldItem.Type())
- p.writeAttrsDiff(objS.Attributes, oldItem, newItem, indent+6, path, result)
- p.buf.WriteString("\n")
- }
- for i := commonLen; i < len(newItems); i++ {
- path := append(path, cty.IndexStep{Key: cty.NumberIntVal(int64(i))})
- newItem := newItems[i]
- oldItem := cty.NullVal(newItem.Type())
- p.writeAttrsDiff(objS.Attributes, oldItem, newItem, indent+6, path, result)
- p.buf.WriteString("\n")
- }
- p.buf.WriteString(strings.Repeat(" ", indent+4))
- p.buf.WriteString("},\n")
- p.writeSkippedElems(unchanged, indent+4)
+ p.writeSkippedElems(unchanged, indent+6)
  p.buf.WriteString(strings.Repeat(" ", indent+2))
  p.buf.WriteString("]")
 

internal/command/format/diff_test.go

@@ -2263,10 +2263,10 @@ func TestResourceChange_nestedList(t *testing.T) {
  ~ resource "test_instance" "example" {
  ~ ami = "ami-BEFORE" -> "ami-AFTER"
  ~ disks = [
- ~ {
- + mount_point = "/var/diska"
- + size = "50GB"
- },
+ + {
+  + mount_point = "/var/diska"
+  + size = "50GB"
+  },
  ]
  id = "i-02ae66f368e8518a9"
 
@@ -2309,9 +2309,9 @@ func TestResourceChange_nestedList(t *testing.T) {
  ~ resource "test_instance" "example" {
  ~ ami = "ami-BEFORE" -> "ami-AFTER"
  ~ disks = [
- ~ {
- + mount_point = "/var/diska"
- },
+ + {
+  + mount_point = "/var/diska"
+  },
  ]
  id = "i-02ae66f368e8518a9"
 
@@ -2371,10 +2371,10 @@ func TestResourceChange_nestedList(t *testing.T) {
  ~ ami = "ami-BEFORE" -> "ami-AFTER"
  ~ disks = [
  ~ {
- + size = "50GB"
- # (1 unchanged attribute hidden)
- },
- # (1 unchanged element hidden)
+  + size = "50GB"
+  # (1 unchanged attribute hidden)
+  },
+  # (1 unchanged element hidden)
  ]
  id = "i-02ae66f368e8518a9"
 
@@ -2437,9 +2437,9 @@ func TestResourceChange_nestedList(t *testing.T) {
  ~ ami = "ami-BEFORE" -> "ami-AFTER"
  ~ disks = [
  ~ {
- ~ mount_point = "/var/diska" -> "/var/diskb" # forces replacement
- # (1 unchanged attribute hidden)
- },
+  ~ mount_point = "/var/diska" -> "/var/diskb" # forces replacement
+  # (1 unchanged attribute hidden)
+  },
  ]
  id = "i-02ae66f368e8518a9"
 
@@ -2493,9 +2493,9 @@ func TestResourceChange_nestedList(t *testing.T) {
  ~ ami = "ami-BEFORE" -> "ami-AFTER"
  ~ disks = [ # forces replacement
  ~ {
- ~ mount_point = "/var/diska" -> "/var/diskb"
- # (1 unchanged attribute hidden)
- },
+  ~ mount_point = "/var/diska" -> "/var/diskb"
+  # (1 unchanged attribute hidden)
+  },
  ]
  id = "i-02ae66f368e8518a9"
 
@@ -2540,10 +2540,10 @@ func TestResourceChange_nestedList(t *testing.T) {
  ~ resource "test_instance" "example" {
  ~ ami = "ami-BEFORE" -> "ami-AFTER"
  ~ disks = [
- ~ {
- - mount_point = "/var/diska" -> null
- - size = "50GB" -> null
- },
+ - {
+  - mount_point = "/var/diska" -> null
+  - size = "50GB" -> null
+  },
  ]
  id = "i-02ae66f368e8518a9"
 
@@ -2674,13 +2674,86 @@ func TestResourceChange_nestedList(t *testing.T) {
  ~ resource "test_instance" "example" {
  ~ ami = "ami-BEFORE" -> "ami-AFTER"
  ~ disks = [
- ~ {
- - mount_point = "/var/diska" -> null
- - size = "50GB" -> null
- },
+ - {
+  - mount_point = "/var/diska" -> null
+  - size = "50GB" -> null
+  },
  ] -> (known after apply)
  id = "i-02ae66f368e8518a9"
 
+ # (1 unchanged block hidden)
+ }
+`,
+ },
+ "in-place update - modification": {
+ 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"),
+ }),
+ cty.ObjectVal(map[string]cty.Value{
+ "mount_point": cty.StringVal("/var/diskb"),
+ "size": cty.StringVal("50GB"),
+ }),
+ cty.ObjectVal(map[string]cty.Value{
+ "mount_point": cty.StringVal("/var/diskc"),
+ "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.ListVal([]cty.Value{
+ cty.ObjectVal(map[string]cty.Value{
+ "mount_point": cty.StringVal("/var/diska"),
+ "size": cty.StringVal("50GB"),
+ }),
+ cty.ObjectVal(map[string]cty.Value{
+ "mount_point": cty.StringVal("/var/diskb"),
+ "size": cty.StringVal("75GB"),
+ }),
+ cty.ObjectVal(map[string]cty.Value{
+ "mount_point": cty.StringVal("/var/diskc"),
+ "size": cty.StringVal("25GB"),
+ }),
+ }),
+ "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 = [
+ ~ {
+ ~ size = "50GB" -> "75GB"
+ # (1 unchanged attribute hidden)
+ },
+ ~ {
+ ~ size = "50GB" -> "25GB"
+ # (1 unchanged attribute hidden)
+ },
+ # (1 unchanged element hidden)
+ ]
+ id = "i-02ae66f368e8518a9"
+
  # (1 unchanged block hidden)
  }
 `,