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) .
It seems there are 4 locations left that use the `helper/multierror`
package, where the rest is TF settled on the `hashicorp/go-multierror`
package.
Functionally this doesn’t change anything, so I suggest to delete the
builtin version as it can only cause confusion (both packages have the
same name, but are still different types according to Go’s type system.
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.
The Exists function can run in a context where the contents of the
template have changed, but it uses the old set of variables from the
state. This means that when the set of variables changes, rendering will
fail in Exists. This was returning an error, but really it just needs to
be treated as a scenario where the template needs re-rendering.
fixes#2344 and possibly a few other template issues floating around
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
This reworks the template lifecycle a bit such that we get nicer diff
behavior.
First, we tick ForceNew on for both filename and vars, so that the diff
indicates that the template will be "replaced" on change. This is mostly
cosmetic, but it also tracks conceptually with the fact that the
identifier we use is a hash of the contents, so any change essentially
makes a "new resource".
Second, we change the Exists implementation to only return `false` when
there has been a change in the rendered template. This lets descendent
resources see the computed value changing so that they'll properly
trigger in the plan.
Fixes#1898
Refs #1866 (but does not fix, there's another deeper issue there)
Other than the fact that "The the" doesn't really make any sense anywhere
that it's used in Terraform, they're a post-punk band from the UK.
Fixes "The The" so that they can get back to playing songs.
AccTests like TestAccComputeInstance_basic_deprecated_network were
failing early on "invalid config" when we are explictly testing behavior
that we know generates warnings.
This fixes some perpetual diffs I saw in Atlas AccTests where an empty
map (`map[string]interface{}{}`) was being `d.Set` for "metadata_full".
Because the MapFieldWriter was not distinguishing between empty and nil,
this trigger the "map delete" logic and no count was written to the
state. This caused subsequent plans to improperly report a diff.
Here we redefine the map delete functionality to explicitly trigger only
on `nil`, so we catch the `.#` field for empty maps.
- Users
- Groups
- Roles
- Inline policies for the above three
- Instance profiles
- Managed policies
- Access keys
This is most of the data types provided by IAM. There are a few things
missing, but the functionality here is probably sufficient for 95% of
the cases. Makes a dent in #28.
Do directory expansion on filenames.
Add basic acceptance tests. Code coverage is 72.5%.
Uncovered code is uninteresting and/or impossible error cases.
Note that this required adding a knob to
helper/resource.TestStep to allow transient
resources.
This is needed as preperation for adding WinRM support. There is still
one error in the tests which needs another look, but other than that it
seems like were now ready to start working on the WinRM part…
I forgot to add `Computed: true` when I made the "key_name" field
optional in #1751.
This made the behavior:
* Name generated in Create and set as ID
* Follow up plan (without refresh) was nice and empty
* During refresh, name gets cleared out on Read, causing a bad diff on
subsequent plans
We can automatically catch bugs like this if we add yet another
verification step to our resource acceptance tests -> a post
Refresh+Plan that we verify is empty.
I left the non-refresh Plan verification in, because it's important that
_both_ of these are empty after an Apply.
Adds an "alias" field to the provider which allows creating multiple instances
of a provider under different names. This provides support for configurations
such as multiple AWS providers for different regions. In each resource, the
provider can be set with the "provider" field.
(thanks to Cisco Cloud for their support)
It turned out the tests didn’t work as expected due to some missing
config in the `newMockLineServer` and a defer located in the wrong
location. All is good again now…
Each acceptance test step plays a Refresh, Plan, Apply for a given
config. This adds a follow up Plan and fails the test if it does not
come back empty. This will catch issues with perpetual, unresolvable
diffs that crop up here and there.
This is going to cause a lot of our existing acceptance tests to fail -
too many to roll into a single PR. I think the best plan is to land this
in master and then fix the failures (each of which should be catching a
legitimate provider bug) one by one until we get the provider suites
back to green.
If a given resource does not define an `Update` function, then all of
its attributes must be specified as `ForceNew`, lest Applys fail with
"doesn't support update" like #1367.
This is something we can detect automatically, so this adds a check for
it when we validate provider implementations.
Add `-target=resource` flag to core operations, allowing users to
target specific resources in their infrastructure. When `-target` is
used, the operation will only apply to that resource and its
dependencies.
The calculated dependencies are different depending on whether we're
running a normal operation or a `terraform destroy`.
Generally, "dependencies" refers to ancestors: resources falling
_before_ the target in the graph, because their changes are required to
accurately act on the target.
For destroys, "dependencies" are descendents: those resources which fall
_after_ the target. These resources depend on our target, which is going
to be destroyed, so they should also be destroyed.
We were previously only recording the schema version on refresh. This
caused the state to be incorrectly written after a `terraform apply`
causing subsequent commands to run the state through an unnecessary
migration.
Providers get a per-resource SchemaVersion integer that they can bump
when a resource's schema changes format. Each InstanceState with an
older recorded SchemaVersion than the cureent one is yielded to a
`MigrateSchema` function to be transformed such that it can be addressed
by the current version of the resource's Schema.
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
Now that readMap filters out '#' fields, when maps are nested in sets,
we exposed a related bug where a set was iterating over nested maps and
expected the '#' key to be present in those nested maps.
By skipping _all_ count fields when iterating over set keys, all is
right with the world again.
An `InstanceDiff` will include `ResourceAttrDiff` entries for the
"length" / `#` field of maps. This makes sense, since for something like
`terraform plan` it's useful to see when counts are changing.
The `DiffFieldReader` was not taking these entries into account when
reading maps out, and was therefore incorrectly returning maps that
included an extra `'#'` field, which was causing all sorts of havoc
for providers (extra tags on AWS instances, broken google compute
instance launch, possibly others).
* fixes#914 - extra tags on AWS instances
* fixes#883 - general core issue sprouted from #757
* removes the hack+TODO from #757
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
This was actually quite nasty as the first bug covered the second one…
The first bug is with HasChange. This function uses reflect.DeepEqual
to check if two instances are the same/have the same content. This
works fine for all types except for Set’s as they contain a function.
And reflect.DeepEqual will only say the functions are equal if they are
both nil (which they aren’t in a Set). So in effect it means that
currently HasChange will always say true for Set’s, even when they are
actually being equal.
As soon as you fix this problem, you will notice the second one (which
the added test is written for). Without saying you want the exact diff,
you will end up with a merged value which will (in most cases) be the
same.
Run all unit tests and a good part of the acc tests to verify this
works as expected and all look good.
Currently the `sync.Once` call is only used to init a Set in the add()
func. So when you add a value to a Set that is the result of one of the
Set operations (i.e. union, difference, intersect) the Set will be
reinitialised and the exiting values will be lost.
I don’t have a clue why this is showing up in my ACC tests just now, as
this code is in there for quite some time already. Somehow it seems to
have something to do with the refactoring of the helper/schema done
last week, as I cannot reproduce this with
47f02f80bc
/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.
Mitchell Hashimoto