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.
To avoid a massively-disruptive change to how EvalNode works, we're now
"smuggling" warnings through the error return value for these, but this
depends on all of the Eval machinery correctly handling this special case
and continuing evaluation when only warnings are returned.
Previous changes missed EvalSequence as a place where execution halts on
error. Now it will accumulate diagnostics itself, aborting if any of
them are error diagnostics, and then wrap its own result up in an error
to be returned by the main Eval function, which already treats non-fatal
errors as a special case, though now produces an explicit log message
about that situation to make it easier to spot in trace logs.
This also includes a more detailed warning message for the warning about
provider input being disabled. While this warning should be removed before
we release anyway, having this additional detail is helpful in debugging
tests where it's being returned.
Since outputs now rely on providers in order to ensure that a schema is
available for evaluation, we need to exclude providers from checking
TargetDownstream.
A provider's schema is the same regardless of its address in the
config. Key them by type so that an evaluation referencing a provider
from an address not included in the graph can still find the schema.
We no longer have this merge behavior, because it is inconsistent with how
variables behave in all other contexts and similar behavior can now be
achieved by merging the user's input with a predefined map in a local
value expression.
Previously we'd create the stub provider in any case where we didn't need
a configured provider, but we also need to skip creating it if there's
already a provider node present, or else we can end up with multiple
stub nodes in the graph.
Since ProviderTransformer now needs the schema in order to infer indirect
references to providers, we must run AttachSchemaTransformer before the
provider transformers in order to calculate the correct ordering of
operations.
The provider schema cache is keyed by provider configuration address
rather than provider type, so we need to do the same inheritance logic
to resolve providers needed because of reference as we do for providers
needed for direct use.
This allows resources that override "provider" or resources in child
modules that have their own provider configurations to be associated
with the provider config they will eventually get schema from, rather
than (as before) always the default configuration for the provider in
the root module.
Eventually it'd probably be better to switch to using a provider cache
that is keyed by provider _type_ rather than provider config, but since
it's currently fetched by visiting the individual provider graph nodes
we currently visit each provider configuration separately and fetch a
schema for each.
Any non-resource (outputs, variables, locals) that references a resource
type must also be connected to that resources provider. This is required
during apply, because the graph built from the diff may not include the
referenced resources because they are being evaluated from the state.
If the provider isn't present already, add a NodeEvalableProvider to
fetch the provider schema.
The provider transformers now need to happen after the outputs, locals,
and variables are transformed.
This is important in particular for shimming the "providers" map in module
blocks:
providers = {
"aws" = "aws.foo"
}
We call this shim for both the key and the value here, and the value would
previously have worked. However, the key is wrapped up by the parser in
an ObjectConsKeyExpr container, which deals with the fact that in normal
use an object constructor key that is just a bare identifier is actually
interpreted as a string. We don't care about that interpretation for our
shimming purposes, and so we can just unwrap it here.
This test seems to have been buggy before our current work, with the test
fixture containing a reference to a resource that doesn't exist.
This both fixes the fixture and adds a mock schema for it, though this
just revealed another error which isn't fixed here, where the a_ids value
seems to come through as unknown after apply. That will be fixed in a
subsequent commit.
Martin Atkins