Multi-line diffs are not displayed properly sometimes

[lawliet89]

Terraform Version

Terraform v0.12.2
+ provider.consul v2.5.0
+ provider.google v2.9.0
+ provider.google-beta v2.9.0
+ provider.helm v0.10.0
+ provider.kubernetes v1.7.1 (custom build with Terraform 0.12 support merged in but unreleased)
+ provider.template v2.1.2

Terraform Configuration Files

Relevant part: https://github.com/basisai/terraform-modules-gcp/blob/d7ff455/modules/consul/esm.tf

Expected Behavior

When showing the diff during plans, the multi-line strings in values should be displayed properly as per #15180

Actual Behavior

The multi-line diffs are still displayed in an unreadable manner.

  # module.workload_services.module.consul.helm_release.consul_esm[0] will be updated in-place
  ~ resource "helm_release" "consul_esm" {
        chart            = "consul-esm"
        disable_webhooks = false
        force_update     = false
        id               = "consul-esm"
        metadata         = [
            {
                chart     = "consul-esm"
                name      = "consul-esm"
                namespace = "core"
                revision  = 1
                values    = "config:\n  externalNodeMeta:\n    external-node: \"true\"\n  httpAddr: \"\"\n  kvPath: consul-esm/\n  logLevel: INFO\n  nodeProbeInterval: 10s\n  nodeReconnectTimeout: 72h\n  pingType: udp\n  serviceName: consul-esm\n  serviceTag: \"\"\nenv:\n- name: HOST_IP\n  valueFrom:\n    fieldRef:\n      fieldPath: status.hostIP\n- name: CONSUL_HTTP_ADDR\n  value: $(HOST_IP):8500\nimage:\n  repository: basisai/consul-esm\n  tag: 0.3.3\ninitContainerSetSysCtl: 0\nreplicaCount: 3\nresources:\n  limits:\n    memory: 256Mi\n  requests:\n    cpu: 200m\n"
                version   = "0.1.0"
            },
        ]
        name             = "consul-esm"
        namespace        = "core"
        recreate_pods    = false
        repository       = "amoy"
        reuse            = false
        reuse_values     = false
        status           = "DEPLOYED"
        timeout          = 300
      ~ values           = [
          - "replicaCount: 3\n\nimage:\n  repository: basisai/consul-esm\n  tag: 0.3.3\n\nresources: {\"limits\":{\"memory\":\"256Mi\"},\"requests\":{\"cpu\":\"200m\"}}\n  # We usually recommend not to specify default resources and to leave this as a conscious\n  # choice for the user. This also increases chances charts run on environments with little\n  # resources, such as Minikube. If you do want to specify resources, uncomment the following\n  # lines, adjust them as necessary, and remove the curly braces after 'resources:'.\n  # limits:\n  #   cpu: 100m\n  #   memory: 128Mi\n  # requests:\n  #   cpu: 100m\n  #   memory: 128Mi\n\nenv: [{\"name\":\"HOST_IP\",\"valueFrom\":{\"fieldRef\":{\"fieldPath\":\"status.hostIP\"}}},{\"name\":\"CONSUL_HTTP_ADDR\",\"value\":\"$(HOST_IP):8500\"}]\n\ninitContainerSetSysCtl: 0\n\nconfig:\n  logLevel: \"INFO\"\n\n  # The service name for this agent to use when registering itself with Consul.\n  serviceName: \"consul-esm\"\n\n  # The service tag for this agent to use when registering itself with Consul.\n  # ESM instances that share a service name/tag combination will have the work\n  # of running health checks and pings for any external nodes in the catalog\n  # divided evenly amongst themselves.\n  serviceTag: \"\"\n\n  # The directory in the Consul KV store to use for storing runtime data.\n  kvPath: \"consul-esm/\"\n\n  # The node metadata values used for the ESM to qualify a node in the catalog\n  # as an \"external node\".\n\n  externalNodeMeta: {\"external-node\":\"true\"}\n  # The length of time to wait before reaping an external node due to failed\n  # pings.\n  nodeReconnectTimeout: \"72h\"\n\n  # The interval to ping and update coordinates for external nodes that have\n  # 'external-probe' set to true. By default, ESM will attempt to ping and\n  # update the coordinates for all nodes it is watching every 10 seconds.\n  nodeProbeInterval: \"10s\"\n\n  # The address of the local Consul agent. Can also be provided through the\n  # CONSUL_HTTP_ADDR environment variable.\n  httpAddr: \"\"\n\n  # The method to use for pinging external nodes. Defaults to \"udp\" but can\n  # also be set to \"socket\" to use ICMP (which requires root privileges).\n  pingType: \"udp\"\n",
          + "replicaCount: 3\n\nimage:\n  repository: basisai/consul-esm\n  tag: 0.3.3\n\nresources: {\"limits\":{\"memory\":\"256Mi\"},\"requests\":{\"cpu\":\"200m\"}}\n  # We usually recommend not to specify default resources and to leave this as a conscious\n  # choice for the user. This also increases chances charts run on environments with little\n  # resources, such as Minikube. If you do want to specify resources, uncomment the following\n  # lines, adjust them as necessary, and remove the curly braces after 'resources:'.\n  # limits:\n  #   cpu: 100m\n  #   memory: 128Mi\n  # requests:\n  #   cpu: 100m\n  #   memory: 128Mi\n\nenv: [{\"name\":\"HOST_IP\",\"valueFrom\":{\"fieldRef\":{\"fieldPath\":\"status.hostIP\"}}},{\"name\":\"CONSUL_HTTP_ADDR\",\"value\":\"$(HOST_IP):8500\"}]\n\ninitContainerSetSysCtl: false\n\nconfig:\n  logLevel: \"INFO\"\n\n  # The service name for this agent to use when registering itself with Consul.\n  serviceName: \"consul-esm\"\n\n  # The service tag for this agent to use when registering itself with Consul.\n  # ESM instances that share a service name/tag combination will have the work\n  # of running health checks and pings for any external nodes in the catalog\n  # divided evenly amongst themselves.\n  serviceTag: \"\"\n\n  # The directory in the Consul KV store to use for storing runtime data.\n  kvPath: \"consul-esm/\"\n\n  # The node metadata values used for the ESM to qualify a node in the catalog\n  # as an \"external node\".\n\n  externalNodeMeta: {\"external-node\":\"true\"}\n  # The length of time to wait before reaping an external node due to failed\n  # pings.\n  nodeReconnectTimeout: \"72h\"\n\n  # The interval to ping and update coordinates for external nodes that have\n  # 'external-probe' set to true. By default, ESM will attempt to ping and\n  # update the coordinates for all nodes it is watching every 10 seconds.\n  nodeProbeInterval: \"10s\"\n\n  # The address of the local Consul agent. Can also be provided through the\n  # CONSUL_HTTP_ADDR environment variable.\n  httpAddr: \"\"\n\n  # The method to use for pinging external nodes. Defaults to \"udp\" but can\n  # also be set to \"socket\" to use ICMP (which requires root privileges).\n  pingType: \"udp\"\n",
        ]
        verify           = false
        version          = "0.1.0"
        wait             = true
    }

Steps to Reproduce

terraform plan

Additional Context

The relevant lines in the provider doesn't seem to be out of the ordinary. I am not sure if this is a problem with the provider itself, or with Terraform.

Or, it might be because the values were provided by the template_file data source.

(EDIT: Replacing it with the templatefile() function does not make a difference.)

I have seen proper diffs for other resources, though.

[apparentlymart]

Hi @lawliet89! Thanks for reporting this inconsistency.

I think the reason for this difference is that these particular attributes are not being updated by this change, and so there isn't a pair of old and new values to show. Instead, it is just showing the old value, and thus Terraform uses a different codepath to render it.

However, this all-on-one-line form isn't readable even if there are no changes, so I agree that Terraform ought to be consistently using the same formatting in both cases, which in this situation would mean just printing out the string over multiple lines even though it won't have any + and - line markers in this situation.

[lawliet89]

Hey @apparentlymart, there is a change in the diff I pasted above, though. It's under values. There's a + and - sign in that multi-line string. It's one of the last few attributes.

[apparentlymart]

Ahh yes, I didn't spot that! That situation is a different one: in that case, Terraform is treating it as a change to the list rather than a change to the string, and Terraform uses an "element-oriented" diff (similar to the line-oriented diff it would normally use for multi-line strings not inside lists) and that diff algorithm can currently produce only "remove" and "add" changes, not edit-in-place changes, because it prefers to be able to show insertions into the list as additions rather than as edits to every subsequent unchanged element.

To improve that one would require some additional functionality: the diff renderer would need to check any case where there's a removal immediately followed by an addition, see if both are strings, and then use a similarity heuristic to decide whether to transform that pair of changes into a single "update in-place" (~) change.

Unfortunately diffing algorithms tend to be very heuristic-based, because it needs to work backwards from just the old and new values to try to infer what change is likely to have caused that difference. We can tweak these tradeoffs to try to maximize readability in common cases, but it'll never be possible for it to get this totally right every time: there are lots of different ways to represent a change between two values.

We'll see what we can do to improve this, though. Addressing the problem I described in my previous comment is likely to be more straightforward, so we may wish to address that one separately first.

[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.