command: Add experimental concise diff renderer

[alisdair]

When rendering a diff between current state and projected state, we only show resources and outputs which have changes. However, we show a full structural diff for these values, which includes all attributes and blocks for a changed resource or output. The result can be a very long diff, which makes it difficult to verify what the changed fields are.

This commit adds an experimental concise diff renderer, which suppresses most unchanged fields, only displaying the most relevant changes and some identifying context. This means:

• Always show all identifying attributes, initially defined as id, name, and tags, even if unchanged;
• Only show changed, added, or removed primitive values: string, number, or bool;
• Only show added or removed elements in unordered collections and structural types: map, set, and object;
• Show added or removed elements with any surrounding unchanged elements for sequence types: list and tuple;
• Only show added or removed nested blocks, or blocks with changed attributes.

If any attributes, collection elements, or blocks are hidden, a count is kept and displayed at the end of the parent scope. This ensures that it is clear that the diff is only displaying a subset of the resource.

The experiment is currently enabled by default, but can be disabled by setting the TF_X_CONCISE_DIFF environment variable to 0. This will be documented in the CHANGELOG update.

Screenshots

[codecov]

Codecov Report

Merging #26187 into master will increase coverage by 0.08%.
The diff coverage is 92.71%.

Impacted Files	Coverage Δ
command/format/diff.go	90.68% <92.56%> (+1.23%)	⬆️
command/format/state.go	48.87% <100.00%> (ø)
helper/experiment/experiment.go	100.00% <100.00%> (ø)
terraform/evaluate.go	52.33% <0.00%> (+0.42%)	⬆️

[alisdair]

📝

[alisdair]

This test change is to verify that multi-line strings are not affected by the concise renderer, which is intentional.

[alisdair]

This new test case added for completeness, although it is covering basically the same code as primitiveList.

[mildwonkey]

I'm curious why you're leaving in tags here as an unsuppressed attribute - in my experience, configurations using tags use a lot of tags, and those tags tend to be duplicated all over the place. I'm more than happy for this to remain, it's not an objection - better to hear what users think - but I am curious!

[alisdair]

This (like most of the good parts of this PR) came from my understanding of an idea proposed by @apparentlymart. My very limited experience suggested that tags are often used to identify grouped resources, and so it made sense to me to include those in the pass-list of attributes used to identify which resources are changing.

I hadn't considered that tags maps/lists might easily be a large part of the diff. Do you think it's worth removing tags from this initial implementation, and adding them back if user feedback suggests it? I think it'll be easier to add to this list than remove from it.

[mildwonkey]

I don't know ! I only have my own experience to fall back on here. But if you think it's going to be easier to add then remove, I would suggest suppressing tags for the first (dev) release or so and seeing what kind of feedback you get.

At my last job, we had about 10 required tags (because ENTERPRISE!! 💰 💸 💵 ) on every resource on top of whatever tags the developer added, and so displaying the tags would make the diff not concise at all.

[alisdair]

And that sounds like the opposite of what we're going for. Removed in 69840e9.

[apparentlymart]

This is definitely a tradeoff with no easy answer: in many cases in AWS the Name tag is the only useful (by which I mean: human-meaningful) identifier for an object, and so I'd considered it important to show at least that tag, but because we're currently using an approximation to allow us to ship this ahead of possible future provider protocol changes we can't really put an AWS-specific assumption like special-casing a tag called Name in here.

On the other hand, as you note @mildwonkey there are likely to be a number of other values in there that are not useful identifiers for this specific object, and so the resulting output will be noisier than it would ideally be.

My sense here is that showing the tags in full while hiding everything else is still considerably more concise than the current behavior for commonly-used AWS provider resource types, and so I feel it's an acceptable compromise until we're ready to design and implement a provider protocol extension to allow the providers themselves to indicate what they consider to be "identifying attributes".

The alternative of hiding tags would make it more likely that a particular object in the diff would have no visible human-readable identifiers at all, which was the problem that the non-concise diff change was originally attempting to solve. For that reason, I'd rather err on the side of being not quite as concise in order to decrease the likelihood of producing a diff that can't be understood without referring to an external system (because in that case, it's very tempting to just trust that the objects shown in the diff are the correct objects, without actually referring to the external system to confirm that).

[alisdair]

I've dropped the prior commit, so we're back to always rendering tags.

[mildwonkey]

The new plan diffs look great! I think users are really going to like this.

I hadn't expected that it would be enabled by default for our first release and I'm not sure how I feel about that - if we get negative feedback during the general release, we'll be in an awkward position where we'd need to roll back our changes or ask users to set the env variable to get the old behavior. I feel like this is a good candidate as an opt-in change which we can then make the default in the next major release.

I've not been involved in the internal planning so if this is something that's already been discussed and agreed upon just let me know and I'll stand down 😄

[alisdair]

@pkolyvas and I talked about the opt-in approach today, but there hasn't been a whole-team decision on this yet, so it's still very much up for discussion!

The thinking behind starting with enabled-by-default is to make it easier to get community feedback from an early alpha release. It's also optimistically hoping that the pre-release reception to the change will be both significant in the number of responses and overwhelmingly positive, in which case we can leave the experiment as opt-in for 0.14.0.

If the feedback is mixed or simply not present, we'd flip the flag default to "off" before we start the beta process. But it's just as easy to invert that process if you and/or others think that's the better approach.

[mildwonkey]

Ok, I see what you (two) are going for. What's sufficient feedback? Do we have an objective measure, so we know when to make the call if we've had "enough" to release 0.14 with the new diff renderer?

I'm not trying to be difficult, I'm in favor of the new behavior - it's also a very big change, and the point (in my opinion, which is definitely not the "law") of experiments was giving willing users a chance to opt-in, instead of forcing users who aren't ready for the change to opt-out.

Another thought: is it possible to do dev releases from a branch where we've set it to enabled by default while leaving master's behavior opt-in?

[alisdair]

What's sufficient feedback? Do we have an objective measure, so we know when to make the call if we've had "enough" to release 0.14 with the new diff renderer?

My answers are "not sure", and "nope", respectively. That woolliness convinces me that we don't have a clear enough plan to leave this experiment on in the main branch, so I've pushed a commit to disable it. It'll be easy to explain how to enable the feature when we announce the alpha release. Thanks for helping me think this through!

[mildwonkey]

Thanks @alisdair ! We can definitely revisit these choices going forward, I'm not just here to block you.

I don't mean to be the grumpy old timer and that's definitely what I felt like when I started asking questions 😁

[apparentlymart]

I don't intend this to necessarily change what you both decided above, but I have some extra historical context that might be useful in weighing this:

In Terraform 0.11 the behavior was to only show what had changed. We occasionally saw feedback that showing only what changed had been implicated as a cause of an outage because the plan didn't include enough information to make an informed decision about whether to accept the plan, and the plan had meant something different than how the reader had interpreted it. My recollection is that number of reports of this was pretty small, but we had considered it significant anyway because the result had been an outage.

For Terraform 0.12 we responded to that by showing all of the attributes, in an attempt to give the reader all of the available information in order to make an informed decision. This behavior was okay (though certainly not ideal) for the most common resources types we'd tested with, and so we elected to move forward with it because the alternative of contributing to outages seemed worse.

However, we've heard much more feedback about the new verbose behavior being too verbose to actually read, particularly for some very extreme resource types like aws_cloudfront_distribution. In the end the extreme verbosity risks creating the same problem: if you can't quickly skim the output to make your decision then you can reasonably develop a habit of not reading it at all and just accepting it.

With all of that said then, my sense of things was that the number of people who raised being harmed by omitting things from the diff was at least an order of magnitude smaller than the number of people who have raised being harmed by the new verbose output, and so it seems reasonable to conclude that the number of people who would be bothered by this change is much smaller than the number of people who would find it to be a better compromise. That could be an incorrect assumption, but I'm also worried about potentially being too cautious in rolling out a change that is, compared to some other changes we've been making recently, relatively easy to continue to iterate on or, in a very extreme case, to reverse altogether.

[alisdair]

Thanks for that context, Martin! That's a compelling reason for starting with opt-out for this change, so I've reverted the previous changes. If the feedback we receive is mixed, and the feature isn't ready for 0.14.0, we can easily flip the flag back to opt-in before the beta period.

[nathanielks]

I saw this included in the 0.14 alpha release this morning and I am very glad to see it! As @apparentlymart mentioned, I would include myself in the group that found the verbose output to be more harm than good, making it harder to discern what was actually changing. Good on you, team!

[bbuivn]

Hi there,

I find this new option really great!
I had a question though, do you guys plan on porting this feature back to TF.012 and/or TF.14 or will it only be available in TF0.14 once released?

Thanks!

[pkolyvas]

Hi there,

I find this new option really great!
I had a question though, do you guys plan on porting this feature back to TF.012 and/or TF.14 or will it only be available in TF0.14 once released?

Thanks!

We are really excited by it as well. That said, we don't plan on back porting this feature to 0.12 or 0.13 unfortunately.

[scott-doyland-burrows]

Regarding displaying the tags - could there be an option on the plan/apply to set the verboseness of the changes displayed.
So the user could decide whether to display the tags or not.

We have to have 14 mandatory tags on our Azure resources. The 0.14.0 plan/apply looks much better without all the unchanged attributes showing up, especially when running in Azure DevOps pipelines where we have to switch off the colours or we just get a garbled display (so we do not get a not a nice orange tilde in the pipeline output to highlight the changes).

However, it is still a lot of tags being displayed.

Thanks.

[alisdair]

@scott-doyland-burrows Thanks, that's useful feedback! Would you mind either reposting this in the discuss forum feedback thread or opening an enhancement issues so we don't lose track of it?

[scott-doyland-burrows]

Sure, I have setup the following below, and in it have pasted a link back to the above post.

#26371

[alisdair]

Thank you!

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