terraform: remove state from validate graph walk#26063
Merged
mildwonkey merged 3 commits intomasterfrom Aug 31, 2020
Merged
Conversation
A recent change to the backend had a side effect of causing all input vars to be requested twice (due to two calls to `contextDirect`) and provider input values to be omitted entirely (those were populated during Validate, but Validate was called on the "extra" context and not the returned context). This PR adds a Context.StatelessCopy function which returns a nearly-identical copy of a terraform.Context with the state "zeroed out", which can then be Validate()d. A better, future refactor should remove the need for this function and instead modify Validate() itself to ignore state. Fixes #26027 I've added tests that confirm both behaviors; I am not sure what test covers the behavior that lead to this change in the first place but I did verify locally using the reproduction in the original PR.
Codecov Report
|
alisdair
approved these changes
Aug 31, 2020
Comment on lines
+568
to
+570
| // This will (helpfully) panic if more than one variable is requested during plan: | ||
| // https://github.com/hashicorp/terraform/issues/26027 | ||
| close := testInteractiveInput(t, []string{"bar"}) |
jbardin
approved these changes
Aug 31, 2020
Member
jbardin
left a comment
jbardin
left a comment
There was a problem hiding this comment.
LGTM! Thanks for finding a better spot to remove the state from Validate.
jbardin
reviewed
Aug 31, 2020
terraform/context_apply_test.go
Outdated
| // from the targeted resource | ||
| if len(mod.OutputValues) != 0 { | ||
| t.Fatalf("expected 0 outputs, got: %#v", mod.OutputValues) | ||
| // the root output should not get removed |
Member
There was a problem hiding this comment.
Maybe change this comment to indicate that we're verifying the current status-quo, but it's not necessarily incorrect to remove the output if we can manage it in the future.
|
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 as resolved and limited conversation to collaborators
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.
This pull reverts a recent change to
backend/localwhich created two context, one with and one without state. Instead I have removed the state entirely from the validate graph (by explicitly passing astates.NewState()).This changed caused a test failure, which (ty so much for the help) @jbardin discovered was inaccurate all along: the test's call to
Validate()was actually what was removing the output from state, which should not have been happening duringValidateanyway. The new expected test output matches terraform's actual behavior on the command line: if you use -target to destroy a resource, an output that references only that resource is not removed from state.This includes two tests to cover the expected behavior:
TestPlan_varsUnsethas been updated so it will panic if it gets more than one request to input a variableTestPlan_providerArgumentUnsetcovers terraform not properly storing provider args from command line when there is no provider block #26035Fixes #26035, #26027