Chaange ResourceProvider to providers.Interface starting from the
context, and fix all type errors.
This only replaced some of method calls directly applicable to the
providers themselves. The resource methods will follow.
Due to how often the state and plan types are referenced throughout
Terraform, there isn't a great way to switch them out gradually. As a
consequence, this huge commit gets us from the old world to a _compilable_
new world, but still has a large number of known test failures due to
key functionality being stubbed out.
The stubs here are for anything that interacts with providers, since we
now need to do the follow-up work to similarly replace the old
terraform.ResourceProvider interface with its replacement in the new
"providers" package. That work, along with work to fix the remaining
failing tests, will follow in subsequent commits.
The aim here was to replace all references to terraform.State and its
downstream types with states.State, terraform.Plan with plans.Plan,
state.State with statemgr.State, and switch to the new implementations of
the state and plan file formats. However, due to the number of times those
types are used, this also ended up affecting numerous other parts of core
such as terraform.Hook, the backend.Backend interface, and most of the CLI
commands.
Just as with 5861dbf3fc49b19587a31816eb06f511ab861bb4 before, I apologize
in advance to the person who inevitably just found this huge commit while
spelunking through the commit history.
Previously we had the evaluate methods accept directly an
addrs.InstanceKey and had our evaluator infer a suitable value for
count.index for it, but that prevents us from setting the index to be
unknown in the validation scenario where we may not be able to predict
the number of instances yet but we still want to be able to check that
the configuration block is type-safe for all possible count values.
To achieve this, we separate the concern of deciding on a value for
count.index from the concern of evaluating it, which then allows for
other implementations of this in future. For the purpose of this commit
there is no change in behavior, with the count.index value being populated
whenever the instance key is a number.
This commit does a little more groundwork for the future implementation
of the for_each feature (which'll support each.key and each.value) but
still doesn't yet implement it, leaving it just stubbed out for the
moment.
The earlier change 5f07201a made it so that the state is always rewritten
by EvalDiffDestroy, but that was too disruptive to other users of
EvalDiffDestroy.
Now we follow the lead of EvalDiff and have a separate pointer for the
_output_ state, which allows the caller to opt in to having its state
pointer updated to reflect the new (nil) state.
NodePlannableResourceInstanceOrphan is the only caller that currently opts
in to this, since that was the focus of 5f07201a. We may need to make a
similar change to other plannable resource destroy nodes, but we'll wait
to see if that needs to be done in a subsequent commit.
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.
On the initial pass here I reached a faulty conclusion about what from
the new world should shim into a NewInstanceInfo, based on a poor read
of existing code.
It actually _should've_ been based on an absolute instance after all,
as evidenced by the expected result of TestContext2Refresh_targetedCount.
Therefore the signature is changed here, and all of the callers (which,
in retrospect, were all holding a full instance address anyway!) are
updated to that new signature.
The initial rework of this function to support traversals didn't correctly
handle the "all" case, due to a logic error where the ignoreAll branch
could be visited only if ignoreChanges were non-empty, but yet the two
are mutually exclusive in practice.
Now we process ignoreAll separately from ignoreChanges, and invert the
two loops so that we will visit all attributes regardless of what is
in the ignoreChanges slice.
These are all things that ought to be present in normal use but can end up
being nil in incorrect tests. Test debugging is simpler if these things
return errors gracefully, rather than crashing.
Due to how deeply the configuration types go into Terraform Core, there
isn't a great way to switch out to HCL2 gradually. As a consequence, this
huge commit gets us from the old state to a _compilable_ new state, but
does not yet attempt to fix any tests and has a number of known missing
parts and bugs. We will continue to iterate on this in forthcoming
commits, heading back towards passing tests and making Terraform
fully-functional again.
The three main goals here are:
- Use the configuration models from the "configs" package instead of the
older models in the "config" package, which is now deprecated and
preserved only to help us write our migration tool.
- Do expression inspection and evaluation using the functionality of the
new "lang" package, instead of the Interpolator type and related
functionality in the main "terraform" package.
- Represent addresses of various objects using types in the addrs package,
rather than hand-constructed strings. This is not critical to support
the above, but was a big help during the implementation of these other
points since it made it much more explicit what kind of address is
expected in each context.
Since our new packages are built to accommodate some future planned
features that are not yet implemented (e.g. the "for_each" argument on
resources, "count"/"for_each" on modules), and since there's still a fair
amount of functionality still using old-style APIs, there is a moderate
amount of shimming here to connect new assumptions with old, hopefully in
a way that makes it easier to find and eliminate these shims later.
I apologize in advance to the person who inevitably just found this huge
commit while spelunking through the commit history.
Containers (maps, lists, sets) in an InstanceDiff need to be handled in
their entirety. Unchanged values cannot be filtered out from diffs, as
providers expect attribute containers to be complete.
If a value in ignore_changes maps to a single key in an attribute
container, and there are other changes present, that ignored value must
be included in the diff as well.
Update all references to the version values to use the new package.
The VersionString function was left in the terraform package
specifically for the aws provider, which is vendored. We can remove that
last call once the provider is updated.
Rather than overloading InstanceDiff with a "Stub" attribute that is
going to be largely meaningless, we are just going to skip
pre/post-diff hooks altogether. This is under the notion that we will
eventually not need to "stub" a diff for scale-out, stateless nodes on
refresh at all, so diff behaviour won't be necessary at that point, so
we should not assume that hooks will run at this stage anyway.
Also as part of this removed the CountHook test that is now failing
because CountHook is out of scope of the new behaviour.
Changed the language of this field to indicate that this diff is not a
"real" diff, in that it should not be acted on, versus a "quiet" mode,
which would indicate just simply to act silently.
This fixes a bug with the new refresh graph behaviour where a resource
was being counted twice in the UI on part of being scaled out:
* We are no longer transforming refresh nodes without state to
plannable resources (the transformer will be removed shortly)
* A Quiet flag has been added to EvalDiff and InstanceDiff - this
allows for the flagging of a diff that should not be treated as real
diff for purposes of planning
* When there is no state for a refresh node now, a new path is taken
that is similar to plan, but flags Quiet, and does nothing with the
diff afterwards.
Tests pending - light testing has confirmed this should fix the double
count issue, but we should have some tests to actually confirm the bug.
When transforming a diff from DestroyCreate to a simple Update,
ignore_changes can cause keys from flatmapped objects to be filtered
form the diff. We need to filter each flatmapped container as a whole to
ensure that unchanged keys aren't lost in the update.
This commit improves the error logging for "Diffs do not match" errors
by using the go-spew library to ensure that the structures are presented
fully and in a consistent order. This allows use of the command line
diff tool to analyse what is wrong.
This set of changes addresses two bug scenarios:
(1) When an ignored change canceled a resource replacement, any
downstream resources referencing computer attributes on that resource
would get "diffs didn't match" errors. This happened because the
`EvalDiff` implementation was calling `state.MergeDiff(diff)` on the
unfiltered diff. Generally this is what you want, so that downstream
references catch the "incoming" values. When there's a potential for the
diff to change, thought, this results in problems w/ references.
Here we solve this by doing away with the separate `EvalNode` for
`ignore_changes` processing and integrating it into `EvalDiff`. This
allows us to only call `MergeDiff` with the final, filtered diff.
(2) When a resource had an ignored change but was still being replaced
anyways, the diff was being improperly filtered. This would cause
problems during apply when not all attributes were available to perform
the replacement.
We solve that by deferring actual attribute removal until after we've
decided that we do not have to replace the resource.
This means it’s shown correctly in a plan and takes into account any
actions that are dependant on the tainted resource and, vice verse, any
actions that the tainted resource depends on.
So this changes the behaviour from saying this resource is tainted so
just forget about it and make sure it gets deleted in the background,
to saying I want that resource to be recreated (taking into account the
existing resource and it’s place in the graph).
Previously these details were relegated to the debug logs, which forces
the user to reproduce the error condition and then go digging for the
error message. Since we're asking users to report this error, let's give
them all the details they need right up front to make it a little easier
on them.
