92 Commits (5b6b1663ef3c19ff9b03058347af672da2502e12)

Author	SHA1	Message	Date
James Bardin	c9e1d26c25	remove the legacy schema access ... Having removed the methods, it is straightforward to mechanically update this file to get rid of all references to the "legacy schema". There is now only one config schema type to deal with in the sdk.	7 years ago
James Bardin	ec65fb960d	sdk: use core schema for json state upgrade ... When handling the json state in UpgradeResourceState, the schema must be what core uses, because that is the schema used for encoding/decoding the json state. When converting from flatmap to json state, the legacy schema will be used to decode the flatmap to a cty value, but the resulting json will be encoded using the CoreConfigSchema to match what core expects.	7 years ago
James Bardin	cf61a689eb	only hold back empty container changes in apply ... When normalizing the state during read, if the resource was previously imported, most nil-able values will be nil, and we need to prefer the values returned by the latest Read operation. This didn't come up before, because Read is usually working with a state create by plan and Apply which has already shaped the state with the expected empty values. Having the src value preferred only during Apply better follows the intent of this function, which should allow Read to return whatever values it deems necessary. Since Read and Plan use the same normalization logic, the implied Read before plan should take care of any perpetual diffs.	7 years ago
James Bardin	b7ff04f1b6	remove extra attributes from state during upgrade ... Terraform core would previously ignore unexpected attributes found in the state, but since we now need to encode/decode the state according the schema, the attributes must match the schema. On any state upgrade, remove attributes no longer present in the schema from the state. The only change this requires from providers is that going forward removal of attribute is considered a schema change, and requires an increment of the SchemaVersion in order to trigger the removal of the attributes from state.	7 years ago
James Bardin	2fe0f9376a	change "preferDst" to "apply" in normalizeNullVals ... Inverting this and renaming it makes it align with the current use of that boolean argument.	7 years ago
James Bardin	0696cf7245	don't retain removed map values ... Make sure values removed from a map during apply are not copied into the new map. The broken test is no longer valid in this case, and the updated diff.Apply should prevent the case it used to cover.	7 years ago
Martin Atkins	88e76fa9ef	configs/configschema: Introduce the NestingGroup mode for blocks ... In study of existing providers we've found a pattern we werent previously accounting for of using a nested block type to represent a group of arguments that relate to a particular feature that is always enabled but where it improves configuration readability to group all of its settings together in a nested block. The existing NestingSingle was not a good fit for this because it is designed under the assumption that the presence or absence of the block has some significance in enabling or disabling the relevant feature, and so for these always-active cases we'd generate a misleading plan where the settings for the feature appear totally absent, rather than showing the default values that will be selected. NestingGroup is, therefore, a slight variation of NestingSingle where presence vs. absence of the block is not distinguishable (it's never null) and instead its contents are treated as unset when the block is absent. This then in turn causes any default values associated with the nested arguments to be honored and displayed in the plan whenever the block is not explicitly configured. The current SDK cannot activate this mode, but that's okay because its "legacy type system" opt-out flag allows it to force a block to be processed in this way anyway. We're adding this now so that we can introduce the feature in a future SDK without causing a breaking change to the protocol, since the set of possible block nesting modes is not extensible.	7 years ago
James Bardin	af8115dc9b	removing the ~ set flag is no longer needed ... The computed set sigil ~ should no longer appear in the diffs, because the config will be cleaned before generating the diff.	7 years ago
James Bardin	a3d58665ad	use LegacyResourceSchema ... rather than the previous .CoreConfigSchemaForShimming	7 years ago
James Bardin	c5023c7702	cleanup after AsSingle removal	7 years ago
James Bardin	1a9c06d0f5	Revert "helper/schema: Implementation of the AsSingle mechanism" ... This reverts commit 1987a92386.	7 years ago
James Bardin	7b67105407	don't strip new-computeds from plan diffs ... Stripping these was a patch for some provider behavior which was fixed in other ways, and is no longer needed. Removing this allows us to implement correct CusomizeDiffFuncs in providers so that they can mark fields with empty values as computed during a plan.	7 years ago
James Bardin	e024960c74	Revert "filter nulls when shimming a config" ... This reverts commit 97bde5467c.	7 years ago
James Bardin	f5395bd98a	validate null values in shimmed configs ... A list-like attribute containing null values will present a list to helper/schema with nils, which can cause panics. Since null values were not possible in configuration before HCL2 and not supported by the legacy SDK, return an error to the user.	7 years ago
James Bardin	64c76be804	fixup lost collections containing unknowns ... It turns out that collections containing only unknowns could be lost, meaning there wasn't a direct correlation between the unknown and null value which would have otherwise been restored.	7 years ago
James Bardin	86e30add98	fix unknowns added to maps by schemaMap ... The legacy diff process inserts unknown values into an optional+computed map. Fix these up in post-plan normalization process, by looking for known strings that were changed to unknown.	7 years ago
James Bardin	009df443f7	restore lost unknowns during a planned update. ... Because schema.ResourceDiff can't differentiate between unknown values and new computed values, unknowns can be lost during an update. If a planned value converted an unknown to a null, restore the unknown so that it can be correctly replaced in the final plan.	7 years ago
Martin Atkins	135121562e	helper/plugin: Implement Schema.SkipCoreTypeCheck ... The previous commit added this flag but did not implement it. Here we implement it by adjusting the shape of schema we return to Terraform Core to mark the attribute as untyped and then ensure that gets handled correctly on the SDK side.	7 years ago
Martin Atkins	1987a92386	helper/schema: Implementation of the AsSingle mechanism ... The previous commit added a new flag to schema.Schema which is documented to make a list with MaxItems: 1 be presented to Terraform Core as a single value instead, giving a way to switch to non-list nested resources without it being a breaking change for Terraform v0.11 users as long as it's done prior to a provider's first v0.12-compatible release. This is the implementation of that mechanism. It's intentionally implemented as a suite of extra fixups rather than direct modifications to existing shim code because we want to ensure that this has no effect whatsoever on the result of a resource type that _isn't_ using AsSingle. Although there is some small unit test coverage of the fixup steps here, the primary testing for this is in the test provider since the integration of all of these fixup steps in the correct order is the more important result than any of the intermediate fixup steps.	7 years ago
James Bardin	e080706e2e	treat normalization during ReadResource like Plan ... This will allow resources to return an unexpected change to set blocks and attributes, otherwise we could mask these changes during normalization. Change the "plan" argument in normalizeNullValues to "preferDst" to more accurately describe what the option is doing, since it no longer applies only to PlanResourceChange.	7 years ago
James Bardin	6ecf9b143b	we can normalize nulls in Read again ... This should be the final change from removing the flatmap normalization. Since we're no longer trying to a consistent zero or null value in the flatmap config, rather we're trying to maintain the previously applied value, ReadResource also needs to apply the normalizeNullValues step in order to prevent unexpected diffs.	7 years ago
James Bardin	11ec3a420e	remove normalizeFlatmapContainers ... This method was added early on when the diff was being applied as the legacy code would have done, which is no longer the case. Everything that normalizeFlatmapContainers does should be covered by the combination of the initial diff.Apply and the normalizeNullValues on the final cty.Value.	7 years ago
James Bardin	9d4bb6ec14	stop removing empty flatmap containers ... As we've improved the cty.Value normalization, we need to remove normalization procedures from the flatmap handling. Keeping the empty containers in the flatmap will prevent unexpected nils from being added to some schema configurations	7 years ago
James Bardin	6cdf9ff566	Revert "normalize all objects read from the provider" ... This reverts commit 209a0a460a.	7 years ago
James Bardin	209a0a460a	normalize all objects read from the provider ... Use objchange.NormalizeObjectFromLegacySDK to ensure that all objects returned from the provider match what is expected based on the configuration according to the schemas.	7 years ago
James Bardin	3600f59bb7	Merge pull request #20525 from hashicorp/jbardin/extra-set-value ... remove the partially-known ~ set sigil in diffs	7 years ago
James Bardin	2b4d030a69	don't re-add removed list values even when planned ... Providers were not strict (and were not forced to be) about customizing the diff when a computed attribute needed to be updated during apply. The fix we have in place to prevent loss of information during the helper/schema apply process would add in single missing value back in. The first place this was caught was when we attempt to fix up the flatmapped attributes. The 1->0 count error is now better handled by our cty.Value normalization step, so we can remove the special apply case here altogether The next place is in normalizeNullValues, and since the intent was to re-insert missing zero-value lists and sets, adding a check for a length of 0 protects us from adding in extra elements. The new test fixture emulated common provider behavior of re-computing values without customizing the diff. Since we can work around it, and core will provider appropriate warnings, the shims should try to maintain the legacy behavior.	7 years ago
James Bardin	47604c36c8	remove the partially-known ~ set sigil in diffs ... The NewExtra values are stored outside the diff from plan, and the original keys may not contain the ~ prefix. Adding the NewExtra back into the diff with the mismatched key was causing an entire new set element to be populated. Since this symbol isn't used to apply the diff in helper/schema, we can simply strip them out.	7 years ago
James Bardin	33d5ddf291	remove empty timeouts blocks in copyTimeoutValues ... The hcl2shims will always add in the timeouts block, because there's no way to differentiate a null single block from an empty one in the flatmapped state. Since we are only concerned with keeping the prior timeouts value, always set the new value to null, and then copy over the prior value if it exists.	7 years ago
James Bardin	49230f8198	existing fields cannot become computed during plan ... Fields with no change can only become computed during initial creation.	7 years ago
James Bardin	9a39af5047	1->0 set changes non longer should happen in Read ... The new normalization should make preventing those changes unnecessary, and will also prevent extra empty elements from being added when resources are refreshed.	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	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	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	8 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.	8 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.	8 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.	8 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.	8 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.	8 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.	8 years ago
Paul Tyng	bb9ae50279	Copy TF version to helper/schema provider	8 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.	8 years ago