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

terraform: Stabilize the variable_validation_crossref experiment #34955

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

apparentlymart
Copy link
Member

@apparentlymart apparentlymart commented Apr 5, 2024

In #34947 we introduced a language experiment that would permit variable validation rules to refer to other objects declared in the same module as the variable.

Now that experiment is concluded and its behavior is available for all modules.

This closes #25609.


This final version deviates slightly from the experiment: we learned from the experimental implementation that we accidentally made the terraform validate command able to validate constant-valued input variables in child modules
despite the usual rule that input variables are unknown during validation, because the previous compromise bypassed the main expression evaluator and built its own evaluation context directly.

Even though that behavior was not intended, it's a useful behavior that is protected by our compatibility promises and so this commit includes a slightly hacky emulation of that behavior, in eval_variable.go, that fetches the variable value in the same way the old implementation would have and then modifies the HCL evaluation context to include that value, while preserving anything else that our standard evaluation context builder put in there.

That narrowly preserves the old behavior for expressions that compare the variable value directly to a constant, while treating all other references (which were previously totally invalid) in the standard way. This quirk was already covered by the existing test TestContext2Validate_variableCustomValidationsFail, which fails if the special workaround is removed.

Our real implementation of the lang.Data interface depends on just about
everything else in the modules runtime to implement its behavior, and so
it's not really appropriate to use in unit tests.

This new implementation is considerably simpler, just returning values
predefined in a set of fixed tables. This is appropriate for unit tests of
features that perform expression evaluation as a side-effect of their work
but that don't contribute their own values to the real evaluation data
implementation.
@apparentlymart
Copy link
Member Author

I've prepared this today in the hope that the experiment feedback is positive and we choose to move forward with stabilization, because it's easier to write a stabilization changeset with the experience of implementing the experiment still fresh in mind.

However, I'm going to leave this in draft until we've actually included the experiment in at least one alpha release and gotten some satisfying feedback about it. If feedback suggests changing approach then this I'll close this PR and replace it with another that implements another round of the experiment.

Previously we introduced a language experiment that would permit variable
validation rules to refer to other objects declared in the same module
as the variable.

Now that experiment is concluded and its behavior is available for all
modules.

This final version deviates slightly from the experiment: we learned from
the experimental implementation that we accidentally made the "validate"
command able to validate constant-valued input variables in child modules
despite the usual rule that input variables are unknown during validation,
because the previous compromise bypassed the main expression evaluator
and built its own evaluation context directly.

Even though that behavior was not intended, it's a useful behavior that is
protected by our compatibility promises and so this commit includes a
slightly hacky emulation of that behavior, in eval_variable.go, that
fetches the variable value in the same way the old implementation would
have and then modifies the hcl evaluation context to include that value,
while preserving anything else that our standard evaluation context
builder put in there. That narrowly preserves the old behavior for
expressions that compare the variable value directly to a constant, while
treating all other references (which were previously totally invalid) in
the standard way. This quirk was already covered by the existing test
TestContext2Validate_variableCustomValidationsFail, which fails if the
special workaround is removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow variable validation conditions to refer to other variables
1 participant