backend/local: do not use backend operation variables

[svanharmelen]

Previously the local backend didn’t merge variables but completely replaced them when any backend operation variables were set.

Before this change this was essentially a dead codepath (that didn't have any tests) as the backend operation would never have a value for these variables and so the complete set of variables created while getting the ContextOpts would be used instead: https://github.com/hashicorp/terraform/blob/v0.11/command/meta.go#L312

But the variables were added to the backend operation because the remote backend needs access to these command line variables so they can be set as run variables. Run variables are not yet supported in this release, but this value is still used in the current remote backend to detect if command line variables were set, in which case a proper error can be returned.

We could also just remove the whole code path, instead of changing the behavior to merge instead of replace the variables. Will discuss that option before merging this.

I added a few tests to guard against this issue in any future releases.

Fixes #19163

EDIT: See the first commit for this initial solution.

[svanharmelen]

In the earlier stages of the command initialization, variables are separated by autoVariabels and variables. But in the backend.Operation struct there is only the Variables field to pass any variables. So it looks like this field was intended to have either all applicable variables, or none (in which case the ones from the context would be used). In that context, the code that was changes in this PR is actually correct and should not change.

But looking very closely through the code base, I noticed that the Variables field was never set (until the change referenced above) and read only once in the local backend. So I would suggest that I update this PR and just remove the updated lines of code from the local backend code instead of changing its behavior.

We can then assume that the field name matches the variable type that can be read from the Meta struct. In order to make that distinction more clear when reading the code we could add a AutoVariables field to the backend.Operation to reflect the different types of variables, but that field would never be used. At least not in the current code base.

So I suggest we, instead, add a very explicit comment to the Variables field to make clear it only expects the command line provided variables and not the auto loaded variables.

EDIT: Created a second commit that reflects the suggestions in this comment.

[mildwonkey]

LGTM but we should probably change the title of this PR to match the current behavior!

[ghost]

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.