My original intention was just to reduce our number of dependencies by
standardizing on a single comparison library, but in the process of doing
so I found various examples of the kinds of problems that caused this
codebase to begin adopting go-cmp instead of go-test/deep in the first
place, which make it easy to accidentally write a false-positive test that
doesn't actually check what the author thinks is being checked:
- deep.Equal silently ignores unexported fields, so comparing two values
that differ only in data in unexported fields succeeds even when it ought
not to.
TestContext2Apply_multiVarComprehensive in package tofu was an excellent
example of this problem: it had various test assertions that were
actually checking absolutely nothing, despite appearing to compare
pairs of cty.Value.
- deep.Equal also silently ignores anything below a certain level of
nesting, and so comparison of deep data structures can appear to succeed
even though they don't actually match.
There were a few examples where that problem had already been found and
fixed by temporarily overriding the package deep global settings, but
with go-cmp the default behavior already visits everything, or panics
if it cannot.
This does mean that in a few cases this needed some more elaborate options
to cmp.Diff to align with the previous behavior, which is a little annoying
but overall I think better to be explicit about what each test is relying
on. Perhaps we can rework these tests to need fewer unusual cmp options
in future, but for this commit I want to keep focused on the smallest
possible changes to remove our dependency on github.com/go-test/deep .
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Previously the Go toolchain had no explicit support for "tools" and so we
used the typical Go community workaround of adding "tools.go" files (two,
for some reason) that existed only to trick the Go toolchain into
considering the tools as dependencies we could track in go.mod.
Go 1.24 introduced explicit support for tracking tools as part of go.mod,
and the ability to run those using "go tool" instead of "go run", and so
this commit switches us over to using that strategy for everything we were
previously managing in tools.go.
There are some intentional exceptions here:
- The protobuf-compile script can't use "go tool" or "go run" because the
tools in question are run only indirectly through protoc. However, we
do still use the "tool" directive in go.mod to tell the Go toolchain that
we depend on those tools, so that it'll track which versions we are
currently using as part of go.mod.
- Our golangci-lint Makefile target uses "go run" to run a specific
version of golangci-lint. We _intentionally_ don't consider that tool
to be a direct dependency of OpenTofu because it has a lot of indirect
dependencies that would pollute our go.mod file. Therefore that continues
to use "go run" after this commit.
- Both of our tools.go files previously referred to
github.com/nishanths/exhaustive , but nothing actually appears to be
using that tool in the current OpenTofu tree, so it's no longer a
dependency after this commit.
All of the dependencies we have _only_ for tools are now classified as
"indirect" in the go.mod file. This is the default behavior of the Go
toolchain and appears to be motivated by making it clearer that these
modules do not contribute anything to the runtime behavior of OpenTofu.
This also corrected a historical oddity in our go.mod where for some reason
the "indirect" dependencies had been split across two different "require"
directives; they are now all grouped together in a single directive.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Due to some past confusion about the purpose of this package, it has grown
to include a confusing mix of currently-viable code and legacy support
code from the move to HCL 2. This has in turn caused confusion about which
parts of this package _should_ be used for new code.
To help clarify that distinction we'll move the legacy support code into
a package under the "legacy" directory, which is also where most of its
callers live.
There are unfortunately still some callers to these outside of the legacy
tree, but the vast majority are either old tests written before HCL 2
adoption or helper code used only by those tests. The one dubious exception
is the use in ResourceInstanceObjectSrc.Decode, which makes a best effort
to shim flatmap as a concession to the fact that not all state-loading
codepaths are able to run the provider state upgrade function that would
normally be responsible for the flatmap-to-JSON conversion, which is
explained in a new comment inline.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This extends statemgr.Persistent, statemgr.Locker and remote.Client to
all expect context.Context parameters, and then updates all of the existing
implementations of those interfaces to support them.
All of the calls to statemgr.Persistent and statemgr.Locker methods outside
of tests are consistently context.TODO() for now, because the caller
landscape of these interfaces has some complications:
1. statemgr.Locker is also used by the clistate package for its state
implementation that was derived from statemgr.Filesystem's predecessor,
even though what clistate manages is not actually "state" in the sense
of package statemgr. The callers of that are not yet ready to provide
real contexts.
In a future commit we'll either need to plumb context through to all of
the clistate callers, or continue the effort to separate statemgr from
clistate by introducing a clistate-specific "locker" API for it
to use instead.
2. We call statemgr.Persistent and statemgr.Locker methods in situations
where the active context might have already been cancelled, and so we'll
need to make sure to ignore cancellation when calling those.
This is mainly limited to PersistState and Unlock, since both need to
be able to complete after a cancellation, but there are various
codepaths that perform a Lock, Refresh, Persist, Unlock sequence and so
it isn't yet clear where is the best place to enforce the invariant that
Persist and Unlock must not be called with a cancelable context. We'll
deal with that more in subsequent commits.
Within the various state manager and remote client implementations the
contexts _are_ wired together as best as possible with how these subsystems
are already laid out, and so once we deal with the problems above and make
callers provide suitable contexts they should be able to reach all of the
leaf API clients that might want to generate OpenTelemetry traces.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This just adds a few previously-missing assertions so that future changes
to these interfaces will correctly identify all of the implementations that
need to be updated.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
Previously we were using a third-party library, but that doesn't have any
support for passing context.Context through its API and so isn't suitable
for our goals of adding OpenTelemetry tracing for all outgoing network
requests.
We now have our own fork that is updated to use context.Context. It also
has a slightly reduced scope no longer including various details that
are tightly-coupled to our cliconfig mechanism and so better placed in the
main OpenTofu codebase so we can evolve it in future without making
lockstep library releases.
The "registry-address" library also uses svchost and uses some of its types
in its public API, so this also incorporates v2 of that library that is
updated to use our own svchost module.
Unfortunately this commit is a mix of mechanical updates to the new
libraries and some new code dealing with the functionality that is removed
in our fork of svchost. The new code is primarily in the "svcauthconfig"
package, which is similar in purpose "ociauthconfig" but for OpenTofu's
own auth mechanism instead of the OCI Distribution protocol's auth
mechanism.
This includes some additional plumbing of context.Context where it was
possible to do so without broad changes to files that would not otherwise
have been included in this commit, but there are a few leftover spots that
are context.TODO() which we'll address separately in later commits.
This removes the temporary workaround from d079da6e9e, since we are now
able to plumb the OpenTelemetry span tree all the way to the service
discovery requests.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
The functions in these files are for handling older state snapshot formats
that current OpenTofu versions never generate, and so it's highly unlikely
that we'll ever make substantial changes to these functions.
Therefore it's unjustified to risk reworking it to pass linting rules, and
so we'll add nolint comments instead. Our priority is to make as few
changes as possible to these functions, to minimize the risk of regressing
a upgrade paths that are exercised very infrequently.
(For context, state version 4 has been current ever since Terraform
v0.12.0, and so the earlier versions are long obsolete.)
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
