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.
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) .
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
Removed fields show a customizable error message to the user when they
are used in a Terraform config. This is a tool that provider authors can
use for user feedback as they evolve their Schemas.
refs #957
Deprecated fields show a customizable warning message to the user when
they are used in a Terraform config. This is a tool that provider
authors can use for user feedback as they evolve their Schemas.
fixes#957
We were waiting until the higher-level (m schemaMap) diffString method
to apply defaults, which was messing with set hashcode evaluation for
cases when a field with a default is included in the hash function.
fixes#824
/cc @svanharmelen - I think some logic changed after my refactor. I now
return Exists: true when Computed: true but the value might be blank to
note that the FieldReader FOUND a value, its just unknown. I think
before it didn't do that so the logic for GetOk has to be "does it exist
and is it _not_ computed"
Seems weird because I just realized there is no way to get the OLD value
of something if it is being computed now, but I looked and there are
tests that verify this and they're like... test #5 of Get. So, they're
not new meaning that must've been expected behavior? Hm. Let me know if
you find any other issues from acceptance tests
/cc @phinze - This is pretty straightforward, almost magically so. The
reason this works is because in `diffString` we use mapstructure[1] with
"weak decode mode" to just be responisble for turning anything into a
string.
[1]: https://github.com/mitchellh/mapstructure
Don't check if the root key is being computed for composite types.
Instead, continue recursing the composite type in order to check if
the sub-key, key.N, for each individual element is being computed.
Fixes a panic which occurs when validating a composite type where
the value is an unknown kind for the schema.
This adds "field.#" values to the state/diff with the element count of a
map. This fixes a major issue around not knowing when child elements are
computed when doing variable access of a computed map.
Example, if you have a schema like this:
"foo": &Schema{
Type: TypeMap,
Computed: true,
}
And you access it like this in a resource:
${type.name.foo.computed-field}
Then Terraform will error that "field foo could not be found on resource
type.name". By adding that "foo.#" is computed, Terraform core will pick
up that it WILL exist, so its okay.
This is a refactored solution for PR #616. Functionally this is still
the same change, but it’s implemented a lot cleaner with less code and
less changes to existing parts of TF.
It’s not enough to only check if no new value is set. It can also be
that a new value is set, but contains a variable that cannot be
interpolated until a depending resource is created during the apply
fase.
I actually found this one as one of the acceptance tests for the AWS
ELB resource was failing. It failed with the following error:
```
--- FAIL: TestAccAWSELB_InstanceAttaching (177.83 seconds)
testing.go:121: Step 1 error: Error applying: aws_elb.bar: diffs
didn't match during apply. This is a bug with the resource provider,
please report a bug.
FAIL
exit status 1
FAIL github.com/hashicorp/terraform/builtin/providers/aws 177.882s
```
After a quick look I noticed it was actually a bug in core TF so added
the test and made sure all unit tests and AWS acceptance tests are now
running successfully.
Prior to this, the diff only contained changed set elements. The issue
with this is that `getSet`, the internal function that reads a set from
the ResourceData, expects that each level (state, config, diff, etc.)
has the _full set_ information. This change was done to fix merging
issues.
Because of this, we need to make sure the full set is visible in the
diff.
This fixes a seemingly minor issue (GH-255) around plans showing changes
when in fact there are none. But in reality this turned out to uncover a
really terrible bug.
The effect of what was happening was that multiple items in a set were
being merged. Now, they were being merged in the right order, so if you
didn't have rich types (lists in a set) then you never saw the effect
since the later value would overwrite the earlier. But with lists (such
as in security groups), you would end up with the lists merging. So, if
you had one ingress rule with CIDR blocks and one with SGs, then after
the merge both ingress rules would have BOTH CIDR and SGs, resulting in
an incorrect plan (GH-255).
This fixes the issue by introducing a `getSourceExact` bitflag to the
ResourceData source. When this is set, ALL data must come from this
level, instead of merging lower levels. In the case of sets and diffs,
this is exactly what you want: "Get me the set 'foo' from the config and
the config ONLY (not the state or diff or w/e)".
Andddddd its fixed.
GH-255
Paul Hinze