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

Improving perception of changes when showing diff #15180

Closed
danielcompton opened this issue Jun 8, 2017 · 12 comments
Closed

Improving perception of changes when showing diff #15180

danielcompton opened this issue Jun 8, 2017 · 12 comments

Comments

@danielcompton
Copy link
Contributor

danielcompton commented Jun 8, 2017

When I make a change to a large list of values (for example), the plan that Terraform shows is not as helpful as it could be. Here is what I see:

screenshot of terminal 8-06-17 3-16-32 pm

There are three important pieces of information here, but only one is clear:

  1. The resource google_project_services.project is changing (this is clear from colouring)
  2. The number of services in the list is changing from 21 -> 20
  3. The service that is being removed is "container.googleapis.com"

There is so much going on in the output that it is hard to easily see the relevant information. I have thought of some possible ways the output could be improved:

  • Highlighting lines that are changing with their related change colour (green, yellow, red for add, change, destroy respectively)
  • Aligning all of the => so it is easier to scan for changes
  • De-emphasising the hash (133405307) with a lighter colour as it's specific value is usually not very important (I think?)
  • Sorting attributes that are changing to the top
  • Removing unchanged attributes from the diff

I haven't really considered how default and computed values would be handled here, but it seems highly relevant and important.

Some mockups:

Highlighting changed lines and deemphasising hash

screenshot of terminal 8-06-17 3-17-41 pm

Align =>

~ google_project_services.project
    services.#:          "21" => "20"
    services.133405307:  "storage-component.googleapis.com"    => "storage-component.googleapis.com"
    services.1560437671: "iam.googleapis.com"                  => "iam.googleapis.com"
    services.1712537408: "containerregistry.googleapis.com"    => "containerregistry.googleapis.com"
    services.2117420113: "pubsub.googleapis.com"               => "pubsub.googleapis.com"
    services.238136042:  "cloudapis.googleapis.com"            => "cloudapis.googleapis.com"
    services.2471815660: "servicemanagement.googleapis.com"    => "servicemanagement.googleapis.com"
    services.2631575801: "sqladmin.googleapis.com"             => "sqladmin.googleapis.com"
    services.2928564140: "dns.googleapis.com"                  => "dns.googleapis.com"
    services.2966512281: "deploymentmanager.googleapis.com"    => "deploymentmanager.googleapis.com"
    services.3010261123: "replicapool.googleapis.com"          => "replicapool.googleapis.com"
    services.3075019877: "replicapoolupdater.googleapis.com"   => "replicapoolupdater.googleapis.com"
    services.3077910291: "resourceviews.googleapis.com"        => "resourceviews.googleapis.com"
    services.323125032:  "cloudtrace.googleapis.com"           => "cloudtrace.googleapis.com"
    services.3237295688: "monitoring.googleapis.com"           => "monitoring.googleapis.com"
    services.3355193353: "logging.googleapis.com"              => "logging.googleapis.com"
    services.3445186629: "sourcerepo.googleapis.com"           => "sourcerepo.googleapis.com"
    services.3644083179: "cloudresourcemanager.googleapis.com" => "cloudresourcemanager.googleapis.com"
    services.3731214611: "compute-component.googleapis.com"    => "compute-component.googleapis.com"
    services.3740470850: "container.googleapis.com"            => ""
    services.3875785048: "storage-api.googleapis.com"          => "storage-api.googleapis.com"
    services.77316126:   "sql-component.googleapis.com"        => "sql-component.googleapis.com"

Sorting changed attributes to top

~ google_project_services.project
    services.#:          "21" => "20"
    services.3740470850: "container.googleapis.com" => ""
        
    services.133405307:  "storage-component.googleapis.com" => "storage-component.googleapis.com"
    services.1560437671: "iam.googleapis.com" => "iam.googleapis.com"
    services.1712537408: "containerregistry.googleapis.com" => "containerregistry.googleapis.com"
    services.2117420113: "pubsub.googleapis.com" => "pubsub.googleapis.com"
    services.238136042:  "cloudapis.googleapis.com" => "cloudapis.googleapis.com"
    services.2471815660: "servicemanagement.googleapis.com" => "servicemanagement.googleapis.com"
    services.2631575801: "sqladmin.googleapis.com" => "sqladmin.googleapis.com"
    services.2928564140: "dns.googleapis.com" => "dns.googleapis.com"
    services.2966512281: "deploymentmanager.googleapis.com" => "deploymentmanager.googleapis.com"
    services.3010261123: "replicapool.googleapis.com" => "replicapool.googleapis.com"
    services.3075019877: "replicapoolupdater.googleapis.com" => "replicapoolupdater.googleapis.com"
    services.3077910291: "resourceviews.googleapis.com" => "resourceviews.googleapis.com"
    services.323125032:  "cloudtrace.googleapis.com" => "cloudtrace.googleapis.com"
    services.3237295688: "monitoring.googleapis.com" => "monitoring.googleapis.com"
    services.3355193353: "logging.googleapis.com" => "logging.googleapis.com"
    services.3445186629: "sourcerepo.googleapis.com" => "sourcerepo.googleapis.com"
    services.3644083179: "cloudresourcemanager.googleapis.com" => "cloudresourcemanager.googleapis.com"
    services.3731214611: "compute-component.googleapis.com" => "compute-component.googleapis.com"
    services.3875785048: "storage-api.googleapis.com" => "storage-api.googleapis.com"
    services.77316126:   "sql-component.googleapis.com" => "sql-component.googleapis.com"

Removing unchanged attributes

~ google_project_services.project
    services.#:          "21" => "20"
    services.3740470850: "container.googleapis.com" => ""
        
    services[]:          "20 values unchanged"

Relates partly to #15179.

@apparentlymart
Copy link
Contributor

Hi @danielcompton! Thanks for this feedback, and these suggestions.

This is similar to #5179, and there was some discussion about that over there too. A root cause for the sub-optimal output in cases like these is that currently the diff renderer has no awareness of data structures (sets, maps, lists) and instead operates on a flat map of strings, making inferences like this difficult.

We've been talking for a little while now about changing the way diffs are represented internally to capture type information and do a structure-oriented rather than a key-by-key diff, which would then give us a better foundation to make enhancements to the output like what you've suggested here.

Unfortunately this isn't something we're going to be able to do quickly, since it requires some pretty deep changes to Terraform's core API. It's something we would definitely like to do eventually, though.

@qsorix
Copy link

qsorix commented Jan 2, 2018

Hi. I'd like to +1 this. Today it's hard to maintain AWS security groups with terraform -- because ingress rules are a list too, and so diffs are impossible to read.

@apparentlymart
Copy link
Contributor

Hi all! Sorry for the long silence here.

The current focus for the Terraform team is to integrate some significant improvements to the configuration language that include a richer type system. Supporting this new type system gives us the information we need to implement better diff rendering as I hinted above, so we're working on prototyping this right now to see if it's something we can reasonably roll out along with the other configuration changes.

The following screenshot shows the current output of the prototype with some test data, showing the direction we're currently looking toward:

terraform-structural-diff-prototype-blocks

This new format is a pretty different approach than what's in current releases today, mimicking the configuration language syntax to hopefully better describe how Terraform has understood the given values and the proposed changes. The new type system is not yet implemented enough to illustrate this with a real state and config, but the above was produced by real code running against fake values of the same types that the state and config will have once the current work is complete.

The current prototype does not align the -> arrows because we saw some bad results when with adjacent values of significantly different lengths, which is quite common. Instead, we've colored the arrows to at least make them easier to scan for. I think that this makes a good compromise in conjunction with the indented rendering of collections and blocks that make the inherent structure easier to see.

You can also see here that the diff renderer has access to type information and so it's able to perform "deep" diffs on nested blocks and collection values, showing added/removed values in lists/sets/maps, added/removed lines in multi-line strings, and edits within nested blocks. The vpc_security_group_ids attribute shown here is the most direct analog to the example given in this issue's opening comment, since it's a set of strings.

This new approach is not fully fleshed out yet since there are some tricky situations that require heuristics, but we think this result is encouraging and plan to investigate further. We're not sure yet whether we'll be able to include this in the initial configuration language improvements release since that's already a pretty significant set of changes, but we will at least lay the groundwork to enable diff rendering like this to be added in a subsequent release.

@combor
Copy link
Contributor

combor commented Apr 8, 2018

Hi @apparentlymart

This is really promising improvement. Do you have any updates about it?

Cheers

@mikeputnam
Copy link

Until this issue is resolved, this small awk utterance works to only show changes.

$ terraform plan | awk -F"\"" '$4 != $2 { print $0 }'

  • -F sets the field separator to be quotes.
  • $4 != $2 compares the quoted values for inequality.
  • {print $0} only prints the line if it is caught by the inequality check.

Would not be printed: ingress.9999999999.description: "foo" => "foo"

Would be printed: ingress.9999999999.description: "foo" => "bar"

Chances are your host already has awk. :)

@Rowern
Copy link

Rowern commented Jul 19, 2018

@apparentlymart Do you have any update on this? It looks really awesome!!

@apparentlymart
Copy link
Contributor

Hi @Rowern! The situation hasn't changed from my comment above: we're hoping to be able to include this in the v0.12.0 release, but it's a stretch goal (since that release already has a large scope) and so it may end up arriving in a subsequent release.

The decision there will largely depend on how the existing prototype copes with a wide variety of real-world resource types. During prototyping we tested it only with some hard-coded aws_instance data since the goal was to mock out the UX rather than the implementation, and so when we take a deeper look into this (which'll probably be after all of the other 0.12 work is feature-complete) we'll try it with some more real-world resource types and configurations to see how much work seems to be left and thus whether we can fit it.

@yuyasat
Copy link

yuyasat commented Sep 7, 2018

I wrote simple ruby script like these. (These codes are pretty bad from the point of rubyist because I wrote in a short time)
I could not test carefully so please be carefull.

# In ~/.bashrc etc...
alias terr_colored_diff=$(cat <<EOS
  ruby -ne '
    input = \$_;
    if !input.nil? && input.include?("=>") then
      print input.split("\"")[-4] == input.split("\"")[-2] ? "#{input}\e[0m" : "\e[34m#{input}\e[0m"
    else
      print input + "\e[0m"
    end
  '
EOS
)
alias terr_delete_no_diff=$(cat <<EOS
  ruby -ne '
    input = \$_;
    if !input.nil? && input.include?("=>") then
      print input.split("\"")[-4] == input.split("\"")[-2] ? nil : "\e[34m#{input}\e[0m"
    else
      print "#{input}\e[0m"
    end
  '
EOS
)
terraform plan | terr_colored_diff
terraform plan | terr_delete_no_diff

@davidvasandani
Copy link

I think [landscape](https://github.com/coinbase/terraform-landscape) would solve all these issues. It's Alto compatible with terragrunt.

@marco-m
Copy link
Contributor

marco-m commented Sep 13, 2018

@davidneudorfer thanks for making me discover landscape!

@Rowern
Copy link

Rowern commented Oct 2, 2018

Any ETA on when 1.12 will be ready ?

Reading the blog post I see "References as first-class values" is still to be announced.

The other announcements are really exciting!

@apparentlymart
Copy link
Contributor

Hi again, all!

The new diff renderer changes are now merged in master ready to be included in the forthcoming v0.12.0 release, so I'm going to close this out.

You can get a preview of it by trying the alpha2 builds, though please do not use this against any "real" infrastructure yet since there are many known issues with other parts of Terraform still left to fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants