Commit Graph

26 Commits

Author SHA1 Message Date
Paddy Carver 2eadc8f625 Don't allow sub-blocks for SetNew.
You can't set individual items in lists, you can only set the list as a
whole, so we shouldn't allow sub-blocks to SetNew or SetNewComputed.
2018-09-26 12:38:38 -07:00
Paddy Carver 7ee4339555 Allow for nested fields with checkKey on ResourceDiffs.
When checking if a ResourceDiff key is valid, allow for keys that exist
on sub-blocks. This allows things like diff.Clear to be called on
sub-block fields, instead of just on top-level fields.
2018-09-05 17:29:03 -07:00
Chris Marchesi d7048cb640
helper/schema: ResourceDiff ForceNew attribute correctness
A couple of bugs have been discovered in ResourceDiff.ForceNew:

* NewRemoved is not preserved when a diff for a key is already present.
This is because the second diff that happens after customization
performs a second getChange on not just state and config, but also on
the pre-existing diff. This results in Exists == true, meaning nil is
never returned as a new value.
* ForceNew was doing the work of adding the key to the list of changed
keys by doing a full SetNew on the existing value. This has a side
effect of fetching zero values from what were otherwise undefined values
and creating diffs for these values where there should not have been
(example: "" => "0").

This update fixes these scenarios by:

* Adding a new private function to check the existing diff for
NewRemoved keys. This is included in the check on new values in
diffChange.
* Keys that have been flagged as ForceNew (or parent keys of lists and
sets that have been flagged as ForceNew) are now maintained in a
separate map. UpdatedKeys now returns the results of both of these maps,
but otherwise these keys are ignored by ResourceDiff.
* Pursuant the above, values are no longer pushed into the newDiff
writer by ForceNew. This prevents the zero value problem, and makes for
a cleaner implementation where the provider has to "manually" SetNew to
update the appropriate values in the writer. It also prevents
non-computed keys from winding up in the diff, which ResourceDiff
normally blocks by design.

There are also a couple of tests for cases that should never come up
right now involving Optional/Computed values and NewRemoved, for which
explanations are given in annotations of each test. These are here to
guard against future regressions.
2018-04-08 07:50:41 -07:00
Chris Marchesi fdfe89ed1d
helper/schema: Computed -> NewValueKnown, add comments to GetOkExists
Added some more context to GetOkExists, moved Computed to NewValueKnown
to accommodate some changes that will be coming up in HCL2 that may make
"Computed" less intuitive of a function name, and updated the docs for
NewValueKnown as well.
2018-03-29 15:18:35 -07:00
Chris Marchesi 274b933077
helper/schema: Add Computed to ResourceDiff, expose GetOkExists
This adds a new method to ResourceDiff: Computed, which exposes the
computed read result field to ResourceDiff. In the context of
customizing the diff, this is important as interpolated and otherwise
computed values will show up in the diff as blank, with no way of
determining if the value is actually blank or if it's a computed value
not available at diff customization time. Currently assumptions need to
be made on this, but this does not help in validation scenarios where
one needs to differentiate between an actual blank value and a value
that will be available later.

This is exposed for the most part via NewComputed in the diff, but the
tests cover both the config reader as well (with no diff, even though
this should not come up in normal operation) and also the newDiff reader
when someone sets a new value using SetNew and SetNewComputed.

This commit also exposes GetOkExists. The tests were mostly pulled from
ResourceData but a few were added to ensure that config was being
properly covered as well, in addition to covering SetNew and
SetNewComputed.
2018-03-29 12:04:36 -07:00
Radek Simko ccf8a31cbb
helper/schema: Allow ResourceDiff.ForceNew on nested fields (avoid crash) 2018-03-01 15:51:01 +00:00
Chris Marchesi cb5ce1d35e
helper/schema: Extend diffChange and bubble up customized values
This extends the internal diffChange method so that ResourceDiff's
implementation of it can report back whether or not the value came from
a customized diff.

This is an effort to work to preserve the pre-ResourceDiff behaviour
that ignores the diff for computed keys when the old value was populated
but the new value wasn't - this behaviour is actually being depended on
by users that are using it to exploit using zero values in modules. This
should allow both scenarios to co-exist by shifting the NewComputed
exemption over to exempting values that come from diff customization.
2017-12-19 16:06:57 -08:00
Chris Marchesi 2c541e8c97 helper/schema: Add behaviour detail to various custom diff comments
Added some more detailed comments to CustomizeDiff's comments. The new
comments detail how CustomizeDiff will be called in the event of
different scenarios like creating a new resource, diffing an existing
one, diffing an existing resource that has a change that requires a new
resource, and destroy/tainted resources.

Also added similar detail to ForceNew in ResourceDiff.

This should help mitigate any confusion that may come up when using
CustomizeDiff, especially in the ForceNew scenario when the second run
happens with no state.
2017-11-01 15:42:24 -07:00
Chris Marchesi f5bdbc0911 helper/schema: Simplify setDiff, remove exported SetDiff mentions
setDiff does not make use of its new parameter anymore, so it has been
removed. Also, as there is no more SetDiff (exported) function, mentions
of that have been removed from comments too.
2017-11-01 14:25:32 -07:00
Chris Marchesi 3ac0cdf0c0 helper/schema: Better ResourceDiff schema key validation
This fixes nil pointer issues that could come up if an invalid key was
referenced (ie: not one in the schema). Also ships a helper validation
function to streamline things.
2017-11-01 14:25:32 -07:00
Chris Marchesi 9625830980 helper/schema: Move computedKeys init to init function
This simplifies the new value initialization and future-proofs it
against more complex initialization functionality.
2017-11-01 14:25:32 -07:00
Chris Marchesi 3444549d60 helper/schema: Move computed key reader logic to childAddrOf
This will make this check safer and will remove the risk of it passing
on keys with a similar prefix.
2017-11-01 14:25:32 -07:00
Chris Marchesi 36aa63b338 helper/schema: Guard against out of range on childAddrOf
This could panic if we sent a parent that was actually longer than the
child (should almost never come up, but the guard will make it safe
anyway).

Also fixed a slice style lint warning in UpdatedKeys.
2017-11-01 14:25:32 -07:00
Chris Marchesi 931b0e1441 helper/schema: Remove exported SetDiff method
The consensus is that it's a generally better idea to move setting the
functionality of old values completely to the refresh/read process,
hence it's moot to have this function around anymore. This also means we
don't need the old value reader/writer anymore, which simplifies things.
2017-11-01 14:25:32 -07:00
Chris Marchesi ad98471559 helper/schema: Correct some comments
Removed some outdated comments for SetNew, and normalized the computed
key note for SetNew, SetNewComputed, and SetDiff.
2017-11-01 14:25:32 -07:00
Chris Marchesi 8a7c9a6f02 helper/schema: frist -> first 2017-11-01 14:25:32 -07:00
Chris Marchesi 7d5f9ed6b1 helper/schema: NewComputed values should be nil
When working on this initially, I think I thought that since NewComputed
values in the diff were empty strings, that it was using the zero value.
After review, it doesn't seem like this is the case - so I have adjusted
NewComputed to pass nil values. There is also a guard now that keeps the
new value writer from accepting computed fields with non-nil values.
2017-11-01 14:25:32 -07:00
Chris Marchesi 6f422d8c44 helper/schema: Remove unused log line
Meant to remove this before finalizing the PR. :P
2017-11-01 14:25:32 -07:00
Chris Marchesi 8af9610b87 helper/schema: Hook CustomizeDiffFunc into diff logic
It's alive! CustomizeDiff logic now has been inserted into the diff
process. The test_resource_with_custom_diff resource provides some basic
testing and a reference implementation.

There should now be plenty of test coverage for this feature via the
tests added for ResourceDiff, and the basic test added to the
schemaMap.Diff test, and the test resource, but more can be added to
test any specific case that comes up otherwise.
2017-11-01 14:25:32 -07:00
Chris Marchesi 196d7e63fe helper/schema: Drop ClearAll from ResourceDiff
As the interface has been specced out for ResourceDiff, the only diff
operation that can function on a non-computed key is ForceNew, and that
is only if a diff exists for that key. As such, allowing the user to
clear keys that cannot be operated on afterwards does not really make
much sense.

Will re-add this function if it is determined it's needed after all on
review.
2017-11-01 14:25:32 -07:00
Chris Marchesi 22220fd0f7 helper/schema: Final set of ResourceDiff tests, bug fixes
Final set of coverage for ResourceDiff and bug fixes to correct issues
the test cases brought up.

Will be dropping ClearAll in next commit, as with how this interface is
intended to be used, it does not really make much sense.
2017-11-01 14:25:32 -07:00
Chris Marchesi 64cc4084d2 helper/schema: ResourceDiff tests, bug fixes
The beginning of tests for SetNew. Noticed that removed values are not
logged for computed keys in a diff - not too sure if that is going to be
a problem (possibly not as GetChange in ResourceData should still
reference state for the old value). I came on this most notably in the
"TestSetNew/complex-ish_set_diff" test, for reference.

More tests pending for other functions for coverage of other functions
and test cases as defined in #8769.
2017-11-01 14:25:32 -07:00
Chris Marchesi f5f4e0329f helper/schema: Track updated keys in ResourceDiff
This ensures that when we hook this into the main diff logic, that we
can just re-diff the keys that are modified, ensuring that the diff is
not re-run on keys that were not touched, or on keys that were
intentionally removed.
2017-11-01 14:25:32 -07:00
Chris Marchesi aeb793f968 helper/schema: Add Clear function to ResourceDiff
This should complete the feature set of the prototype. This function
removes a specific key from the existing diff, preventing conflicts.

Further functionality might be needed to make this behave as expected,
namely ensuring that we don't re-process the whole diff after the
CustomizeDiffFunc runs.
2017-11-01 14:25:32 -07:00
Chris Marchesi 1e8cfc52e9 helper/schema: Updated ResourceDiff prototype
This new prototype removes the dependence on a underlying ResourceData,
moving several items up to ensure an approach that more matches
ResourceData. Namely, we create our own MultiLevelFieldReader that
also layers updated diff values on top. New values use a small
re-implementation of the MapFieldWriter/Reader that allows computed
values to be set.

The assumption here now is that a second diff will happen after the
first one is done, processing the new values set in the 2 new
reader/writer levels and updating the diff as necessary.
2017-11-01 14:25:32 -07:00
Chris Marchesi b99c615ee6 helper/schema: New ResourceDiff object
This adds a new object, ResourceDiff, to the schema package. This
object, in conjunction with a function defined in CustomizeDiff in the
resource schema, allows for the in-flight customization of a Terraform
diff. This helps support use cases such as when there are necessary
changes to a resource that cannot be detected in config, such as via
computed fields (most of the utility in this object works on computed
fields only). It also allows for a wholesale wipe of the diff to allow
for diff logic completely offloaded to an external API, if it is a
better use case for a specific provider.

As part of this work, many internal diff functions have been moved to
use a special resourceDiffer interface, to allow for shared
functionality between ResourceDiff and ResourceData. This may be
extended to the DiffSuppressFunc as well which would restrict use of
ResourceData in DiffSuppressFunc to generally read-only fields.

This work is not yet in its final state - CustomizeDiff is not yet
implemented in the general diff workflow, new functions may be added
(notably Clear() for a single key), and functionality may be altered.
Tests will follow as well.
2017-11-01 14:25:32 -07:00