core: Check rule error message expressions#30613
Merged
Conversation
apparentlymart
approved these changes
Mar 3, 2022
Contributor
apparentlymart
left a comment
apparentlymart
left a comment
There was a problem hiding this comment.
This looks great to me!
My inline feedback is all small things that needn't block merging. Most of them are subjective things, so feel free to ignore or take a different direction if you disagree with what I'm suggesting.
Error messages for preconditions, postconditions, and custom variable validations have until now been string literals. This commit changes this to treat the field as an HCL expression, which must evaluate to a string. Most commonly this will either be a string literal or a template expression. When the check rule condition is evaluated, we also evaluate the error message. This means that the error message should always evaluate to a string value, even if the condition passes. If it does not, this will result in an error diagnostic. If the condition fails, and the error message also fails to evaluate, we fall back to a default error message. This means that the check rule failure will still be reported, alongside diagnostics explaining why the custom error message failed to render. As part of this change, we also necessarily remove the heuristic about the error message format. This guidance can be readded in future as part of a configuration hint system.
During the validation walk, we attempt to proactively evaluate check rule condition and error message expressions. This will help catch some errors as early as possible. At present, resource values in the validation walk are of dynamic type. This means that any references to resources will cause validation to be delayed, rather than presenting useful errors. Validation may still catch other errors, and any future changes which cause better type propagation will result in better validation too.
Custom variable validations specified using JSON syntax would always parse error messages as string literals, even if they included template expressions. We need to be as backwards compatible with this behaviour as possible, which results in this complex fallback logic. More detail about this in the extensive code comments.
alisdair
force-pushed
the
alisdair/check-rule-error-message-expressions
branch
from
b848f79 to
f21d0e8
Compare
Contributor
|
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
Contributor
|
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Error messages for preconditions, postconditions, and custom variable validations have until now been string literals. This PR changes this to treat the field as an HCL expression, which must evaluate to a string. Most commonly this will either be a string literal or a template expression.
When the check rule condition is evaluated, we also evaluate the error message. This means that the error message should always evaluate to a string value, even if the condition passes. If it does not, this will result in an error diagnostic.
If the condition fails, and the error message also fails to evaluate, we fall back to a default error message. This means that the check rule failure will still be reported, alongside diagnostics explaining why the custom error message failed to render.
As part of this change, we also necessarily remove the heuristic about the error message format. This guidance can be re-added in future as part of a configuration hint system.
Fixes #24407. Fixes #24160.