It was incorrect to use a type switch to detect the optional schema
attachment interfaces, because they are not mutually-exclusive: resource
nodes implement both GraphNodeAttachResourceSchema and
GraphNodeAttachProvisionerSchema.
This fixes a number of test regressions around dependency analysis in
"provisioner" blocks.
In #14526 we fixed a sticky edge-case where a resource with count = 0 set
won't create its containing module state on apply, and thus when another
expression refers to it we need to deal with that absense.
The original bug fixed by #14526 was actually a nil dereference panic in
this case. Our new HCL2-oriented expression evaluation codepath was, on
the other hand, correctly checking for the nil, but was not taking the
correct action in response to it, leading to the result being an
unexpected unknown value.
Here we replicate the fix to #14526 by behaving as if there are just no
instances present in this case. We achieve this in a slightly different
way here by just creating an empty ModuleState, but the effect is the
same as #14526.
This fixes TestContext2Apply_multiVarMissingState.
While we're planning we must always update the state with the proposed new
data resulting from the plan. In this case, we must record that the
orphan instance doesn't exist at all in the proposed new state by storing
its state as nil.
This in turn allows references to the containing resource to evaluate
properly, using the new updated resource count. This fixes
TestContext2Apply_multiVarCountDec.
This also includes a number of changes to the test output of
TestContext2Apply_multiVarCountDec that make it easier to debug failures.
Both ProviderTransformer and ReferenceTransformer need schema information,
and so there's a chicken-and-egg problem here where previously the schemas
were not getting attached to provider nodes created during
ProviderTransformer.
As a stop-gap measure for now we'll just run AttachSchemaTransformer
twice, so we can catch any new nodes created during the provider
transforms.
Previously we fetched schemas during the AttachSchemaTransformer,
potentially multiple times as that was re-run for each graph built. Now
we fetch the schemas just once during context construction, passing that
result into each of the graph builders.
This only addresses the schema accesses during graph construction. We're
still separately loading schemas during the main walk for evaluation
purposes. This will be addressed in a later commit.
An aliased provider should not be automatically inherited, nor
implicitly instantiated in a module. This test should not have
previously passed.
Add a proxy provider block to the module and update the provider to
match the schema.
The state after EvalReadDataDiff is no longer nil during plan, which
means that we can't use that as a proxy for requiring the diff.
Rather than exiting early to save the EvalWriteState and EvalWriteDiff
evaluations, continue normally regardless to ensure we have the latest
diff and state after the plan. This also aligns the data data source
handling with that of the managed resource.
The Provider field in ResourceState is now required, whereas before it
could be omitted and have Terraform try to discover a fitting provider
configuration automatically.
The automatic behavior was a compatibility shim added in v0.11 to support
states from prior versions without an explicit migration, but for v0.12
we will have a migration to our new state format anyway and so we will
fix this up during that migration pass.
This comprehensive test was covering a few different behaviors that are
intentionally different for v0.12:
- Applying the splat operator to a list of resource instances that haven't
been created yet produces a list of unknown values rather than a single
unknown list as before. This is important because it allows that list
to be passed into length().
- Wrapping a splat expression in another round of brackets now produces
a list of lists, whereas before we had a special case (for compatibility
with prior to v0.10) that would flatten this away in the schema layer.
Previously we would just retain an empty InstanceState in this case, but
now that we must enumerate all of the available instances during
expression evaluation it's important that we be able to recognize
instances that have been deleted.
Because we currently rely on the ReferenceTransformer to introduce the
necessary edges between local/output values and resource destroy nodes, we
must include the destroy phase of any resource we depend on in the
references of these.
This works in conjunction with the changes in the prior commit to restore
correct handling of dependencies for local and output values during
destroy.
With the current design, several seemingly-separate parts of the code must
all coincidentally agree with one another for destroy edges to be created
properly, which makes this code very hard to maintain. In future we should
refactor this so that ReferenceTransformer doesn't create edges for
destroy nodes at all, and have _all_ destroy edges (including
create_before_destroy) be dealt with in the single DestroyEdgeTransformer,
where they can be maintained and unit tested together.
Prior to the introduction of our "addrs" package, we represented destroy
nodes as a special kind of address string ending in ".destroy" or
".destroy-cbd".
Using references to resolve these dependencies is a strange idea to begin
with, since these are not user-visible addresses, but rather than refactor
that now we instead have these weird pseudo-address types ResourcePhase
and ResourceInstancePhase that correspond go those weird address suffixes,
thus restoring the prior behavior.
In future we should rework this so that destroy node edges are not handled
as references at all, and instead handled as part of
DestroyEdgeTransformer where there's better context for implementing this
logic and it can be maintained and tested in a single place.
The old testApplyFn would overwrite ID with "foo" in all cases there
wasn't a diff, which made the test fixtures harder to reason about. If
there's an ID, keep it the same.
The initial destroyer map is constructed using DestroyAddr(), which
returns resource instance addresses, but we were then going on to _use_
that map with resource addresses, which means the keys can't match when
indexed instances are being destroyed.
Now we'll use resource instance addresses in all cases.
This also includes some additional logging that was helpful in debugging
this issue.
The adaptation of ModuleState.RemovedOutputs for the new config types
was incorrect because it took the absence of any output map as "nothing to
do", rather than "everything has been removed" as expected.
Now it treats a nil map like an empty map, detecting _all_ of the outputs
as having been removed if the output map is nil.
This is temporarily broken until we implement the new plan file format,
since terraform.Plan is no longer serializable with gob. Rather than have
an error that seems like it needs immediate fixing, we'll be explicit
about it in the error message and focus our efforts on other test failures
for now, and return to implement the new file format later.
An earlier commit today reworked this to handle non-fatal errors, which
are returned "smuggled" as a special type of error to avoid changing the
EvalNode interface.
Unfortunately, that change then broke the _other_ special thing we smuggle
through the error return path: early exit.
Now we'll handle them both. This is not perfect because the early-exit
path causes us to discard any warnings we've already collected, but it's
more important that we bail early than retain warnings.
We previously added a special case for dealing with references to
instances in the plan graph where there are only resource nodes. However,
this was too general a fix and so it upset the handling of graphs where
instances _are_ present.
Now we'll do that fallback behavior only if there is no instance node in
the graph already, so the exact matching behavior will be used in graphs
where the instances are present.
The provider transforms now depend on analyzing references in order to
properly create provider edges, and so we need to now insert all of the
nodes that can have references and attach schemas before we run
TransformProviders.
This was done for the main graph builders in a previous commit, but as
usual we missed this surprising hidden graph builder that lives inside
a graph transformer. 🙄
Due to the need for schema in order to resolve references in expressions,
we now create additional provider dependency edges when a node refers to
an attribute from a resource.
During import we constrain provider configuration to allow only references
to variables, but since provider configurations in child modules might
refer to variables from the parent, we still need to include the module
variables, outputs and locals in the graph here and attach the provider
schemas.
In future a better check would be that the provider configuration doesn't
refer to anything that is currently unknown, but we'll save that for
another day.
The previous wording of this message was a little awkward, and a little
confusing due to the mention of it being a non-existing "resource", when
elsewhere in our output we use that noun to refer to the configuration
construct rather than the remote object.
Here we rework it as a diagnostic message, and while here also include an
extra note about a common problem of using an id from a different region
than the provider is configured for, to help the user realize what is
wrong in that case.
The previous commit rewrote this incorrectly because the fatal message
made it seem like it was failing when an error occurs, but an error is
actually expected here.
Also includes a more detailed error message for this case, consistent with
our new diagnostics style.
ctx.Import now returns tfdiags.Diagnostics rather than "error", so these
tests need to now expect that API for proper behavior.
Several of these tests are still failing for other reasons. That will be
addressed in subsequent commits.
Martin Atkins