This is a purely mechanical refactor PR: I de-exported a few more
functions which did not need to be exported in the first place, and
fixed a few outdated log outputs.
* rename files for consistency with contents
* terraform: refactor EvalValidateSelfref
The EvalValidateSelfref eval node implementation was removed in favor of a regular function.
* terraform: refactor EvalValidateProvisioner
EvalValidateProvisioner is now a method on NodeValidatableResource.
* terraform: refactor EvalValidateResource
EvalValidateResource is now a method on NodeValidatableResource, and the
functions called by (the new) validateResource are now standalone
functions.
This particular refactor gets the prize for "most complicated test
refactoring".
* terraform: refactor EvalMaybeTainted
EvalMaybeTainted was a relatively simple operation which never returned
an error, so I've refactored it into a plain function and moved it into
the only file its called from.
* terraform: eval-related cleanup
De-exported preApplyHook, which got missed in my general cleanup sweeps.
Removed resourceHasUserVisibleApply in favor of moving the logic inline
- it was a single-line check so calling the function was (nearly) as
much code as just checking if the resource was managed.
* terraform: refactor EvalApplyProvisioners
EvalApplyProvisioners.Eval is now a method on
NodeResourceAbstractInstance. There were two "apply"ish functions, so I
named the first "evalApplyProvisioners" since it mainly determined if
provisioners should be run before passing off execution to
applyProvisioners.
* terraform: refactor EvalApply
EvalApply is now a method on NodeAbstractResourceInstance. This was one
of the trickier Eval()s to refactor, and my goal was to change as little
as possible to avoid unintended side effects.
One notable change: there was a createNew boolean that was only used in
NodeApplyableResourceInstance.managedResourceExecute, and that boolean
was populated from the change (which was available from
managedResourceExecute), so I removed it from apply entirely. Out of an
abundance of caution I assigned the value to createNew in (roughtly) the same spot,
in case I was missing some place where the change might get modified.
TODO: Destroy nodes passed nil configs into apply, and I am curious if
we can get the same functionality by checking if the planned change is a
destroy, instead of passing a config into apply. That felt too risky for
this refactor but it is something I would like to explore at a future
point.
There are also a few updates to log output in this PR, since I spent
some time staring at logs and noticed various spots I missed.
* terraforn: refactor EvalRefresh
EvalRefresh.Eval(ctx) is now Refresh(evalRefreshReqest, ctx). While none
of the inner logic of the function has changed, it now returns a
states.ResourceInstanceObject instead of updating a pointer. This is a
human-centric change, meant to make the logic flow (in the calling
functions) easier to follow.
* terraform: refactor EvalReadDataPlan and Apply
This is a very minor refactor that removes the (currently) redundant
types EvalReadDataPlan and EvalReadDataApply in favor of using
EvalReadData with a Plan and Apply functions.
This is in effect an aesthetic change; since there is no longer an
Eval() abstraction we can rename functions to make their functionality
as obvious as possible.
* terraform: refactor EvalCheckPlannedChange
EvalCheckPlannedChange was only used by NodeApplyableResourceInstance
and has been refactored into a method on that type called
checkPlannedChange.
* terraform: refactor EvalDiff.Eval
EvalDiff.Eval is now a method on NodeResourceAbstracted called Plan
which takes as a parameter an EvalPlanRequest. Instead of updating
pointers it returns a new plan and state.
I removed as many redundant fields from the original EvalDiff struct as
possible.
* terraform: refactor EvalReduceDiff
EvalReduceDiff is now reducePlan, a regular function (without a method)
that returns a value.
* terraform: refactor EvalDiffDestroy
EvalDiffDestroy.Eval is now NodeAbstractResourceInstance.PlanDestroy
which takes ctx, state and optional DeposedKey and returns a change.
I've removed the state return value since it was only ever returning a
nil state.
* terraform: refactor EvalWriteDiff
EvalWriteDiff.Eval is now NodeAbstractResourceInstance.WriteChange.
* rename files to something more logical
* terrafrom: refresh refactor, continued!
I had originally made Refresh a stand-alone function since it was
(obnoxiously) called from a graphNodeImportStateSub, but after some
(greatly appreciated) prompting in the PR I instead made it a method on
the NodeAbstractResourceInstance, in keeping with the other refactored
eval nodes, and then built a NodeAbstractResourceInstance inside import.
Since I did that I could also remove my duplicated 'writeState' code
inside graphNodeImportStateSub and use n.writeResourceInstanceState, so
double thanks!
* unexport eval methods
* re-refactor Plan, it made more sense on NodeAbstractResourceInstance. Sorry
* Remove uninformative `Eval`s from EvalReadData, consolidate to a single
file, and rename file to match function names.
* manual rebase
* terraform: refactor EvalPreApply and EvalPostApply
EvalPreApply and EvalPostApply have been refactored as methods on
NodeAbstractResourceInstance.
* terraform: remove EvalReadState and EvalReadStateDeposed
These two functions had already been re-implemented as functions on
NodeAbstractResource, so this commit finished the process of removing
the Evals and refactoring the tests.
* terraform: remove EvalRefreshLifecycle
EvalRefreshLifecycle was only used in one node,
NodePlannableResourceInstance, so the functionality has been moved
directly inline.
* terraform: remove EvalDeposeState
EvalDeposeState was only used in one function, so it has been removed
and the logic placed in-line in
NodeApplyableResourceInstance.managedResourceExecute.
* terraform: remove EvalMaybeRestoreDeposedObject
EvalMaybeRestoreDeposedObject was only used in one place, so I've
removed it in favor of in-line code.
* Split node_resource_abstract.go into two files, putting
NodeAbstractResourceInstance methods in their own file - it was getting
large enough to be tricky for (my) human eyeballs.
* un-exported the functions that were created as part of the EvalTree()
refactor; they did not need to be public.
function
The original EvalReadState node is used only by `NodeAbstractResource`s,
so I've created a new method on NodeAbstractResource which does the same
thing as EvalReadState. When the EvalNode refactor project is complete,
EvalReadState will be removed entirely.
When we need to select a qualified provider address based on an implied
provider name, we have a special case that the name "terraform" maps to
terraform.io/builtin/terraform instead of
registry.terraform.io/hashicorp/terraform as would be the case for other
prefixes.
However, in order for that to work properly we need to use
addrs.ImpliedProviderForUnqualifiedType instead of
addrs.NewDefaultProvider, because the latter just unconditionally always
produces a "default" provider configuration (belonging to the "hashicorp"
namespace on the public registry).
Our reference transformer analyses and our destroy transformer analyses
are built around static (not-yet-expanded) addresses so that they can
correctly handle mixtures of expanded and not-yet-expanded objects in the
same graph.
However, this characteristic also makes them unnecessarily conservative
in their handling of references between resources within different
instances of the same module: we know they can never interact with each
other in practice because the dependencies for all instances of a module
are the same and so one instance cannot possibly depend on another.
As a compromise then, here we introduce a new helper function that can
recognize when a proposed edge is between two resource instances that
belong to different instances of the same module, and thus allow us to
skip actually creating those edges even though our imprecise analyses
believe them to be needed.
As well as significantly reducing the number of edges in situations where
multi-instance resources appear inside multi-instance modules, this also
fixes some potential cycles in situations where a single plan includes
both destroying an instance of a module and creating a new instance of the
same module: the dependencies between the objects in the instance being
destroyed and the objects in the instance being created can, if allowed
to connect, cause Terraform to believe that the create and the destroy
both depend on one another even though there is no need for that to be
true in practice.
This involves a very specialized helper function to encode the situation
where this exception applies. This function has an ugly name to reflect
how specialized it is; it's not intended to be of any use outside of these
three situations in particular.
The AbstractResourceInstance type was storing the entire Resource from
the state, when it only needs the actual instance state. This would
cause resources to consume memory on the order of n^2, where n in the
number of instances of the resource.
Rather than attaching the entire resource state, which includes copying
each individual instance, only attach the ResourceInstance state, and
extract out the provider address from the Resource.
Resources that are not yet created will not be in the graph during
refresh, and therefore cannot be attached to the data source nodes. In
this case we still need to indicate if there are depends_on entries
inherited from the module call, which we can do with the forceDependsOn
field.
GraphNodeAttachDependsOn give us a method for adding all transitive resource
dependencies found through depends_on references, so that data source
can determine if they can be read during plan. This will be done by
inspecting the changes of all dependency resources, and delaying read
until apply if any changes are planned.
This encapsulates the logic for selecting an implied FQN for an
unqualified type name, which could either come from a local name used in
a module without specifying an explicit source for it or from the prefix
of a resource type on a resource that doesn't explicitly set "provider".
This replaces the previous behavior of just directly calling
NewDefaultProvider everywhere so that we can use a different implication
for the local name "terraform", to refer to the built-in terraform
provider rather than the stale one that's on registry.terraform.io for
compatibility with other Terraform versions.
Use the new addrs type here.
Also remove the uniqueMap from the config transformer. We enforce
uniqueness during config loading, and this is more likely to have false
positives due to stringification than anything.
* terraform: large refactor to use Provider from configs.Resource
configs.Resource.ImpliedProvider() now returns a string; it is the
callers' responsibility to turn that into an addrs.Provider if needed.
GraphNodeProviderConsumer ProvidedBy() no longer returns nil (reverting
to earlier, pre-provider-fqn behavior): it will return either the
provider set in config, provider set in state, or the default provider.
Make the interface name reflect the new return type of the method.
Remove the confusingly named and unused ResourceAddress method from the
resource nodes as well.
Remove the requirement for most *Resource types to be a
GraphNodeModuleInstance, ensuring that they never call ctx.Path while
being evaluated. This gets rid of most of the direct need for the Path
method currently implemented by NodeResourceAbstract, leaving the
provider and schema related calls for a subsequent PR.
This adds more shimming into that node itself, but allows us to pull it
out of the config transformer, and ensure we can create the resources
correctly from the config. The shimmed address usage can then be raised
out of the abstract resource, into the expanded node types.
GraphNodeModulePath is similar to GraphNodeSubPath, except that it
returns an addrs.Module rather than an addrs.ModuleInstance. This is
used by the ReferenceTransformer to connect references, when modules may
not yet be expanded.
Because references only exist within the scope of a module, we can
connect everything knowing only the module path. If the reference is to
an expanded module instance output, we can still properly order the
reference because we'll wait for the entire module to complete
evaluation.
* add Config to AttachSchemaTransformer for providerFqn lookup
* terraform: refactor ProvidedBy() to return nil when provider is not set
in config or state
* terraform: refactor ProvidedBy() to return an addrs.ProviderConfig
interface
This refactor allows terraform to indicate whether a specific provider
configuration was found for the resource or if it is instead returning
the assumed default.
With that additional information the provider transformer can check if
there is a specific (non-default) provider FQN.
Implement a new provider_meta block in the terraform block of modules, allowing provider-keyed metadata to be communicated from HCL to provider binaries.
Bundled in this change for minimal protocol version bumping is the addition of markdown support for attribute descriptions and the ability to indicate when an attribute is deprecated, so this information can be shown in the schema dump.
Co-authored-by: Paul Tyng <paul@paultyng.net>
* WIP: dynamic expand
* WIP: add variable and local support
* WIP: outputs
* WIP: Add referencer
* String representation, fixing tests it impacts
* Fixes TestContext2Apply_outputOrphanModule
* Fix TestContext2Apply_plannedDestroyInterpolatedCount
* Update DestroyOutputTransformer and associated types to reflect PlannableOutputs
* Remove comment about locals
* Remove module count enablement
* Removes allowing count for modules, and reverts the test,
while adding a Skip()'d test that works when you re-enable
the config
* update TargetDownstream signature to match master
* remove unnecessary method
Co-authored-by: James Bardin <j.bardin@gmail.com>
a large refactor to addrs.AbsProviderConfig, embedding the addrs.Provider instead of a Type string. I've added and updated tests, added some Legacy functions to support older state formats and shims, and added a normalization step when reading v4 (current) state files (not the added tests under states/statefile/roundtrip which work with both current and legacy-style AbsProviderConfig strings).
The remaining 'fixme' and 'todo' comments are mostly going to be addressed in a subsequent PR and involve looking up a given local provider config's FQN. This is fine for now as we are only working with default assumption.
* Introduce "Local" terminology for non-absolute provider config addresses
In a future change AbsProviderConfig and LocalProviderConfig are going to
become two entirely distinct types, rather than Abs embedding Local as
written here. This naming change is in preparation for that subsequent
work, which will also include introducing a new "ProviderConfig" type
that is an interface that AbsProviderConfig and LocalProviderConfig both
implement.
This is intended to be largely just a naming change to get started, so
we can deal with all of the messy renaming. However, this did also require
a slight change in modeling where the Resource.DefaultProviderConfig
method has become Resource.DefaultProvider returning a Provider address
directly, because this method doesn't have enough information to construct
a true and accurate LocalProviderConfig -- it would need to refer to the
configuration to know what this module is calling the provider it has
selected.
In order to leave a trail to follow for subsequent work, all of the
changes here are intended to ensure that remaining work will become
obvious via compile-time errors when all of the following changes happen:
- The concept of "legacy" provider addresses is removed from the addrs
package, including removing addrs.NewLegacyProvider and
addrs.Provider.LegacyString.
- addrs.AbsProviderConfig stops having addrs.LocalProviderConfig embedded
in it and has an addrs.Provider and a string alias directly instead.
- The provider-schema-handling parts of Terraform core are updated to
work with addrs.Provider to identify providers, rather than legacy
strings.
In particular, there are still several codepaths here making legacy
provider address assumptions (in order to limit the scope of this change)
but I've made sure each one is doing something that relies on at least
one of the above changes not having been made yet.
* addrs: ProviderConfig interface
In a (very) few special situations in the main "terraform" package we need
to make runtime decisions about whether a provider config is absolute
or local.
We currently do that by exploiting the fact that AbsProviderConfig has
LocalProviderConfig nested inside of it and so in the local case we can
just ignore the wrapping AbsProviderConfig and use the embedded value.
In a future change we'll be moving away from that embedding and making
these two types distinct in order to represent that mapping between them
requires consulting a lookup table in the configuration, and so here we
introduce a new interface type ProviderConfig that can represent either
AbsProviderConfig or LocalProviderConfig decided dynamically at runtime.
This also includes the Config.ResolveAbsProviderAddr method that will
eventually be responsible for that local-to-absolute translation, so
that callers with access to the configuration can normalize to an
addrs.AbsProviderConfig given a non-nil addrs.ProviderConfig. That's
currently unused because existing callers are still relying on the
simplistic structural transform, but we'll switch them over in a later
commit.
* rename LocalType to LocalName
Co-authored-by: Kristin Laemmert <mildwonkey@users.noreply.github.com>
References from a resource-level connection blocks were not returned
from NodeAbstractResource.References, causing the provisioner connection
attributes to sometimes be evaluated too early.
The DestroyEdgeTransformer cannot determine ordering from the graph when
the destroyers are from orphaned resources, because there are no
references to resolve. The new stored Dependencies provides what we need
to connect the instances in this case.
We also add the StateDependencies method directly in the
GraphNodeResourceInstance interface, since all instances already
implement this, and we don't need another optional interface to check.
The old code in DestroyEdgeTransformer may no longer be needed in the
long run, but that can be determined separately, since too many of the
tests start with an incomplete state and rely on the Dependencies being
determined from the configuration alone.
Make use of the new Dependencies field in the instance state.
The inter-instance dependencies will be determined from the complete
reference graph, so that absolute addresses can be stored, rather than
just references within a module. The Dependencies are added to the node
in the same manner as state, i.e. via an "attacher" interface and
transformer. This is because dependencies are calculated from the graph
itself, and not from the config.
* core: don't panic in NodeAbstractResourceInstance References()
It is possible for s.Current to be nil. This was hard to reproduce, so
the root cause is still unknown, but we can guard against the symptom.
* add log statement
Previously we were fetching these from the provider but then immediately
discarding the version numbers because the schema API had nowhere to put
them.
To avoid a late-breaking change to the internal structure of
terraform.ProviderSchema (which is constructed directly all over the
tests) we're retaining the resource type schemas in a new map alongside
the existing one with the same keys, rather than just switching to
using the providers.Schema struct directly there.
The methods that return resource type schemas now return two arguments,
intentionally creating a little API friction here so each new caller can
be reminded to think about whether they need to do something with the
schema version, though it can be ignored by many callers.
Since this was a breaking change to the Schemas API anyway, this also
fixes another API wart where there was a separate method for fetching
managed vs. data resource types and thus every caller ended up having a
switch statement on "mode". Now we just accept mode as an argument and
do the switch statement within the single SchemaForResourceType method.
Since StateReferences was implemented on NodeAbstractResource rather than
NodeAbstractResourceInstance it wasn't properly detecting references to
the same instance as self-references.
Now that we are using "seen" to filter out duplicates we can also simplify
how we handle these self-references by just pretending we saw them before
we even start the loop.
This change is confirmed by
TestContext2Apply_provisionerMultiSelfRefSingle
As previously mentioned in a comment here, versions of Terraform prior to
0.12 would store references to module outputs as just references to the
module as a whole in the state. It's not really clear why, but we wanted
to preserve this behavior for 0.12.
The previous implementation actually failed to do so, in spite of the
comment, so this commit fixes it to actually do what the comment
originally claimed.
In a later release we might remove this special case and just depend
directly on outputs where possible, since that'd allow us to produce a
more precise dependency graph for destroy actions, but when we do that
we'll first need to confirm that there isn't a good reason for the
original exception here.
The underlying References function includes duplicates and returns refs
in the order they appeared in source (approximately), but after we reduce
to just the raw addresses it's better to dedupe and return in a
predictable order.
We no longer use strings to represent addresses, so this method was a
leftover outlier from previous refactoring efforts.
At this time the result is not actually being used due to the state type
refactoring, which is a bug we'll address in a subsequent commit.
Kristin Laemmert