Some small improvements to diagnostic reporting #31299
Conversation
HCL's diagnostic model now includes the idea of "extra information" which works by attaching an initially-opaque interface value to each diagnostic and then asking callers to type-assert against that value to sniff for particular interfaces in order to discover additional machine-readable context about a certain diagnostic message. This commit echoes that idea into our tfdiags API, for now only for diagnostics that are backed by an hcl.Diagnostic. All other implementations of the diagnostic interface just always return nil, which means they never carry any "extra information". As is typical for our wrapping abstraction, we have here also a modified copy of HCL's helper function for conveniently probing a diagnostic for information of a particular type, designed to work with our diagnostic interface instead of HCL's concrete diagnostic type.
When an error occurs in a function call, the error message text often includes references to particular parameters in the function signature. This commit improves that reporting by also including a summary of the full function signature as part of the diagnostic context in that case, so a reader can see which parameter is which given that function arguments are always assigned positionally and so the parameter names do not appear in the caller's source code.
By observing the sorts of questions people ask in the community, and the ways they ask them, we've inferred that various different people have been confused by Terraform reporting that a value won't be known until apply or that a value is sensitive as part of an error message when that message doesn't actually relate to the known-ness and sensitivity of any value. Quite reasonably, someone who sees Terraform discussing an unfamiliar concept like unknown values can assume that it must be somehow relevant to the problem being discussed, and so in that sense Terraform's current error messages are giving "too much information": information that isn't actually helpful in understanding the problem being described, and in the worst case is a distraction from understanding the problem being described. With that in mind then, here we introduce an explicit annotation on diagnostic objects that are directly talking about unknown values or sensitive values, and then the diagnostic renderer will react to that to avoid using the terminology "known only after apply" or "sensitive" in the generated diagnostic annotations unless we're rendering a message that is explicitly related to one of those topics. This ends up being a bit of a cross-cutting concern because the code that generates these diagnostics and the code that renders them are in separate packages and are not directly aware of each other. With that in mind, the logic for actually deciding for a particular diagnostic whether it's flagged in one of these special ways lives inside the tfdiags package as an intermediation point, which both the diagnostic generator (in the core package) and the diagnostic renderer can both depend on.
There was a problem hiding this comment.
This is a really nice improvement!
It might be worth adding the json-output label for folks using that to catch changes to our JSON formats. We could possibly also bump the streaming JSON UI JSON_UI_VERSION and the terraform validate -json FormatVersion minor versions, although since this is purely additional that's probably not necessary.
|
Thanks for the reminder about the label! Thanks also for raising the question about format version numbers. I think I'm going to leave the version numbers unchanged here because I know we've had some grief changing those version numbers in the past (even just changing the minor versions) and although all of the consumers I'm directly aware of should now be updated to deal with it I'd rather not risk problems for other consumers without there being some reason for a consumer to vary its behavior by minor version here. (I expect a system consuming this would just use the presence of the new data as the trigger to act on it, rather than using the minor version as a proxy for that.) |
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR upgrades to a newer version of HCL which supports annotating diagnostics with "extra information", and then uses that new mechanism as the basis for two separate improvements to our diagnostic rendering.
Function signatures for function call errors
HCL itself is now annotating diagnostics relating to problems calling a function with extra information to allow a renderer to determine which function is being called.
We can now use that information to add additional context to error messages about incorrect function calls, so that it's easier to see which of the positional parameters a particular name mentioned in an error message belongs to:
In the above example I made the classic mistake of getting the two
joinarguments mixed up, which causes one diagnostic for each argument but both of them include the additional statement "while calling join(separator, lists...)" so a reader can see which of the parameters is "separator" and which is "lists".This particular change also adds a new property to the JSON representation of diagnostics in order to describe the function name and signature, so any external UIs built in terms of the JSON will need a corresponding change if they want to include the same information.
Don't mention unknown values and sensitive values when they aren't relevant
I've repeatedly noticed people asking questions in a way that betrays that they thought that Terraform saying that a value would be "known only during apply" was Terraform describing a problem they needed to fix, rather than just describing which values were in scope during evaluation. That has come up less often for sensitive values, but a similar principle applies in both cases: mentioning possibly-unfamiliar concepts that are not relevant to the error message can cause a reasonable reader to assume that they are part of the problem Terraform is describing, and it can therefore be a "red herring" leading the user off in an unproductive direction when trying to understand the problem.
To address that, I've added an ability to explicitly mark a diagnostic as being related to unknown values or related to sensitive values. The diagnostic render will then avoid using the words "known only after apply" and avoid mentioning sensitive values at all unless the diagnostic has the corresponding annotation indicating that the additional information is likely to be relevant and helpful.
I don't have a handy example to show of this one since it's a subtractive change rather than an additive one -- we'll now be showing slightly less information than we were before -- but essentially the idea is to prefer to not mention a value at all if there is no way to talk about it without mentioning that it is unknown or sensitive. As a compromise, there is a special case where if we have an unknown value of a known type we will still state the value's type:
In the above example,
boop.beepwascty.UnknownVal(cty.String)and so we can at least mention that it's a string, even though we'll no longer include the subsequent ", known only after apply" that we would've included before this.In this case the filtering is done at the JSON generation stage and so anyone consuming the JSON form of diagnostics will just see fewer elements in the array of values than before, and so any alternative UI based on the JSON output will automatically follow along and skip showing the distracting information.