Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Diffs are hard to read #367

Open
errm opened this issue Feb 11, 2021 · 6 comments
Open

Diffs are hard to read #367

errm opened this issue Feb 11, 2021 · 6 comments
Labels
enhancement New feature or request feature request help wanted requires internal discussion The issue/PR requires further internal discussion to understand if it's appropriate for the project.

Comments

@errm
Copy link

errm commented Feb 11, 2021

Terraform Version

Terraform v0.14.6
+ provider registry.terraform.io/fastly/fastly v0.24.0

Affected Resource(s)

  • fastly_service_v1

Expected Behavior

What should have happened?

When making a change to attributes in a backend block, or a few lines of a custom vcl I would like to see a diff that explained what changed.

e.g.

  # fastly_service_v1.example will be updated in-place
  ~ resource "fastly_service_v1" "example" {
        id             = "redacted"
        name           = "example.example.org"
        # (5 unchanged attributes hidden)

      ~ backend {
          ~ first_byte_timeout    = 21000 -> 1500
          # (n unchanged attributes hidden)
        }

        # (8 unchanged blocks hidden)
    }

Actual Behavior

What actually happened?

changing any attribute in a block shows it being replaced. This makes it much harder to review the changes being made to the service by looking at the diff.

The user experiance e.g. when looking at the diff in the fastly web UI is much nicer.


  # fastly_service_v1.global-api-test will be updated in-place
  ~ resource "fastly_service_v1" "example" {
        id             = "redacted"
        name           = "example.example.org"
        # (5 unchanged attributes hidden)

      + backend {
          + address               = "backend.example.org"
          + auto_loadbalance      = false
          + between_bytes_timeout = 10000
          + connect_timeout       = 1000
          + error_threshold       = 0
          + first_byte_timeout    = 1500
          + max_conn              = 200
          + name                  = "example-backend"
          + port                  = 443
        }
      - backend {
          - address               = "backend.example.org" -> null
          - auto_loadbalance      = false -> null
          - between_bytes_timeout = 10000 -> null
          - connect_timeout       = 1000 -> null
          - error_threshold       = 0 -> null
          - first_byte_timeout    = 21000 -> null
          - max_conn              = 200 -> null
          - name                  = "example-backend" -> null
          - port                  = 443 -> null
        }

        # (8 unchanged blocks hidden)
    }

I am not totally sure if this is technically possible, but I don't recall this kind of experiance e.g. when using the AWS provider.

Presumably if the blocks were keyed of their name it would be possible to diff the old and the new block in a more useful way than is currently output.

@errm
Copy link
Author

errm commented Feb 26, 2021

I spent some time looking into this.

As far as I can work out this kind of behaviour is not currently possible with the current version of terraform.

Since nested blocks map directly to terraform's Set type there identity is based on a hash of all atributes on the object.


Some possible solutions to this could be:

  1. Using a map, would break backwards compat.

e.g:

resource "fastly_service_v1" "example" {
 
  ...

  backends = {
    example-backend = {
          address               = "backend.example.org"
          auto_loadbalance      = false
          between_bytes_timeout = 10000
          connect_timeout       = 1000
          error_threshold       = 0
          first_byte_timeout    = 1500
          max_conn              = 200
          name                  = "example-backend"
          port                  = 443
    },
    other-backend = {
          address               = "other.example.org"
          auto_loadbalance      = false
          between_bytes_timeout = 10000
          connect_timeout       = 1000
          error_threshold       = 0
          first_byte_timeout    = 1500
          max_conn              = 200
          name                  = "other-backend"
          port                  = 443
    } 
  }
}
  1. Get terraform to provide a way for nested blocks/sets to specify identity.

In the example of backends, really we just want to use a hash of the name for set identity.

I did manage to make some changes to the provider code to achieve this, but when that hash is equal terraform treats the plan as a noop, so there clearly would need to be some changes to terraform core to support this.

  1. parse terraform plans with a script and render a nicer diff...

For now I am using terraform show -json to spit out the plan then using a script to print a nicer diff, it also lets me shell out to diff in order to provide a nice linewise diff of changes to VCL!

@MasonM
Copy link

MasonM commented Apr 11, 2022

Here's a script that can be used to nicely format plan diffs (adapted from https://gist.github.com/Integralist/2b4298d4d287376b8a939c4e9eadd693)

#!/bin/bash
terraform plan -out=tfplan > /dev/null 2>&1
CHANGES=$(terraform show -json tfplan | jq '.resource_changes[].change')
diff -u <(echo "$CHANGES" | jq '.before' | sed 's/\\n/\n/g') <(echo "$CHANGES" | jq '.after' | sed 's/\\n/\n/g')

@a-suenami
Copy link

I have the same problem. Is there a plan to improve it?

@Integralist Integralist added enhancement New feature or request help wanted feature request requires internal discussion The issue/PR requires further internal discussion to understand if it's appropriate for the project. labels Sep 28, 2022
@Integralist
Copy link
Collaborator

Integralist commented Jan 16, 2023

👋🏻 @a-suenami there is a new Hashicorp plugin framework (incompatible with their v2 SDK that the Fastly provider uses) and in the new framework HashiCorp recommend moving away from nested blocks to nested attributes. This would be a breaking interface change for the Fastly provider but it might also be something that helps to improve the diff issue reported here (related: #597). Additionally, there is a new 'map' data type with the new framework that might help the diff issue.

@Integralist
Copy link
Collaborator

UPDATE: The new HashiCorp framework's map nested attribute type does indeed help with diff clarity.

See Integralist/terraform-provider-fastly-framework#7 for details.

@pavelpulec
Copy link

Hello,
is there any update related to that?

We are fine with implementing that as a breaking change. If you provide steps how to migrate or we can start re-create our resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature request help wanted requires internal discussion The issue/PR requires further internal discussion to understand if it's appropriate for the project.
Projects
None yet
Development

No branches or pull requests

5 participants