In the old protocol, returning a nil InstanceState was a way to indicate
that the object had been deleted. In the new world we signal that with
an actual object that contains a null value, which Terraform Core itself
will then recognize and turn into a nil state, eventually removing the
entry from state altogether.
If the plan called for us to delete but the result isn't null then that's
suspect, because it suggests the object wasn't deleted after all.
Likewise, no other apply action should cause the the result to be missing.
In order to avoid the confusing user experience that results in this case
(since it often looks like Terraform did nothing at all) we'll produce
some errors about it, but still update the state to reflect what the
provider returned anyway to allow for debugging and recovery.
Incorrect pointer discipline here was causing the error to be lost rather
than returned as expected.
Additionally we'll include a log line in this case because otherwise an
apply error is reported so far from the actual apply operation that it
can be difficult to understand what happened.
Previously we had a bug where we would fail to populate resource-level
metadata in the state during apply when count = 0, because the apply
graph would contain only instance nodes, not whole-resource nodes.
To address this, we add to the apply graph a node for each resource in
the configuration alongside the separate resource instance nodes. This
node's job is just to populate the state metadata for the resource, which
ensures it gets updated correctly even when count = 0.
When count is not zero this ends up doing some redundant work that
would've happened as a side-effect of applying individual resource
instances anyway, but it's harmless and makes the updating of our
resource-level metadata more explicit.
We're not yet showing outputs in the rendered diff, so it doesn't make
sense to count them for the purpose of deciding which change action
symbols to include in the legend.
Our state models cannot store unknown values (since state only deals with
knowns) and so following the lead of recent similar changes for resource
instances we'll treat the planned changeset as a sort of overlay on the
state, preferring values stored there if present, and then write in basic
planned output changes to the plan when we evaluate them.
We're abusing the plan model a little here: its current design is intended
to lay the groundwork for a future release where output values have a
full lifecycle similar to resource instances where we can properly track
changes during the plan phase, but the rest of Terraform isn't yet ready
for that and so we'll just retain an approximation of the planned action
by only using Create and Destroy actions.
A future release should change this so that output changes can be tracked
accurately using an approach similar to that of resource instances.
During the plan operation we need to retain _somewhere_ the planned
changes for all outputs so we can refer to them during expression
evaluation. For consistency with how we handle resource instance changes,
we'll keep them in the plan so we can properly retain unknown values,
which cannot be written to state.
As with output values in the state, only root output plans are retained
in a round-trip through the on-disk plan file format, but that's okay
because we can trivially re-calculate all of these during apply. We
include the _root_ outputs in the plan file only because they are
externally-visible side effects that ought to be included in any rendering
of the plan made from the plan file for user inspection.
We've intentionally changed the behavior for "count = 1" so that it'll
assign an index to the created instance even though there's only one. The
un-indexed behavior now applies only if count isn't set _at all_, thus
avoiding weird behavior if a count is _dynamically_ set to 1 via an
expression but is assumed to be a list elsewhere in configuration.
We previously tried to take a shortcut for an empty diff, just returning
the given value directly. This is incorrect in the weird case where we're
creating a new instance but it has no attributes (and thus an empty diff)
because in that case we'd return the given null value, turning the result
into a no-op or destroy change.
To fix this, we just always do the work to construct a new value, even
if we might end up doing all this just to reconstruct the same value we
started with in some cases.
This allows the provider to distinguish whether a particular value is set
in configuration or whether it's coming from prior state. It has no
particular purpose other than that.
We often want to bail out of a test if diagnostics are present, and it's
easiest to debug that when the diagnostics are printed in a compact but
complete manner that is non-trivial to produce.
Rather than duplicating that diagnostic formatting in every test, these
helpers allow us to succinctly print diagnostics and bail out when they
are present.
This is a pretty basic attempt to turn a pair of values into an old-school
diff. It probably won't work correctly for all tests, but hopefully works
well enough that we can just update the remaining tests in-place to use
the new API directly.
We now handle impure functions by having them return an unknown value
during plan, since we can't predict what the value will be during apply.
This test was assuming the old behavior.
The provider is allowed to return a partial result if it also includes
error diagnostics. Real providers still return at least a null value in
that case due to the RPC format, but test mocks are often more sloppy.
Since the refresh walk creates a partial plan to account for objects that
are yet to be created, we need to provide at least a basic mock of the
PlanProviderChange provider method.
For now we're using the old-style "DiffFn" shim interface since that's
already available for use in other tests.
It's not possible for a normal RPC-based provider to get into this
situation because a nil value can't go over the wire, but it's easy to
cause this by not correctly configuring a provider mock during tests.
By panicking early here we produce a more helpful error message and stack
trace than we'd otherwise produce if we let this nil value escape out
into the rest of Terraform.
Significant changes to the provider interface left a lot of the
tests in a non-buildable state. This set of changes gets the
tests building again but does not attempt to make them run to
completion or pass.
After this commit, it is possible to build a test program for
the ./terraform package but it will panic during its run. That
will be addressed in subsequent commits.
Since we do our deletes using a separate graph node from all of the other
actions, and a "Replace" change implies both a delete _and_ a create, we
need to pretend at apply time that a single replace change was actually
two separate changes.
This will also early-exit eval if a destroy node finds a non-Delete change
or if an apply node finds a Delete change. These should not happen in
practice because we leave these nodes out of the graph when they are not
needed for the given action, but we do this here for robustness so as not
to have an invisible dependency between the graph builder and the eval
phase.
When we're working on a create or destroy change it's expected for one of
the values to be null. Here we mimick the pre-0.12 behavior of producing
just an empty map in that case, which the helper/schema code (now the only
caller of this shim) then ignores completely.
For PreApply hook purposes we only actually use the Delete, Create, and
Update actions, because other actions are handled in different ways than
a direct call to ApplyResourceChange.
However, if there's a bug in core that causes it to pass a different
action, it's better for us to mark it as being explicitly unknown in the
UI rather than simply defaulting to "Modifying...", which can thus obscure
the problem and make for a confusing result.
The "id" field is assumed to always exist, and must have a valid value.
Set "id" to unknown when planning new resource changes to indicate that
it will be computed.
Martin Atkins