For a long time now, the diff logic has relied on the behavior of
`mapstructure.WeakDecode` to determine how various primitives are
converted into strings. The `schema.DiffString` function is used for
all primitive field types: TypeBool, TypeInt, TypeFloat, and TypeString.
The `mapstructure` library's string representation of booleans is "0"
and "1", which differs from `strconv.FormatBool`'s "false" and "true"
(which is used in writing out boolean fields to the state).
Because of this difference, diffs have long had the potential for
cosmetically odd but semantically neutral output like:
"true" => "1"
"false" => "0"
So long as `mapstructure.Decode` or `strconv.ParseBool` are used to
interpret these strings, there's no functional problem.
We had our first clear functional problem with #6005 and friends, where
users noticed diffs like the above showing up unexpectedly and causing
troubles when `ignore_changes` was in play.
This particular bug occurs down in Terraform core's EvalIgnoreChanges.
There, the diff is modified to account for ignored attributes, and
special logic attempts to handle properly the situation where the
ignored attribute was going to trigger a resource replacement. That
logic relies on the string representations of the Old and New fields in
the diff to be the same so that it filters properly.
So therefore, we now get a bug when a diff includes `Old: "0", New:
"false"` since the strings do not match, and `ignore_changes` is not
properly handled.
Here, we introduce `TypeBool`-specific normalizing into `finalizeDiff`.
I spiked out a full `diffBool` function, but figuring out which pieces
of `diffString` to duplicate there got hairy. This seemed like a simpler
and more direct solution.
Fixes#6005 (and potentially others!)
* MaxItems defines a maximum amount of items that can exist within a
TypeSet or TypeList. Specific use cases would be if a TypeSet is being
used to wrap a complex structure, however more than one instance would
cause instability.
It was a mistake to switched fully to `==` when activating waiting for
capacity on updates in #3947. Users that didn't set `min_elb_capacity ==
desired_capacity` and instead treated it as an actual "minimum" would
see timeouts for every create, since their target numbers would never be
reached exactly.
Here, we fix that regression by restoring the minimum waiting behavior
during creates.
In order to preserve all the stated behavior, I had to split out
different criteria for create and update, criteria which are now
exhaustively unit tested.
The set of fields that affect capacity waiting behavior has become a bit
of a mess. Next major release I'd like to rework all of these into a
more consistently named block of config. For now, just getting the
behavior correct and documented.
(Also removes all the fixed names from the ASG tests as I was hitting
collision issues running them over here.)
Fixes#4792
Implementation notes:
* The hash implementation was not considering key value, causing "diffs
did not match" errors when a value was updated. Switching to default
HashResource implementation fixes this
* Using HashResource as a default exposed a bug in helper/schema that
needed to be fixed so the Set function is picked up properly during
Read
* Stop writing back values into the `key` attribute; it triggers extra
diffs when `default` is used. Computed values all just go into `var`.
* Includes a state migration to prevent unnecessary diffs based on
"key" attribute hashcodes changing.
In the tests:
* Switch from leaning on the public demo Consul instance to requiring a
CONSUL_HTTP_ADDR variable be set pointing to a `consul agent -dev`
instance to be used only for testing.
* Add a test that exposes the updating issues and covers the fixes
Fixes#774Fixes#1866Fixes#3023
Changing the Set internals makes a lot of sense as it saves doing
conversions in multiple places and gives a central place to alter
the key when a item is computed.
This will have no side effects other then that the ordering is now
based on strings instead on integers, so the order will be different.
This will however have no effect on existing configs as these will
use the individual codes/keys and not the ordering to determine if
there is a diff or not.
Lastly (but I think also most importantly) there is a fix in this PR
that makes diffing sets extremely more performand. Before a full diff
required reading the complete Set for every single parameter/attribute
you wanted to diff, while now it only gets that specific parameter.
We have a use case where we have a Set that has 18 parameters and the
set consist of about 600 items (don't ask 😉). So when doing a diff
it would take 100% CPU of all cores and stay that way for almost an
hour before being able to complete the diff.
Debugging this we learned that for retrieving every single parameter
it made over 52.000 calls to `func (c *ResourceConfig) get(..)`. In
this function a slice is created and used only for the duration of the
call, so the time needed to create all needed slices and on the other
hand the time the garbage collector needed to clean them up again caused
the system to cripple itself. Next to that there are also some expensive
reflect calls in this function which also claimed a fair amount of CPU
time.
After this fix the number of calls needed to get a single parameter
dropped from 52.000+ to only 2! 😃
I promised myself that next time I jumped in this file I'd fix this up.
Now we don't have to manually index the file with comments, we can just
add descriptive names to the test cases!
Previous this would return the following sort of error:
expected type 'string', got unconvertible type '[]interface {}'
This is the raw error returned by the underlying mapstructure library.
This is not a helpful error message for anyone who doesn't know Go's
type system, and it exposes Terraform's internals to the UI.
Instead we'll catch these cases before we try to use mapstructure and
return a more straightforward message.
By checking the type before the IsComputed exception this also avoids
a crash caused when the assigned value is a computed list. Otherwise
the list of interpolations is allowed through here and then crashes later
during Diff when the value is not a primitive as expected.
A common issue with new resource implementations is not considering parts
of a complex structure that's used inside a set, which causes quirky
behavior.
The schema helper has enough information to provide a default reasonable
implementation of a set function that includes all non-computed attributes
in a deterministic way. Here we implement such a function and use it
when no explicit hashing function is provided.
In order to achieve this we encapsulate the construction of the zero
value for a schema in a new method schema.ZeroValue, which allows us to
put the fallback logic to the new default function in a single spot.
It is no longer valid to use &Set{F: schema.Set} and all uses of that
construct should be replaced with schema.ZeroValue().(*Set) .
We need to set the value to an empty value so the state file does
indeed change the value. Otherwise the obsolote value is still
intact and doesn't get changed at all. This means `terraform show`
still shows the obsolote value when the particular value is not
existing anymore. This is due the AWS API which is returning a null
instead of an empty string.
This was just a missed exit from the resource.Apply function -
subsequent refreshes would add the SchemaVersion back into the state,
but having the state recorded once without the meta information can
cause problems with Atlas's remote state checksumming.
By prefixing them with `cmd /c` it will work with both `winner` and
`ssh` connection types.
This PR also reverts some bad stringer changes made in PR #2673
In `helper/schema` we already makes a distinction between `Default`
which is always applied and `InputDefault` which is displayed to the
user for an empty field.
But for variables we just have `Default` which is treated like
`InputDefault`. This changes it to _not_ prompt the user for a value
when the variable declaration includes a default.
Treating this as a UX bugfix and the "don't prompt for variables w/
defaults set" behavior as the originally expected behavior we were
failing to honor.
Added an already-passing test to verify and cover the `helper/schema`
behavior.
Perhaps down the road we can add a `input_default` attribute to
variables to allow similar behavior to `helper/schema` in variables, but
for now just sticking with the fix.
Fixes#2592
This is the initial pure "all tests passing without a diff" stage. The
plan is to change the internal representation of StringList to include a
suffix delimiter, which will allow us to recognize empty and
single-element lists.
I couldn't see a simple path get this working for Maps, Sets,
and Lists, so lets land it as a primitive-only schema feature.
I think validation on primitives comprises 80% of the use cases anyways.
Guarantees that the `interface{}` arg to ValidateFunc is the proper
type, allowing implementations to be simpler.
Finish the docstring on `ValidateFunc` to call this out.
/cc @mitchellh
The runtime impl of ConfictsWith uses Resource.Get(), which makes it
work with any other attribute of the resource - the InternalValidate was
only checking against the local schemaMap though, preventing subResource
from using ConflictsWith properly.
It's a lot of wiring and it's a bit ugly, but it's not runtime code, so
I'm a bit less concerned about that aspect.
This should take care of the problem mentioned in #1909
Paul Hinze