Proposal: improve display of Set diffs

[Bowbaq]

Context

I'm currently using terraform to manage 10 Elastic Beanstalk environments (with #3157). Each environment has ~30 setting sub-resources, and that number is growing as we tailor the defaults provided by EB.

A typical terraform plan output when changing one of the settings looks like this. Check it out, you'll see it's basically impossible to tell what's going on. This particular change was across all 10 environments...

Proposal

I think the diff output for sets can be vastly improved by making a few changes:

• Only print the values that changed, omit the ones that didn't. Maybe this should be heuristically conditioned on the size of the set (ie. for a small set it's ok to display everything?)
• When a value gets updated, properly display the update instead of a addition/deletion pair. I think this could be achieved by adding an optional "key hash function" to the set implementation. If that function is provided, then an addition/deletion pair where the key hash is identical on both sides is an update.

I think the first step should be fairly straightforward. Second one is maybe a stretch goal. @phinze what do you think?

[phinze]

I'm very keen on improving Set diff output!

Some initial implementation pointers to guide you as you start to poke around:

• The code that displays attribute diffs is pretty simple (read: dumb) right now. Plenty of room for improvement!
• There's a bit of an impedance mismatch we'll have to figure out how to address, in that an ResourceAttrDiff has no concept of a "Set" - it sees only the individual fields. The set begins life as a schema.Set and is broken down by the MapFieldWriter into individual attributes.
• We've got DiffFieldReader hanging around, which could potentially be a useful helper to pull in as we stitch back together the concept of a Set up at the display layer - but right now it has a lot of assumptions about being used by its single caller: schemaMap / ResourceData.
• I think you might be able to avoid most of this mess and still accomplish your first goal of hiding attribute diffs with no change - that might be best to play with first.

Hope this helps! Keep me posted as questions crop up.

[apparentlymart]

Over in #5065 a misunderstanding of that request caused me to start thinking about different presentation for collections (sets, lists and maps all) in diffs.

Repeating that comment here since this is a much more appropriate home for it. 😀

---

[snip...] Terraform doesn't do well at rendering diffs for sets. If this change is motivated entirely by diff readability, do you think we should instead try to find a better way for Terraform to present set diffs?

For example, Terraform could, in theory, detect the .# suffix and render the subsequent rows with the collection member prefix hidden, and a more traditional line-oriented diff:

    access_logs.#:       '2' => '2'
        * interval:      "60"
          bucket:        "logs_1"
          bucket_prefix: ""
      --- interval:      "60"
          bucket:        "logs_2"
          bucket_prefix: ""
      +++ interval:      "120"
          bucket:        "logs_2"
          bucket_prefix: ""

The intent here is to indicate that the set contained two items and one of them is unchanged while the other has been replaced by a new item.

Lists could be treated in the same way, since in that case too the indices are not really important. The relative ordering of the items is the main interesting thing in a List diff, so I think you'd just order the items by their prefix and then process the list in a similar way to how you'd diff lines of source code.

[Bowbaq]

@apparentlymart that's an interesting approach. I went a different way, which seems to at least fix my Elastic Beanstalk issue.

diff --git a/command/format_plan.go b/command/format_plan.go
index daf3f60..e2062d1 100644
--- a/command/format_plan.go
+++ b/command/format_plan.go
@@ -107,12 +107,17 @@ func formatPlanModuleExpand(
                // determine the longest key so that we can align them all.
                keyLen := 0
                keys := make([]string, 0, len(rdiff.Attributes))
-               for key, _ := range rdiff.Attributes {
+               prefixChanged := make(map[string]bool)
+               for key, attr := range rdiff.Attributes {
                        // Skip the ID since we do that specially
                        if key == "id" {
                                continue
                        }

+                       if attr.Old != attr.New || attr.NewComputed {
+                               prefixChanged[prefix(key)] = true
+                       }
+
                        keys = append(keys, key)
                        if len(key) > keyLen {
                                keyLen = len(key)
@@ -129,6 +134,12 @@ func formatPlanModuleExpand(
                                v = "<computed>"
                        }

+                       // If nothing changed, skip display. Prefix tracking helps display useful context for values that
+                       // did change
+                       if attrDiff.Old == attrDiff.New && !prefixChanged[prefix(attrK)] {
+                               continue
+                       }
+
                        newResource := ""
                        if attrDiff.RequiresNew && rdiff.Destroy {
                                newResource = opts.Color.Color(" [red](forces new resource)")
@@ -181,3 +192,12 @@ func formatPlanModuleSingle(
                len(m.Resources)))
        buf.WriteString(opts.Color.Color("[reset]\n"))
 }
+
+func prefix(key string) string {
+       parts := strings.Split(key, ".")
+       if len(parts) > 1 {
+               return strings.Join(parts[:len(parts)-1], ".")
+       }
+
+       return key
+}

This doesn't take care of the addition/deletion problem, but it makes things much more manageable.

[myoung34]

This is also partially duplicated by #7517 and #8950 with a WIP PR at #5207
I'm linking because I'm very interested in this fix.

[apparentlymart]

Hi @Bowbaq! Sorry for the long silence here.

(My previous comment on this issue was from when I was a community contributor. As I write this, I'm now an employee of HashiCorp working on the Terraform team.)

We've been doing some prototypes recently of using the new type system introduced by the configuration language improvements work in progress to give us more information to produce richer diffs, which includes line-by-line diffs of multi-line strings.

I posted a longer comment about this in #15180, which includes an example with some placeholder data, including per-element diffing for both set values and for nested blocks that are backed by sets, illustrated as vpc_security_group_ids and ebs_block_device in that example.

Now that we have a more concrete plan for improving the diff output, I'm going to close this just to consolidate the discussion over in #15180. I expect that we'll make all of the diff improvement changes together, since the new format is a total rewrite of the diff renderer with a new approach.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.