992 Commits (6cc3e1d0bd635cdae92b3e0055c19d86dfa7a222)

Author	SHA1	Message	Date
Brian Flad	3d908f56aa	helper/schema: Add deprecation to ResourceData.UnsafeSetFieldRaw ... This functionality is no longer supported in Terraform 0.12 and above.	7 years ago
James Bardin	f9b62cb5fe	Merge pull request #20335 from hashicorp/jbardin/diff-apply ... Diff apply needs to check for both types of containers keys	7 years ago
James Bardin	c34c37fbd5	missed .% suffixes in diff.Apply ... Diff.Apply checks for unneeded container count diffs, but was missing the check for maps. Add an early return for planning a destroy.	7 years ago
Martin Atkins	fedbd6c3b8	helper/plugin: fix panic with empty objects in normalizeNullValues ... cty.Value.AsValueMap can return nil if called on an empty map or object. The logic above was dealing with that case for maps, but object types were falling through into this codepath and panicking when trying to assign a new key into the nil dstMap. This also includes a bonus fix where we were calling ty.ElementType in a switch case that accepts object types. Object types don't have a single element type, so we can't call ElementType on those (that also panics) but we _can_ use the type of the value we selected from src to construct our placeholder null value.	7 years ago
Martin Atkins	eb1346447f	Merge #20282: Enforce expected behaviors for provider PlanResourceChange ... An exception remains for the legacy SDK, which does not meet all of these requirements.	7 years ago
Martin Atkins	31299e688d	core: Allow legacy SDK to opt out of plan-time safety checks ... Due to the inprecision of our shimming from the legacy SDK type system to the new Terraform Core type system, the legacy SDK produces a number of inconsistencies that produce only minor quirky behavior or broken edge-cases. To retain compatibility with those existing weird behaviors, the legacy SDK opts out of our safety checks. The intent here is to allow existing providers to continue to do their previous unsafe behaviors for now, accepting that this will allow certain quirky bugs from previous releases to persist, and then gradually migrate away from the legacy SDK and remove this opt-out on a per-resource basis over time. As with the apply-time safety check opt-out, this is reserved only for the legacy SDK and must not be used in any new SDK implementations. We still include any inconsistencies as warnings in the logs as an aid to anyone debugging weird behavior, so that they can see situations where blame may be misplaced in the user-visible error messages.	7 years ago
James Bardin	3cecacb660	Merge pull request #20292 from hashicorp/jbardin/sdk ... allow 0 and unset to be equal in count tests	7 years ago
James Bardin	1bfc27817e	process state even after provider.Apply errors ... Terraform core expects a sane state even when the provider returns an error. Make sure at the prior state is always the default value to return, and then alway attempt to process any state returned by provider.Apply.	7 years ago
James Bardin	c02f1d7256	allow 0 and unset to be equal in count tests ... This was changed in the single attribute test cases, but the AttrPair test is used a lot for data source. As far as tests are concerned, 0 and unset should be treated equally for flatmapped collections.	7 years ago
James Bardin	82588af892	switch blocks based on value type, and check attrs ... Check attributes on null objects, and fill in unknowns. If we're evaluating the object, it either means we are at the top level, or a NestingSingle block was present, and in either case we need to treat the attributes as null rather than the entire object. Switch on the block types rather than Nesting, so we don't need add any logic to change between List/Tuple or Map/Object when DynamicPseudoType is involved.	7 years ago
James Bardin	32671241e0	set unknowns during initial PlanResourceChange ... If ID is not set, make sure it's unknown. Use SetUnknowns to set the rest of the computed values to Unknown.	7 years ago
James Bardin	d17ba647a8	add SetUnknowns ... SetUnknown walks through a resource and changes any unset (null) values that are going computed in the schema to Unknown.	7 years ago
Martin Atkins	1530fe52f7	core: Legacy SDK providers opt out of our new apply result check ... The shim layer for the legacy SDK type system is not precise enough to guarantee it will produce identical results between plan and apply. In particular, values that are null during plan will often become zero-valued during apply. To avoid breaking those existing providers while still allowing us to introduce this check in the future, we'll introduce a rather-hacky new flag that allows the legacy SDK to signal that it is the legacy SDK and thus disable the check. Once we start phasing out the legacy SDK in favor of one that natively understands our new type system, we can stop setting this flag and thus get the additional safety of this check without breaking any previously-released providers. No other SDK is permitted to set this flag, and we will remove it if we ever introduce protocol version 6 in future, assuming that any provider supporting that protocol will always produce consistent results.	7 years ago
James Bardin	3b18dd7c01	Merge pull request #20224 from hashicorp/jbardin/sdk ... SDK set fixes	7 years ago
James Bardin	8be864c1c7	don't allow computed set elems to be equal ... If set elements are computed, we can't be certain that they are actually equal. Catch identical computed set hashes when they are added to the set, and alter the set key slightly to keep the set counts correct. In previous versions the interpolation string would be included in the set, and different string values would cause the set to hash differently, so this is change is only activated for the new protocol.	7 years ago
James Bardin	58c9c2311a	Turn on helper/schema proto5 flag in GetSchema ... This turns it on at the last moment, and in one place for all uses of helper/schema. There's no way to use the new protocol without calling GetSchema, so we can be sure that any subsequent api calls have this set when required.	7 years ago
James Bardin	55b4307767	add proto5 feature flag ... Add feature flag to allow special proto 5 behavior in helper/schema. This is Meant to be used as a last resort for shim-related bugs.	7 years ago
James Bardin	81a4e705b1	DiffSuppressFunc should noop diffs in sets ... Sets rely on diffs being complete for all elements, even when they are unchanged. When encountering a DiffSuppressFunc inside a set the diffs were being dropped entirely, possible causing set elements to be lost.	7 years ago
Martin Atkins	bdcac8792d	plugin: Use correct schema when marshaling imported resource objects ... Previously we were using the type name requested in the import to select the schema, but a provider is free to return additional objects of other types as part of an import result, and so it's important that we perform schema selection separately for each returned object. If we don't do this, we get confusing downstream errors where the resulting object decodes to the wrong type and breaks various invariants expected by Terraform Core. The testResourceImportOther test in the test provider didn't catch this previously because it happened to have an identical schema to the other resource type being imported. Now the schema is changed and also there's a computed attribute we can set as part of the refresh phase to make sure we're completing the Read call properly during import. Refresh was working correctly, but we didn't have any tests for it as part of the import flow.	7 years ago
James Bardin	4a603011c5	don't normalizeNullValues in ReadResource ... The required normalization now happens in PlanResourceChange, and this function is no longer appropriate for ReadResource.	7 years ago
Martin Atkins	4c99864dad	helper/resource: TestCheckResourceAttrPair allow nonexist ... This checking helper is frequently used in provider tests for data sources, as a shorthand to verify that an attribute of the data source matches with the corresponding attribute on a managed resource. Since we now leave empty collections null in more cases, this function is sometimes effectively asked to verify that a given attribute is _unset_ in both the data source and the resource, so here we slightly adjust the definition of the check to consider two nulls to be equal to one another, which at this layer manifests as the keys not being present in the state attributes map at all. This check function didn't previously have tests, so this commit also adds a basic suite of tests, including coverage for the new behavior.	7 years ago
James Bardin	ba081f5de4	change copyMissingValues to normalizeNullValues ... While copyMissingValues was meant to re-insert empty values that were null after apply, it turns out plan is sometimes not predictable as well. normalizeNullValue is meant to fix up any null/empty transitions between to values, and be useful during plan as well. For plan the function only concerns itself with individual, known values, and skips sets entirely. The result of running with plan == true is that only changes between empty and null collections should be fixed.	7 years ago
James Bardin	9cf8f48239	decode legacy timeouts ... The new decoder is more precise, and unpacks the timeout block into a single map, which ResourceTimeout.ConfigDecode was updated to handle. We however still need to work with legacy versions of terraform, with the old decoder.	7 years ago
James Bardin	3b04b41250	fix RequiresNew in diff ... With the new diff.Apply we can keep the diff mostly intact, but we need turn off all RequiresNew flags so that the prior state is not removed from the apply.	7 years ago
Martin Atkins	477da57a92	helper/plugin: Honor resource type overrides in import ... One quirky aspect of our import feature is that we allow the importer to produce additional resources alongside the one that was imported, such as to create separate rules for each rule of an imported security group. Providers need to be able to set the types of these other resources since they may not match the "main" resource type. They do this by calling ResourceData.SetType, which in turn sets InstanceState.Ephemeral.Type. In our shims here we therefore need to copy that out into our new TypeName field so that the new core import code can see it and create the right type in the state. Testing this required a minor change to the test harness to allow the ImportStateCheck function to see the resource type.	7 years ago
Paul Tyng	bb9ae50279	Copy TF version to helper/schema provider	7 years ago
Martin Atkins	ae0be75ae0	helper/schema: TypeMap of Resource is actually of TypeString ... Historically helper/schema did not support non-primitive map attributes because they cannot be represented unambiguously in flatmap. When we initially implemented CoreConfigSchema here we mapped that situation to a nested block of mode NestingMap, even though that'd never worked until now, assuming that it'd be harmless because providers wouldn't be using it. It turns out that some providers are, in fact, incorrectly populating a TypeMap schema with Elem: &schema.Resource, apparently under the false assumption that it would constrain the keys allowed in the map. In practice, helper/schema has just been ignoring this and treating such attributes as map of string. (#20076) In order to preserve the behavior of these existing incorrectly-specified attribute definitions, here we mimic the helper/schema behavior by presenting as an attribute of type map(string). These attributes have also been shown in some documentation as nested blocks (with no equals sign), so that'll need to be fixed in user configurations as they upgrade to Terraform 0.12. However, the existing upgrade tool rules will take care of that as a natural consequence of the name being indicated as an attribute in the schema, rather than as a block type. This fixes #20076.	7 years ago
James Bardin	37b5e2dc87	don't remove empty diff values ... Our new diff handling no longer requires stripping the empty diffs out, and provider may be relying on some of the empty-value quirks in helper/schema.	7 years ago
James Bardin	46a4628782	Merge pull request #20081 from hashicorp/jbardin/list-block ... New Diff.Apply method	7 years ago
Martin Atkins	f65b7c5372	helper/plugin: Discard meaningless differences from provider planning ... Due to various inprecisions in the old SDK implementation, applying the generated diff can potentially make changes to the data structure that have no real effect, such as replacing an empty list with a null list or vice-versa. Although we can't totally eliminate such diff noise, here we attempt to avoid it in situations where there are _only_ meaningless changes -- where the prior state and planned state are equivalent -- by just echoing back the prior state verbatim to ensure that Terraform will treat it as a noop change. If there _are_ some legitimate changes then the result may still contain meaningless changes alongside it, but that is just a cosmetic problem for the diff renderer, because the meaningless changes will be ignored altogether during a subsequent apply anyway. The primary goal here is just to ensure we can converge on a fixpoint when there are no explicit changes in the configuration.	7 years ago
James Bardin	8d302c5bd2	update grpc_provider for new diffs ... Keep the diff as-is before applying.	7 years ago
James Bardin	286cb0a39d	clean out diff a little more before checking ... Check if there wasn't any real diff attributes first, before returning the original state in PlanResourceChange.	7 years ago
James Bardin	4f691c5988	don't replace null strings with empty strings ... This adds unexpected values in some cases, and since the case this handles is only within set objects, we'll deal woth this when tackling the sets themselves.	7 years ago
James Bardin	2cc651124e	don't overwrite values in plan ... Plan can change known values too, which we can't match in sets. We'll find another way to normalize these eithout losing plan values.	7 years ago
James Bardin	7d05dee08d	refactor ApplyResourceChange ... Remove a bunch of indentation by returning early, and make sure we don't fail on non-fatal error without saving the applied value.	7 years ago
James Bardin	0a731167db	add a round trip through the shims during apply ... Cycle through the shim operations after Apply, to ensure that we can converge on a stable value for for Plan. While the shims produce valid values in both directions, helper/schema sometimes does not agree on which containers should be empty or null.	7 years ago
Martin Atkins	86c02d5c35	command: "terraform init" can partially initialize for 0.12upgrade ... There are a few constructs from 0.11 and prior that cause 0.12 parsing to fail altogether, which previously created a chicken/egg problem because we need to install the providers in order to run "terraform 0.12upgrade" and thus fix the problem. This changes "terraform init" to use the new "early configuration" loader for module and provider installation. This is built on the more permissive parser in the terraform-config-inspect package, and so it allows us to read out the top-level blocks from the configuration while accepting legacy HCL syntax. In the long run this will let us do version compatibility detection before attempting a "real" config load, giving us better error messages for any future syntax additions, but in the short term the key thing is that it allows us to install the dependencies even if the configuration isn't fully valid. Because backend init still requires full configuration, this introduces a new mode of terraform init where it detects heuristically if it seems like we need to do a configuration upgrade and does a partial init if so, before finally directing the user to run "terraform 0.12upgrade" before running any other commands. The heuristic here is based on two assumptions: - If the "early" loader finds no errors but the normal loader does, the configuration is likely to be valid for Terraform 0.11 but not 0.12. - If there's already a version constraint in the configuration that excludes Terraform versions prior to v0.12 then the configuration is probably _already_ upgraded and so it's just a normal syntax error, even if the early loader didn't detect it. Once the upgrade process is removed in 0.13.0 (users will be required to go stepwise 0.11 -> 0.12 -> 0.13 to upgrade after that), some of this can be simplified to remove that special mode, but the idea of doing the dependency version checks against the liberal parser will remain valuable to increase our chances of reporting version-based incompatibilities rather than syntax errors as we add new features in future.	7 years ago
Martin Atkins	0c0a437bcb	Move module install functionality over to internal/initwd	7 years ago
James Bardin	041ed67e46	type names don't imply the resource mode ... The addr type doesn't imply the resource mode, so data sources and managed resources with the same type name could shim incorrectly.	7 years ago
James Bardin	e8096e9c8b	normalize values during ReadResource ... Match the normalization behavior of Apply, so we don't end up causing any diffs between zero values when refreshing resources.	7 years ago
James Bardin	bc5eecd7f2	make sure id really gets set in SetId ... SetId needs to overwrite the newState as well, since the internal calls to DataSource.Id() will override the set attribute.	7 years ago
James Bardin	a7b399cb4c	use actual schema.Resources for state shims ... Provider tests often rely on checking values contained within sets, by directly accessing their flatmapped representation. In order to provider the test harness with the expected set hashes, the sets must be generated by the schema.Resource itself. During the test we now build a fixed map of the providers, which should only contain schema.Provider instances, and pass them into each TestStep. The individual schema.Resource instances can then be pulled from the providers, and used to recreate the state from the cty.Value returned by the core operations.	7 years ago
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"	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
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.	7 years ago
James Bardin	8ab5698e2a	Merge pull request #19587 from hashicorp/jbardin/safe-appends ... don't modify argument slices	7 years ago