A significant goal of the design changes around checks in earlier commits
(with the introduction of package "checks") was to allow us to
differentiate between a configuration object that we didn't expand at all
due to an upstream error, which has _unknown_ check status, and a
configuration object that expanded to zero dynamic objects, which
therefore has a _passing_ check status.
However, our initial lowering of checks.State into states.CheckResults
stayed with the older model of just recording each leaf check in isolation,
without any tracking of the containers.
This commit therefore lightly reworks our representation of check results
in the state and plan with two main goals:
- The results are grouped by the static configuration object they came
from, and we capture an aggregate status for each of those so that
we can differentiate an unknown aggregate result from a passing
aggregate result which has zero dynamic associated objects.
- The granularity of results is whole checkable objects rather than
individual checks, because checkable objects have durable addresses
between runs, but individual checks for an object are more of a
syntactic convenience to make it easier for module authors to declare
many independent conditions that each have their own error messages.
Since v1.2 exposed some details of our checks model into the JSON plan
output there are some unanswered questions here about how we can shift to
reporting in the two-level heirarchy described above. For now I've
preserved structural compatibility but not semantic compatibility: any
parser that was written against that format should still function but will
now see fewer results. We'll revisit this in a later commit and consider
other structures and what to do about our compatibility constraint on the
v1.2 structure.
Otherwise though, this is an internal-only change which preserves all of
the existing main behaviors of conditions as before, and just gets us
ready to build user-facing features in terms of this new structure.
The "checks" package is an expansion what we previously called
plans.Conditions to accommodate a new requirement that we be able to track
which checks we're expecting to run even if we don't actually get around
to running them, which will be helpful when we start using checks as part
of our module testing story because test reporting tools appreciate there
being a relatively consistent set of test cases from one run to the next.
So far this should be essentially a no-op change from an external
functionality standpoint, aside from some minor adjustments to how we
report some of the error and warning cases from condition evaluation in
light of the fact that the "checks" package can now track errors as a
different outcome than a failure of a valid check.
As is often the case with anything which changes what we track
in the EvalContext and persist between plan and apply, Terraform Core is
pretty brittle and so this had knock-on effects elsewhere too. Again, the
goal is for these changes to not create any material externally-visible
difference, and just to accommodate the new assumption that there will
always be a "checks" object available for tracking during a graph walk.
The checks.Checks type aims to encapsulate keeping track of check results
during a run and then reporting on them afterwards even if the run was
aborted early for some reason.
The intended model here is that each new run starts with an entirely fresh
checks.Checks, with all of the statuses therefore initially unknown, and
gradually populates the check results as we walk the graph in Terraform
Core. This means that even if we don't complete the run due to an error
or due to targeting options we'll still report anything we didn't visit
yet as unknown.
This commit only includes the modeling of checks in the checks package.
For now this is just dead code, and we'll wire it in to Terraform Core in
subsequent commits.
We previously added methods like this for some of the other types in this
package, including Local in this same file, but apparently haven't needed
these two yet.
Our existing addrs.Checkable represents a particular (possibly-dynamic)
object that can have checks associated with it.
This new addrs.ConfigCheckable represents static configuration objects
that can potentially generate addrs.Checkable objects.
The idea here is to allow us to predict from the configuration a set of
potential checkable object containers and then dynamically associate
the dynamic checkable objects with them as we make progress with planning.
This is intended for our integration of checks into the "terraform test"
testing harness, to be used instead of the weirdo builtin provider we were
using as a placeholder before we had first-class syntax for checks.
Test reporting tools find it helpful for there to be a consistent set of
test cases from one run to the next so that they can report on trends over
multiple runs, and so our ConfigCheckable addresses will serve as the
relatively-static "test case" that we'll then associate the dynamic checks
with, so that we can still talk about objects in the test result report
even if we end up not reaching them due to an upstream conditution failure.
protoc-gen-go generates non-style-compliant import directives, but since
those files are just generated anyway we don't need to worry too much
about what style they are in: the style that protoc-gen-go generates is
the canonical style for these ones.
The importscheck script now uses "go run" to run this program and so
the Go toolchain will install it automatically using the version number
of golang.org/x/tools specified in our go.mod file.
All four of these scripts have #! lines calling for them to run in bash,
and they are all marked as executable, so there's no harm in running them
directly and it makes it clearer that we're running them in the shell
specified on the #! line, rather than with whatever "sh' happens to be
on the current system.
(Note that this _was_ still correctly using the #! lines before, because
it's `sh -c` rather than just `sh`, but nonetheless that old way seemed
needlessly confusing.)
When we run many of these together e.g. in the checks.yaml GitHub Actions
workflow, it's hard to tell from the output exactly which command is
producing which subset of the output.
To help clarify that, we'll ask make to print out the command line it's
running before it runs it.
At the risk of a little bit of hidden spooky action at a distance, this
will slightly change the behavior of the "goimports check" to compare
against the base branch of a PR rather than to origin/main if we happen
to find one of the environment variables that GitHub Actions sets
automatically in its runners. This is targeting our "checks.yml" workflow
in particular.
The intention here is to avoid misreporting files that haven't actually
changed when a PR is targeting a branch other than the main branch, such
as directly targeting a historical release branch.
We'll still run against origin/main when we're not running in GitHub
Actions, since that's _typically_ the correct branch to use for new
work, even if it will eventually get backported to a release branch.
The previous implementation of this check tried to accumulate all of the
changed files into a single big string and then run goimports once with
all of them, but that approach ran into problems for changesets of a
certain (platform-specific) size due to limits on maximum command line
length.
This new version instead uses bash arrays and runs goimports separately
for each of the files which appear to have changed relative to the
base branch. This is likely to be slower to complete for changesets that
have many different changed files, but it's better for it to be slow than
to return an error and fail to check some of the files.
This is a complement to "timestamp" and "timeadd" which allows
establishing the ordering of two different timestamps while taking into
account their timezone offsets, which isn't otherwise possible using the
existing primitives in the Terraform language.
This fixes a possible panic in what Terraform calls the "flatten" function
in situations where a user passes in a null value of a sequence type.
The function will now treat that the same as a null value of any other
type.
Because we maintain multiple versions of Terraform across different
release branches, we aim to avoid creating needless differences between
the branches to maximize the chance of successful automatic backporting.
Part of that policy is that we don't make cross-cutting changes to respond
to deprecation of functions in upstream packages and instead we respond
to them gradually over time when we'd be changing the nearby code anyway,
or when new work requires using the replacement APIs.
In recognition of that, this turns of the staticcheck rule that would
otherwise force us to resolve all deprecations before moving forward with
any other change.
Go 1.19's "fmt" has some awareness of the new doc comment formatting
conventions and adjusts the presentation of the source comments to make
it clearer how godoc would interpret them. Therefore this commit includes
various updates made by "go fmt" to acheve that.
In line with our usual convention that we make stylistic/grammar/spelling
tweaks typically only when we're "in the area" changing something else
anyway, I also took this opportunity to review most of the comments that
this updated to see if there were any other opportunities to improve them.
Prevously the cloud backend would only render post-plan run tasks. Now
that pre-plan tasks are in beta, this commit updates the plan phase to
render pre-plan run tasks. This commit also moves some common code to
the common backend as it will be used by other task stages in the
future.
