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

command/format: Restructure diagnostic presentation #30331

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

apparentlymart
Copy link
Member

Based on how folks have asked their questions, it seems that our current ordering of summary/snippet/values/detail is causing those skimming the error messages to believe that the "values" portion is the error message, and not read the "detail" part.

We originally placed the snippet immediately after the summary so that it would allow quickly seeing the location of the error, but that goal can still be met by including only a reference to the filename and line number up top, and then keeping the full snippet for later in the message.

This does make the diagnostic messages slightly longer, by introducing a few new lines where one of them is slightly redundant, but it also makes the most relevant information -- the details of the diagnostic -- closer to the top and thus hopefully more likely to be read first by those who are skimming the messages, rather than reading them in detail.

╷
│ Error: Invalid for_each argument
│     on for-each-unknown.tf line 10
│     in resource "null_resource" "other2"
│ 
│ The "for_each" map includes keys derived from resource attributes that cannot
│ be determined until apply, and so Terraform cannot determine the full set of
│ keys that will identify the instances of this resource.
│ 
│ When working with unknown values in for_each, it's better to define the map
│ keys statically in your configuration and place apply-time results only in
│ the map values.
│ 
│ Alternatively, you could use the -target planning option to first apply only
│ the resources that the for_each value depends on, and then apply a second
│ time to fully converge.
│ 
│ for-each-unknown.tf, in resource "null_resource" "other2":
│   10:   for_each = {
│   11:     (null_resource.example.id) = "baz"
│   12:   }
│     ├────────────────
│     │ null_resource.example.id is a string, known only after apply
╵

This is just a draft for now, because if we decide to go forward with it we'll need to spend a non-trivial amount of effort updating all of the tests for this, and so we want to make sure it's a good direction first before spending that time.

Based on how folks have asked their questions, it seems that our current
ordering of summary/snippet/values/detail is causing those skimming the
error messages to believe that the "values" portion is the error message,
and not read the "detail" part.

We originally placed the snippet immediately after the summary so that it
would allow quickly seeing the location of the error, but that goal can
still be met by including only a reference to the filename and line number
up top, and then keeping the full snippet for later in the message.

This does make the diagnostic messages slightly longer, by introducing a
few new lines where one of them is slightly redundant, but it also makes
the most relevant information -- the details of the diagnostic -- closer
to the top and thus hopefully more likely to be read first by those who
are skimming the messages, rather than reading them in detail.
@apparentlymart apparentlymart self-assigned this Jan 10, 2022
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@apparentlymart
Copy link
Member Author

Over in #31299 we did a less severe change to just hide any mentions of unknown values or sensitive values unless the problem is actually related to the presence of those, and I'd hoped that would be a good compromise to address the concern that originally motivated this.

However, today I saw a question on Stack Overflow where someone read the context about what type a variable had as the error message and completely ignored the actual error message below it, which shows that this problem isn't limited only to unknown values and sensitive values.

This PR is a bit stuck because our feedback loop on this is so long and unpredictable that we don't have any good way to prove that reformatting the messages in this way would actually make the problem better: it's possible that it would just cause the same confusion for a different set of people who bias towards reading the information closest to the bottom of the output because they read upwards from the command prompt (which is the assumption that motivated how Terraform currently presents these.)

@apparentlymart
Copy link
Member Author

Just this week I've seen five separate people ask for help and share only the second half of the error message, so clearly our current presentation is not making it clear to folks where the error messages start and end. I still have no way to prove that what I tried here is an improvement, but the current presentation doesn't seem to be working for people.

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

Successfully merging this pull request may close these issues.

None yet

2 participants