When executing an apply with no plan, it's possible for a cancellation
to arrive during the final batch of provider operations, resulting in no
errors in the plan. The run context was next checked during the
confirmation for apply, but in the case of -auto-approve that
confirmation is skipped, resulting in the canceled plan being applied.
Make sure we directly check for cancellation before confirming the plan.
This commit replaces `ioutil.TempDir` with `t.TempDir` in tests. The
directory created by `t.TempDir` is automatically removed when the test
and all its subtests complete.
Prior to this commit, temporary directory created using `ioutil.TempDir`
needs to be removed manually by calling `os.RemoveAll`, which is omitted
in some tests. The error handling boilerplate e.g.
defer func() {
if err := os.RemoveAll(dir); err != nil {
t.Fatal(err)
}
}
is also tedious, but `t.TempDir` handles this for us nicely.
Reference: https://pkg.go.dev/testing#T.TempDir
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
* fix: local variables should not be overridden by remote variables during `terraform import`
* chore: applied the same fix in the 'internal/cloud' package
* backport changes from cloud package to remote package
Co-authored-by: Alisdair McDiarmid <alisdair@users.noreply.github.com>
Co-authored-by: uturunku1 <luces.huayhuaca@gmail.com>
Previously we had three different layers all thinking they were
responsible for substituting a default value for an unset root module
variable:
- the local backend, via logic in backend.ParseVariableValues
- the context.Plan function (and other similar functions) trying to
preprocess the input variables using
terraform.mergeDefaultInputVariableValues .
- the newer prepareFinalInputVariableValue, which aims to centralize all
of the variable preparation logic so it can be common to both root and
child module variables.
The second of these was also trying to handle type constraint checking,
which is also the responsibility of the central function and not something
we need to handle so early.
Only the last of these consistently handles both root and child module
variables, and so is the one we ought to keep. The others are now
redundant and are causing prepareFinalInputVariableValue to get a slightly
corrupted view of the caller's chosen variable values.
To rectify that, here we remove the two redundant layers altogether and
have unset root variables pass through as cty.NilVal all the way to the
central prepareFinalInputVariableValue function, which will then handle
them in a suitable way which properly respects the "nullable" setting.
This commit includes some test changes in the terraform package to make
those tests no longer rely on the mergeDefaultInputVariableValues logic
we've removed, and to instead explicitly set cty.NilVal for all unset
variables to comply with our intended contract for PlanOpts.SetVariables,
and similar. (This is so that we can more easily catch bugs in callers
where they _don't_ correctly handle input variables; it allows us to
distinguish between the caller explicitly marking a variable as unset vs.
not describing it at all, where the latter is a bug in the caller.)
When showing a saved plan, we do not need to check the state lineage
against current state, because the plan cannot be applied. This is
relevant when plan and apply specify a `-state` argument to choose a
non-default state file. In this case, the stored prior state in the plan
will not match the default state file, so a lineage check will always
error.
As explained in the changes: The 'enhanced' backend terminology, which
only truly pertains to the 'remote' backend with a single API (Terraform
Cloud/Enterprise's), has been found to be a confusing vestige which need
only be explained in the context of the 'remote' backend.
These changes reorient the explanation(s) of backends to pertain more
directly to their primary purpose, which is storage of state snapshots
(and not implementing operations).
That Terraform operations are still _implemented_ by the literal
`Backend` and `Enhanced` interfaces is inconsequential a user of
Terraform, an internal detail.
For Terraform Cloud users using the 'remote' backend, the existing
'pattern' prompt should work just fine - but because their workspaces
are already present in TFC, the 'migration' here is really just
realigning their local workspaces with Terraform Cloud. Instead of
forcing users to do the mental gymnastics of what it means to migrate
from 'prefix' - and because their remote workspaces probably already exist and
already conform to Terraform Cloud's naming concerns - streamline the
process for them and calculate the necessary pattern to migrate as-is,
without any user intervention necessary.
1. ParseDeclaredValues: parses unparsed variables into terraform.InputValues
2. ProbeUndeclaredVariableValues: compares variable declarations with unparsed values to warn/error about undeclared variables
Implementing this test was quite a rabbithole, as in order to satisfy
backendTestBackendStates() the workspaces returned from
backend.Workspaces() must match exactly, and the shortcut taken to test
pagination in 3cc58813f0 created an
impossible circumstance that got plastered over with the fact that
prefix filtering is done clientside, not by the API as it should be.
Tagging does not rely on clientside filtering, and expects that the
request made to the TFC API returns exactly those workspaces with the
given tags.
These changes include a better way to test pagination, wherein we
actually create over a page worth of valid workspaces in the mock client
and implement a simplified pagination behavior to match how the TFC API
actually works.
These changes include additions to fulfill the interface for the client
mock, plus moving all that logic (which needn't be duplicated across
both the remote and cloud packages) over to the cloud package under a
dedicated mock client file.
The cloud package intends to implement a new integration for
Terraform Cloud/Enterprise. The purpose of this integration is to better
support TFC users; it will shed some overly generic UX and architecture,
behavior changes that are otherwise backwards incompatible in the remote
backend, and technical debt - all of which are vestiges from before
Terraform Cloud existed.
This initial commit is largely a porting of the existing 'remote'
backend, which will serve as an underlying implementation detail and not
be a typical user-level backend. This is because to re-implement the
literal backend interface is orthogonal to the purpose of this
integration, and can always be migrated away from later.
As this backend is considered an implementation detail, it will not be
registered as a declarable backend. Within these changes it is, for easy
of initial development and a clean diff.
We don't use this library anywhere else in Terraform, and this backend was
using it only for trivial helpers that are easy to express inline anyway.
The new direct code is also type-checkable, whereas these helper functions
seem to be written using reflection.
This gives us one fewer dependency to worry about and makes the test code
for this backend follow a similar assertions style as the rest of this
codebase.
Ensure that we still check for a stale plan even when it was created
with no previous state.
Create separate errors for incorrect lineage vs incorrect serial.
To prevent confusion when applying a first plan multiple times, only
report it as a stale plan rather than different lineage.
Previously we would reject attempts to delete a workspace if its state
contained any resources at all, even if none of the resources had any
resource instance objects associated with it.
Nowadays there isn't any situation where the normal Terraform workflow
will leave behind resource husks, and so this isn't as problematic as it
might've been in the v0.12 era, but nonetheless what we actually care
about for this check is whether there might be any remote objects that
this state is tracking, and for that it's more precise to look for
non-nil resource instance objects, rather than whole resources.
This also includes some adjustments to our error messaging to give more
information about the problem and to use terminology more consistent with
how we currently talk about this situation in our documentation and
elsewhere in the UI.
We were also using the old State.HasResources method as part of some of
our tests. I considered preserving it to avoid changing the behavior of
those tests, but the new check seemed close enough to the intent of those
tests that it wasn't worth maintaining this method that wouldn't be used
in any main code anymore. I've therefore updated those tests to use
the new HasResourceInstanceObjects method instead.
In historical versions of Terraform the responsibility to check this was
inside the terraform.NewContext function, along with various other
assorted concerns that made that function particularly complicated.
More recently, we reduced the responsibility of the "terraform" package
only to instantiating particular named plugins, assuming that its caller
is responsible for selecting appropriate versions of any providers that
_are_ external. However, until this commit we were just assuming that
"terraform init" had correctly selected appropriate plugins and recorded
them in the lock file, and so nothing was dealing with the problem of
ensuring that there haven't been any changes to the lock file or config
since the most recent "terraform init" which would cause us to need to
re-evaluate those decisions.
Part of the game here is to slightly extend the role of the dependency
locks object to also carry information about a subset of provider
addresses whose lock entries we're intentionally disregarding as part of
the various little edge-case features we have for overridding providers:
dev_overrides, "unmanaged providers", and the testing overrides in our
own unit tests. This is an in-memory-only annotation, never included in
the serialized plan files on disk.
I had originally intended to create a new package to encapsulate all of
this plugin-selection logic, including both the version constraint
checking here and also the handling of the provider factory functions, but
as an interim step I've just made version constraint consistency checks
the responsibility of the backend/local package, which means that we'll
always catch problems as part of preparing for local operations, while
not imposing these additional checks on commands that _don't_ run local
operations, such as "terraform apply" when in remote operations mode.
Previously the planfile.Create function had accumulated probably already
too many positional arguments, and I'm intending to add another one in
a subsequent commit and so this is preparation to make the callsites more
readable (subjectively) and make it clearer how we can extend this
function's arguments to include further components in a plan file.
There's no difference in observable functionality here. This is just
passing the same set of arguments in a slightly different way.
Historically the responsibility for making sure that all of the available
providers are of suitable versions and match the appropriate checksums has
been split rather inexplicably over multiple different layers, with some
of the checks happening as late as creating a terraform.Context.
We're gradually iterating towards making that all be handled in one place,
but in this step we're just cleaning up some old remnants from the
main "terraform" package, which is now no longer responsible for any
version or checksum verification and instead just assumes it's been
provided with suitable factory functions by its caller.
We do still have a pre-check here to make sure that we at least have a
factory function for each plugin the configuration seems to depend on,
because if we don't do that up front then it ends up getting caught
instead deep inside the Terraform runtime, often inside a concurrent
graph walk and thus it's not deterministic which codepath will happen to
catch it on a particular run.
As of this commit, this actually does leave some holes in our checks: the
command package is using the dependency lock file to make sure we have
exactly the provider packages we expect (exact versions and checksums),
which is the most crucial part, but we don't yet have any spot where
we make sure that the lock file is consistent with the current
configuration, and we are no longer preserving the provider checksums as
part of a saved plan.
Both of those will come in subsequent commits. While it's unusual to have
a series of commits that briefly subtracts functionality and then adds
back in equivalent functionality later, the lock file checking is the only
part that's crucial for security reasons, with everything else mainly just
being to give better feedback when folks seem to be using Terraform
incorrectly. The other bits are therefore mostly cosmetic and okay to be
absent briefly as we work towards a better design that is clearer about
where that responsibility belongs.
