backend/local: Handle interactive prompts for variables in UI layer #23040
Conversation
ghost
added
the
sdkv1
apparentlymart
added
bug
cli
v0.12
We previously deferred this to Validate, but all of our operations require a valid set of variables and so checking this up front makes it more obvious when a call into Terraform Core from the CLI layer isn't populating variables correctly, instead of having it fail downstream somewhere. It's the caller's responsibility to ensure that it has collected values for all of the variables in some way before calling NewContext, because in the main case driven by the CLI there are many different places that variable values can be collected from and so handling the main user-facing validation in the CLI allows us to return better error messages that take into account which way a variable is (or is not) being set.
During the 0.12 work we intended to move all of the variable value collection logic into the UI layer (command package and backend packages) and present them all together as a unified data structure to Terraform Core. However, we didn't quite succeed because the interactive prompts for unset required variables were still being handled _after_ calling into Terraform Core. Here we complete that earlier work by moving the interactive prompts for variables out into the UI layer too, thus allowing us to handle final validation of the variables all together in one place and do so in the UI layer where we have the most context still available about where all of these values are coming from. This allows us to fix a problem where previously disabling input with -input=false on the command line could cause Terraform Core to receive an incomplete set of variable values, and fail with a bad error message. As a consequence of this refactoring, the scope of terraform.Context.Input is now reduced to only gathering provider configuration arguments. Ideally that too would move into the UI layer somehow in a future commit, but that's a problem for another day.
Terraform Core expects all variables to be set, but for some ancillary commands it's fine for them to just be set to placeholders because the variable values themselves are not key to the command's functionality as long as the terraform.Context is still self-consistent. For such commands, rather than prompting for interactive input for required variables we'll just stub them out as unknowns to reflect that they are placeholders for values that a user would normally need to provide. This achieves a similar effect to how these commands behaved before, but without the tendency to produce a slightly invalid terraform.Context that would fail in strange ways when asked to run certain operations.
apparentlymart
force-pushed
the
b-root-variable-checks
branch
from
c70651d
to
9a0d669
Compare
| } else { | ||
| // If interactive input is enabled, we might gather some more variable | ||
| // values through interactive prompts. | ||
| // TODO: Need to route the operation context through into here, so that |
There was a problem hiding this comment.
Can you make a comment (here, not necessarily in the code) about why this TODO isn't being done now?
There was a problem hiding this comment.
I don't have any grand reason for it... just wanted to keep the function signature changes to a minimum here in order to keep the diff minimal, and the context wasn't routed in before either so this hasn't made things any worse than they used to be.
|
|
||
| // If we get here then we're planning to prompt for at least one additional | ||
| // variable's value. | ||
| sort.Strings(needed) // prompt in lexical order |
There was a problem hiding this comment.
This is a question, not a suggestion, despite the phrasing:
If needed wasn't sorted, what order would the variables show up in? If the variables are (at least per-fil) in the same order that they are in the configuration files it might make more sense to the user if the variables are presented in that order. I'm thinking of scenarios where one has a lot of related variables lumped together in each file and where the variables make more sense when thought of as a whole (as opposed to seeing them "split up" in lexical order).
There was a problem hiding this comment.
The variable definitions and values both live in maps so the default sort here would be pseudorandom.
In principle we could try to sort them by their source locations in order to recover the order in source code that you're thinking about here. So far the only precedent we've had for sorting things in lexical order is that we sort diagnostic messages that way in the hope of them appearing in an order that allows you to work through them one by one; I expect we could adapt similar logic here but we'd probably need to generalize some logic for actually sorting by the source locations so that it can be used from multiple locations.
I'm open to doing that, but I'd rather pursue it as a separate change, since my goal here was to retain the same user-facing behavior we had before and just move the logic to a different place in the code.
| func (v unparsedUnknownVariableValue) ParseVariableValue(mode configs.VariableParsingMode) (*terraform.InputValue, tfdiags.Diagnostics) { | ||
| return &terraform.InputValue{ | ||
| Value: cty.UnknownVal(v.WantType), | ||
| SourceType: terraform.ValueFromInput, |
There was a problem hiding this comment.
Another just-a-question: I'm not familiar with the usage of SourceType. Is there value in a SourceType that indicates that this value was explicitly left unset vs. the given terraform.ValueFromInput?
There was a problem hiding this comment.
In this particular case the meaning I was trying to represent here was "this isn't known yet, but it will eventually be determined by user input" (as long as the user changes nothing else in the mean time, of course).
In practice we don't currently really make strong use of this SourceType. It's used to lightly adjust some error message phrasing so that the messages can say things like "...which you set on the command line", or "...which you set in foo.tf", etc. We could add a new source type that would mean something like "unknown source" to go with the "unknown value". Perhaps we'd use that to suppress certain error messages altogether, like we do with unknown values in HCL. 🤔
| if !diags.HasErrors() { | ||
| // Error should be: The input variable "provider_var" has not been assigned a value. |
There was a problem hiding this comment.
Thank you for adding these, I'll be sure to start doing the same!
There was a problem hiding this comment.
Honestly, if I were writing these tests from scratch today I'd rather make the test code itself assert the error message, but I didn't want to mess too much with existing tests here and distract from the main goal...
|
After pondering @mildwonkey's questions/feedback for a little while, I'm going to go ahead and merge this in the interests of addressing the main issue at hand but will keep these other good ideas in mind for future work in this area, at which time we'll hopefully have time to consider the design implications more fully, as opposed to aiming for a minimal refactor to fix the bug. |
|
This impacted |
|
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. |
ghost
locked and limited conversation to collaborators
During the 0.12 work we intended to move all of the variable value collection logic into the UI layer (command package and backend packages) and present them all together as a unified data structure to Terraform Core. However, we didn't quite succeed because the interactive prompts for unset required variables were still being handled after calling into Terraform Core.
Here we complete that earlier work by moving the interactive prompts for variables out into the UI layer too, thus allowing us to handle final validation of the variables all together in one place and do so in the UI layer where we have the most context still available about where all of these values are coming from.
This allows us to fix a problem where previously disabling input with
-input=falseon the command line could cause Terraform Core to receive an incomplete set of variable values, and fail with a bad error message. (Fixes #21659)This refactoring also exposed a few cases where tests were not correctly constructing a realistic invocation of Terraform but had been getting away with it before due to the missing validation and a lucky coincidence. Therefore this PR includes some fixes for tests that were valid situations incorrectly framed, and some removals of tests that were testing situations that are impossible in normal use because the caller is responsible for preventing them from arising.
There are a few new tests here but since the overall functionality is still the same as it was before, and we've just pushed the concerns about a bit here, our existing context tests, command tests, and end-to-end tests were already covering the affected behaviors here and continue to do so.
I have also manually verified the correct behavior of various commands against a local test configuration with various combinations of required and optional variables.
As a consequence of this refactoring, the scope of
terraform.Context.Inputis now reduced to only gathering provider configuration arguments. Ideally that too would move into the UI layer somehow in a future commit, but that's a problem for another day.