Commit Graph

950 Commits

Author SHA1 Message Date
James Bardin 7973872524 allow TestCheckNoResourceAttr for empty containers
Stricter type handling in the new shims may add empty containers into
the state where they were previously elided. Since the detection of
missing and empty containers in the legacy state was never reliable,
allow TestCheckNoResourceAttr to succeed if the key is a container count
index, and the value is "0"
2019-01-09 13:09:02 -05:00
James Bardin c63040c737 have TestCheckResourceAttr accept missing counts
Missing containers were often erroneously kept in the state, but since
the addition of the new provider shims, they can often be correctly
eliminated. There are however many tests that check for a "0" count in
the flatmap state when there shouldn't be a key at all. This addition
looks for a container count key and "0" pair, and allows for the key to
be missing.

There may be some tests negatively effected by this which were
legitimately checking for empty containers, but those were also not
reliably detected, and there should be much fewer tests involved.
2019-01-09 13:01:17 -05:00
James Bardin b55ec74c27 add copyMissingValues for normalizing shimmed Vals
Zero values and empty containers can be lost during the shimming
process, and during the provider's Apply step.

If we have known zero value containers and primitives in the source,
which appear as null values in the destination, we copy over the zero
value. Sets (and lists to an extent) are more difficult, since there
before and after indexes may not correlate. In that case we take the
entire container if it's wholly known, expecting the provider to have
correctly handled the value.
2019-01-08 16:26:22 -05:00
James Bardin 8300d65539 don't strip sets with count 1 when normalizing
normalizeFlatmapContainers should retain sets with a count of 1, and
convert sets with a count of 0 if they were 1 before the Apply step.
2019-01-08 16:26:21 -05:00
Martin Atkins cdad78d69b helper/resource: Allow multiple providers in a single TestCase
Due to incorrect use of a loop iterator variable inside a closure, all of
the given providers were ending up with the same factory function.
Now we copy the factory function to a local within the loop first so that
each iteration has its own variable.

This is the second round of similar bugs in this function, so we'll also
add a test case for it to reduce the risk of future regressions given that
most real callers don't exercise this with multiple providers in practice.
2019-01-07 16:58:36 -08:00
Martin Atkins b190d3b4f2 helper/resource: Shim back to old state must preserve schema version
We use a shim to convert from the new state model back to the old because
the provider test API is still using the old API throughout. However, the
shim was not preserving the schema version recorded in the new-style state
and so a round-trip through this shim would cause the schema versions to
all revert to zero.

This can cause trouble with the destroy phase of provider tests because
(for API legacy reasons) we round-trip from old state back to new again
before the destroy phase and thus causing the providers to try to upgrade
from state version zero even though the data was already latest, which
can cause errors because state upgrades are generally not idempotent.
2019-01-05 10:00:30 -08:00
Martin Atkins 06acc3f6c8 helper/schema: Skip validation of unknown values
With the introduction of explicit "null" in 0.12 it's possible for a value
that is unknown during plan to become a known null during apply, so we
need to slightly weaken our validation rules to accommodate that, in
particular skipping the validation of conflicting attributes if the result
could potentially be valid after the unknown values become known.

This change is in the codepath that is common to both 0.12 and 0.11
callers, but that's safe because 0.11 re-runs validation during the apply
step and so will still catch problems here, albeit in the apply step
rather than in the plan step, thus matching the 0.12 behavior. This new
behavior is a superset of the old in the sense that everything that was
valid before is still valid.

The implementation here also causes us to skip all other validation for
an attribute whose value is unknown. Most of the downstream validation
functions handle this directly anyway, but again this doesn't add any new
failure cases, and should clean up some of the rough edges we've seen with
unknown values in 0.11 once people upgrade to 0.12-compatible providers.
Any issues we now short-circuit during planning will still be caught
during apply.

While working on this I found that the existing "Not a list" test was not
actually testing the correct behavior, so this also includes a tweak to
that to ensure that it really is checking the "should be a list" path
rather than the "cannot be set" codepath it was inadvertently testing
before.
2019-01-04 14:46:47 -08:00
James Bardin 8ab5698e2a
Merge pull request #19587 from hashicorp/jbardin/safe-appends
don't modify argument slices
2018-12-10 15:10:02 -05:00
James Bardin 3d6ec09a83
Merge pull request #19552 from olindata/bugfix/setting-sets-in-list
helper/schema: Fix setting a set in a list caused error
2018-12-10 12:25:23 -05:00
James Bardin b5de50c0a2 don't modify argument slices
There were a couple spots where argument slices weren't being copied
before `append` was called, which could possibly modify the caller's
slice data.
2018-12-10 11:59:27 -05:00
Martin Atkins f9fef56167 helper/resource: print full diagnostics for operation errors in tests
This causes the output to include additional helpful context such as
the values of variables referenced in the config, etc. The output is in
the same format as normal Terraform CLI error output, though we don't
retain a source code cache in this codepath so it will not include a
source code snippet.
2018-12-07 17:05:36 -08:00
Martin Atkins 55469cd416 helper/resource: Get schemas from Terraform context
Previously the test harness was preloading schemas from the providers
before running any test steps.

Since terraform.NewContext already deals with loading provider schemas,
we can instead just use the schemas it loaded for our shimming needs,
avoiding the need to reimplement the schema lookup behavior and thus
the need to create a throwaway provider instance with which to do it.
2018-12-07 08:12:59 -08:00
Martin Atkins a4991c5780 helper/resource: Create a separate provider instance each call
Previously we were running the factory function only once when
constructing the provider resolver, which means that all contexts created
from that resolver share the same provider instance.

Instead now we will call the given factory function once for each
instantiation, ensuring that each caller ends up with a separate object
as would be the case in real-world use.
2018-12-07 08:12:59 -08:00
James Bardin 98870fadb1
Merge pull request #19544 from hashicorp/jbardin/import-tests
Fix provider import tests
2018-12-05 20:30:46 -05:00
James Bardin ac63d2995f
Merge pull request #19559 from hashicorp/jbardin/resource-test-shim
don't add numeric indexes to resources with a count of 0
2018-12-05 17:34:30 -05:00
James Bardin 7d296f752c don't add numeric indexes to resources with a count of 0 2018-12-05 13:41:53 -05:00
Farid Neshat 44a45b7332 helper/schema: Fix setting a set in a list
The added test in this commit, without the fix, will make d.Set return
the following error:

`Invalid address to set: []string{"ports", "0", "set"}`

This was due to the fact that setSet in feild_writer_map tried to
convert a slice into a set by creating a temp set schema and calling
writeField on that with the address(`[]string{"ports", "0", "set"}"` in
this case). However the temp schema was only for the set and not the
whole schema as seen in the address so, it should have been `[]string{"set"}"`
so it would align with the schema.

This commits adds another variable there(tempAddr) which will only
contain the last entry of the address that would be the set key, which
would match the created schema

This commit potentially fixes the problem described in #16331
2018-12-05 10:09:54 +01:00
Brian Flad 1e81a3e7fa
helper/schema: Always propagate NewComputed for previously zero value primative type attributes
When the following conditions were met:
* Schema attribute with a primative type (e.g. Type: TypeString) and Computed: true
* Old state of attribute set to zero value for type (e.g. "")
* Old state ID of resource set to non-empty (e.g. existing resource)

Attempting to use CustomizeDiff with SetNewComputed() would result in the difference  previously being discarded. This update ensures that previous zero values or resource existence does not influence the propagation of the computed update.

Previously:

```
--- FAIL: TestSetNewComputed (0.00s)
    --- FAIL: TestSetNewComputed/NewComputed_should_always_propagate (0.00s)
        resource_diff_test.go:684: Expected (*terraform.InstanceDiff)(0xc00051cea0)({
             mu: (sync.Mutex) {
              state: (int32) 0,
              sema: (uint32) 0
             },
             Attributes: (map[string]*terraform.ResourceAttrDiff) (len=1) {
              (string) (len=3) "foo": (*terraform.ResourceAttrDiff)(0xc0003dcec0)({
               Old: (string) "",
               New: (string) "",
               NewComputed: (bool) true,
               NewRemoved: (bool) false,
               NewExtra: (interface {}) <nil>,
               RequiresNew: (bool) false,
               Sensitive: (bool) false,
               Type: (terraform.DiffAttrType) 0
              })
             },
             Destroy: (bool) false,
             DestroyDeposed: (bool) false,
             DestroyTainted: (bool) false,
             Meta: (map[string]interface {}) <nil>
            })
            , got (*terraform.InstanceDiff)(0xc00051ce80)({
             mu: (sync.Mutex) {
              state: (int32) 0,
              sema: (uint32) 0
             },
             Attributes: (map[string]*terraform.ResourceAttrDiff) {
             },
             Destroy: (bool) false,
             DestroyDeposed: (bool) false,
             DestroyTainted: (bool) false,
             Meta: (map[string]interface {}) <nil>
            })

--- FAIL: TestSchemaMap_Diff (0.01s)
    --- FAIL: TestSchemaMap_Diff/79-NewComputed_should_always_propagate_with_CustomizeDiff (0.00s)
        schema_test.go:3289: expected:
            *terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{"foo":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, Meta:map[string]interface {}(nil)}

            got:
            <nil>

FAIL
FAIL  github.com/hashicorp/terraform/helper/schema  0.825s
```
2018-12-04 22:48:30 -05:00
James Bardin 484d67028a handle shim errors in provider tests
These should be rare, and even though it's likely a shim bug, the error
is probably easier for provider developers to deal with than the
panic
2018-12-04 16:33:16 -05:00
James Bardin 547d63bcde remove empty flatmap containers from test states
Don't compare attributes with zero-length flatmap continers in tests.
2018-12-04 16:33:16 -05:00
James Bardin 924b97238f Handle StateFuncs in provider shim
Any state modifying functions can only be run once during the plan-apply
cycle. When regenerating the Diff during ApplyResourceChange, strip out
all StateFunc and CustomizeDiff functions from the schema.

Thew NewExtra diff field was where config data that was modified by a
StateFunc was stored, and needs to be maintained between plan and apply.

During PlanResourceChange, store any NewExtra data from the Diff in the
PlannedPrivate data, and re-insert the NewExtra data into the Diff
generated during ApplyResourceChange.
2018-12-03 18:12:02 -05:00
James Bardin 3dacdba678
Merge pull request #19521 from hashicorp/jbardin/prepare-provider-config
catch conversion errors in PrepareProviderConfig
2018-11-30 15:32:56 -05:00
James Bardin 5f9b189fcf catch conversion errors in PrepareProviderConfig
Errors were being ignore with the intention that they would be caught
later in validation, but it turns out we nee dto catch those earlier.

The legacy schemas also allowed providers to set and empty string for a
bool value, which we need to handle here, since it's not being handled
from user input like a normal config value.
2018-11-30 14:51:52 -05:00
Martin Atkins 72e279e6b2 providers: Consistently use int64 for schema versions
The rest of Terraform is still using uint64 for this in various spots, but
we'll update that gradually later. We use int64 here because that matches
what's used in our protobuf definition, and unsigned integers are not
portable across all of the protobuf target languages anyway.
2018-11-30 11:22:39 -08:00
James Bardin 6daf4989d4
Merge pull request #19475 from hashicorp/jbardin/computed-containers
Shim computed containers
2018-11-27 09:15:23 -05:00
James Bardin 6f4d86094f preserve possible zero values when normalizing
When normalizing flatmapped containers, compare the attributes to the
prior state and preserve pre-existing zero-length or unknown values. A
zero-length value that was previously unknown is preserved as a
zero-length value, as that may have been computed as such by the
provider.
2018-11-27 08:54:15 -05:00
Martin Atkins 58fa38b89a helper/schema: Update docs for PromoteSingle
This is no longer effective and should not be used in any new schema.
2018-11-26 17:11:34 -08:00
Martin Atkins 3d54af9c09 helper/schema: Better mimic some undocumented behaviors in Core schema
Since the SDK's schema system conflates attributes and nested blocks, it's
possible to state some nonsensical schema situations such as:

- A nested block is both optional but has MinItems > 0
- A nested block is entirely computed but has MinItems or MaxItems set

Both of these weird situations are handled here in the same way that the
existing helper/schema validation code would've handled them: by
effectively disabling the MinItems/MaxItems checks where they would've
been ignored before.

the MinItems/MaxItems
2018-11-26 17:11:34 -08:00
Martin Atkins 37da625ee9 helper/schema: Tell Core attribute is optional if set conditionally
The SDK has a mechanism that effectively makes it possible to declare an
attribute as being _conditionally_ required, which is not a concept that
Terraform Core is aware of.

Since this mechanism is in practice only used for a small UX improvement
in prompting for these values interactively when the environment variable
is not set, we avoid here introducing all of this complexity into the
plugin protocol by just having the provider selectively modify its schema
if it detects that such an attribute might be set dynamically.

This then prevents Terraform Core from validating the presence of the
argument or prompting for a new value for it, allowing the null value to
pass through into the provider so that the default value can be generated
again dynamically.

This is a kinda-kludgey solution which we're accepting here because the
alternative would be a much-more-complex two-pass decode operation within
Core itself, and that doesn't seem worth it.

This fixes #19139.
2018-11-26 17:11:34 -08:00
James Bardin f375691819 add missing key-value from test 2018-11-19 18:58:29 -05:00
Martin Atkins 4fe9632f09 plugin: Establish our current plugin protocol as version 5
The main significant change here is that the package name for the proto
definition is "tfplugin5", which is important because this name is part
of the wire protocol for references to types defined in our package.

Along with that, we also move the generated package into "internal" to
make it explicit that importing the generated Go package from elsewhere is
not the right approach for externally-implemented SDKs, which should
instead vendor the proto definition they are using and generate their
own stubs to ensure that the wire protocol is the only hard dependency
between Terraform Core and plugins.

After this is merged, any provider binaries built against our
helper/schema package will need to be rebuilt so that they use the new
"tfplugin5" package name instead of "proto".

In a future commit we will include more elaborate and organized
documentation on how an external codebase might make use of our RPC
interface definition to implement an SDK, but the primary concern here
is to ensure we have the right wire package name before release.
2018-11-19 09:56:41 -08:00
James Bardin e95f2b586e another test case in helper/plugin 2018-11-16 15:12:16 -05:00
James Bardin 3716db3865
Merge pull request #19384 from hashicorp/jbardin/nested-sets
New Attribute and Diff handling in shims
2018-11-16 11:55:41 -05:00
James Bardin 89b2c6f21e comment fixes 2018-11-16 11:24:14 -05:00
James Bardin 17ecda53b5 strip empty containers from flatmap attributes
In order to prevent mismatched states between read/plan/apply, we need
to ensure that the attributes are generated consistently each time.
Because of the various ways in which helper/schema and the hcl2 shims
interpret empty values, the only way to ensure consistency is to always
remove them altogether.
2018-11-16 09:59:03 -05:00
James Bardin 21dfa56766 use ShimInstanceStateFromValue in DiffFromValues
This makes sure the diff is generated with the matching set ids from
helper/schema.

Update the tests to add ID fields to the state, which will exists in
practice, since any state traversing through the shims will have the ID
inserted.
2018-11-16 09:59:03 -05:00
James Bardin 34766ca666 use the new InstanceState shim 2018-11-16 09:59:03 -05:00
James Bardin df04e2e7a6 move InstanceState shim into schema.Resource
This was the resource can rebuild the flatmapped state using the
schema and ResourceData, providing us the the correct set key values.
2018-11-16 09:59:03 -05:00
James Bardin b872491baa incremental progress towards applying diffs 2018-11-16 09:58:42 -05:00
James Bardin ce5d7ff6d0 spelling 2018-11-13 18:41:53 -05:00
Dana Hoffman 7edbb3c8bf return state even if cfg is invalid 2018-11-12 21:49:30 -05:00
Radek Simko 0cbf745e5a
helper/schema: Avoid erroring out on undefined timeouts 2018-11-07 15:38:58 +00:00
Radek Simko 7eae051a16
Merge pull request #19286 from hashicorp/radeksimko/b-timeouts-parsing-fix
helper/schema: Fix timeout parsing during Provider.Diff
2018-11-06 11:18:31 +00:00
Radek Simko 1cb8f1df80
helper/schema: Fix timeout parsing in ResourceTimeout.ConfigDecode 2018-11-05 12:42:12 +00:00
Radek Simko 82a77f9bb5
helper/schema: Add test for invalid timeout value 2018-11-05 12:42:12 +00:00
Radek Simko 2fe3f16cb3
helper/schema: Return error on invalid timeout type 2018-11-05 12:42:11 +00:00
Radek Simko 186a6dcc38
helper/schema: Add test for wrong timeout type 2018-11-05 12:42:11 +00:00
James Bardin ae1f93a24f
Merge pull request #19236 from hashicorp/jbardin/resource-tests
skip resource tests for now
2018-11-01 11:52:50 -04:00
James Bardin a5ef403dfd skip resource tests for now
These aren't going to be fixed in the immediate future, and are
preventing the CI tests from being helpful.
2018-10-31 14:17:23 -04:00
James Bardin 718a3c400a fix state variable name 2018-10-31 13:43:50 -04:00