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>
From some more practical testing of this I realized that usually the first
thing I want to know after seeing this warning is what the object literal
was being assigned to and what else was also defined inside it, and so
this sets the diagnostic's "context" to include the whole containing
object literal so that the source snippet in the diagnostic message is more
immediately useful, without having to cross-reference to the source code
in a separate text editor.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This generalizes the previously-added lint-like check for when an object
constructor is used to define an input variable and it contains a
definition for an attribute that isn't part of the target type, so that
now it also works for various nested structures that commonly arise in
real-world configurations.
Because this is now considerably more complicated I factored it out into
a new package called "lint" which could potentially grow to include other
similar "technically valid but probably a mistake" situations in future,
but for now it just introduced an opportunity to produce similar warning
messages for ignored attribute definitions in the default value for an
input variable.
It seems to me that there is actually no useful reason to include an
unexpected attribute definition in either of these two cases: that
attribute will never appear as part of any expression that any other part
of the configuration can use. Therefore I considered making these be
treated as errors rather than warnings, but turning something that was
previously valid into an error is risky so I'm suggesting that we start
with these as warnings and then consider upgrading them to errors in a
later release if we don't hear of anyone reporting a false-positive that
was _somehow_ actually useful. (I find that very unlikely, but still...)
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
We intentionally allow assigning object types with a superset of the
attributes included in an input variable's object type constraints because
it makes it possible to assign a whole object for which only some of the
attributes are relevant for one input variable but a different subset might
be relevant when the object value is used in a different part of the
configuration.
However, when the variable is defined using an object literal expression
there is no possible way an unexpected attribute could be useful in a
different part of the configuration, and so that's very very likely to be
a mistake rather than intentional. Therefore we'll generate a "linter-like"
warning in that case to help the author notice their mistake without
introducing any new "strict-mode" language features, or other complexity
that would be harder to maintain and evolve over time.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This continues our ongoing effort to get a coherent chain of
context.Context all the way from "package main" to all of our calls to
external components.
Context.Apply does not yet do anything with its new context, but this gets
the context plumbed in enough that we should be able to pass values like
telemetry spans all the way from the top-level in future.
OpenTofu has some historical situational private uses of context.Context
to handle the graceful shutdown behaviors. Those use context.Context as
a private implementation detail rather than public API, and so this commit
leaves them as-is and adds a new "primary context" alongside. Hopefully
in future refactoring we can simplify this to use the primary context also
as the primary cancellation signal, but that's too risky a change to bundle
in with this otherwise-mostly-harmless context plumbing.
All of the _test.go file updates here are purely mechanical additions of
the extra argument. No test is materially modified by this change, which
is intentional to get some assurance that isn't a breaking change.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
This continues our ongoing effort to get a coherent chain of
context.Context all the way from "package main" to all of our calls to
external components.
Context.Plan does not yet do anything with its new context, but this gets
the context plumbed in enough that we should be able to pass values like
telemetry spans all the way from the top-level in future.
OpenTofu has some historical situational private uses of context.Context
to handle the graceful shutdown behaviors. Those use context.Context as
a private implementation detail rather than public API, and so this commit
leaves them as-is and adds a new "primary context" alongside. Hopefully
in future refactoring we can simplify this to use the primary context also
as the primary cancellation signal, but that's too risky a change to bundle
in with this otherwise-mostly-harmless context plumbing.
All of the _test.go file updates here are purely mechanical additions of
the extra argument. No test is materially modified by this change, which
is intentional to get some assurance that isn't a breaking change.
Signed-off-by: Martin Atkins <mart@degeneration.co.uk>
