make outputs error #16204
Merged
make outputs error #16204
Conversation
This file contains 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
jbardin
force-pushed
the
jbardin/outputs
branch
4 times, most recently
from
c2844b7
to
7307335
Compare
apparentlymart
approved these changes
Sep 29, 2017
terraform/features.go
Outdated
| func init() { | ||
| featureOutputErrors = os.Getenv("TF_OUTPUT_ERRORS") != "" | ||
| if featureOutputErrors { | ||
| fmt.Println("WARNING: output errors feature enabled") |
There was a problem hiding this comment.
I remember we've had problems before with early output interfering with... something. Perhaps it was the plugin RPC protocol? If things seem to be working okay then this is probably not a big deal since this is off by default anyway, but just wanted to flag it since I can't remember what exactly the problem was when we did something like this before.
There was a problem hiding this comment.
Yeah, I wanted this to be kind of loud, but I didn't think about it getting built into a provider.
This is internal only anyway, so it's probably better to drop it for now.
jbardin
force-pushed
the
jbardin/outputs
branch
from
7307335
to
2dcdddf
Compare
Ouptuts don't need to be re-evaluated during destroy, since everything is already in the state, so we can simply remove them.
Remove the Input flag threaded through the input graph creation process to prevent interpolation failures on module variables. Use an EvalOpFilter instead to inset the correct EvalNode during walkInput. Remove the EvalTryInterpolate type, and use the same ContinueOnErr flag as the output node for consistency and to try and keep the number possible eval node types down.
Module outputs may not have complete information during Input, because it happens before refresh. Continue process on output interpolation errors during the Input walk.
A Targeted graph may include outputs that were transitively included, but if they are missing any dependencies they will fail to interpolate later on. Prune any outputs in the TargetsTransformer that have missing dependencies, and are not depended on by any resource. This will maintain the existing behavior of outputs failing silently ni most cases, but allow errors to be surfaced where the output value is required.
When working on an existing plan, the context always used walkApply, even if the plan was for a full destroy. Mark in the plan if it was icreated for a destroy, and transfer that to the context when reading the plan.
DestroyValueReferenceTransformer is used during destroy to reverse the edges for output and local values. Because destruction is going to remove these from the state, nodes that depend on their value need to be visited first.
We're going to start merging breaking functgionality behind feature flags, to reduce the need for long-lived feature branches.
Add some tests for output errors and catch the errors behind the output errors feature flag.
This test was set to fail once this issue was fixed, and now it's fixed.
jbardin
force-pushed
the
jbardin/outputs
branch
from
2dcdddf
to
3ed7b1a
Compare
This was referenced Oct 27, 2017
|
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Allow outputs to generate errors.
Since this can break existing configuration relying on outputs failing silently, the output failure is blocked by the TF_OUTPUT_ERRORS feature flag.
A lot of module "bugs" can be traced back to an output that failed to generate a value. An interpolation error in an output should be handled the same as anywhere else, so that evaluation doesn't continue with possibly invalid data.
Start by making output interpolation errors fatal.
We bypass the known unavoidable errors in 3 ways:
During Input the errors are logged, the value is set to unknown, and the walk continues.
During Destroy the outputs themselves are destroyed, by ignoring their value and removing them from the state.
During a targeted walk, we prune outputs that can't resolve all their dependencies as long as nothing else targeted depends on the output.
The final case where output would fail was when applying a destroy plan, which would be treated as an apply. This is fixed by marking the plan for "Destroy", then transferring that to the context so that
walkDestroyis used rather thanwalkApply.When an output value is needed during a destroy walk, the destruction of the output can cause dependent resources to fail to destroy. This also applies to local values. This is fixed by creating the
DestroyValueReferenceTransformer, which walk the graph and reverses the edges for output and local values, so that they are not removed until after their dependents.