helper/schema: change GetOk semantics to mean non-zero #993
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -828,7 +828,7 @@ func TestResourceDataGetOk(t *testing.T) { | |||
|
|
|||
| Key: "availability_zone", | |||
| Value: "", | |||
| Ok: true, | |||
| Ok: false, | |||
There was a problem hiding this comment.
This is the behavior change mentioned in the description.
|
I'm wrong, I looked around, and I don't think anything wanted this behavior (in builtins). If we want to support that in the future I think it'll require core changes to annotate the Diff about where the data came from. |
| } | ||
| } | ||
| // NOTE: ValueType has more functions defined on it in schema.go. We can't | ||
| // put them here because we reference other files. |
|
LGTM, this will make providers more sane |
mitchellh
added a commit
that referenced
this pull request
Feb 18, 2015
helper/schema: change GetOk semantics to mean non-zero
|
In my limited testing this fixes #950. |
phinze
added a commit
that referenced
this pull request
Feb 21, 2015
The `SourceDestCheck` attribute can only be changed via `ModifyInstance`, so the AWS instance resource's `Create` function calls out to `Update` before it returns to take care of applying `source_dest_check` properly. The `Update` function originally guarded against unnecessary API calls with `GetOk`, which worked fine until #993 when we changed the `GetOk` semantics to no longer distinguish between "configured and zero-value" and "not configured". I attempted in #1003 to fix this by switching to `HasChange` for the guard, but this does not work in the `Create` case. I played around with a few different ideas, none of which worked: (a) Setting `Default: true` on `source_dest_check' has no effect (b) Setting `Computed: true` on `source_dest_check' and adding a `d.Set` call in the `Read` function (which will initially set the value to `true` after instance creation). I really thought I could get this to work, but it results in the following: ```go d.Get('source_dest_check') // true d.HasChange('source_dest_check') // false d.GetChange('source_dest_check') // old: false, new: false ``` I couldn't figure out a way of coherently dealing with that result, so I ended up throwing up my hands and giving up on the guard altogether. We'll call `ModifyInstance` more than we have to, but this at least yields expected behavior for both Creates and Updates. Fixes #1020
yahyapo
pushed a commit
to yahyapo/terraform
that referenced
this pull request
Mar 13, 2015
The `SourceDestCheck` attribute can only be changed via `ModifyInstance`, so the AWS instance resource's `Create` function calls out to `Update` before it returns to take care of applying `source_dest_check` properly. The `Update` function originally guarded against unnecessary API calls with `GetOk`, which worked fine until hashicorp#993 when we changed the `GetOk` semantics to no longer distinguish between "configured and zero-value" and "not configured". I attempted in hashicorp#1003 to fix this by switching to `HasChange` for the guard, but this does not work in the `Create` case. I played around with a few different ideas, none of which worked: (a) Setting `Default: true` on `source_dest_check' has no effect (b) Setting `Computed: true` on `source_dest_check' and adding a `d.Set` call in the `Read` function (which will initially set the value to `true` after instance creation). I really thought I could get this to work, but it results in the following: ```go d.Get('source_dest_check') // true d.HasChange('source_dest_check') // false d.GetChange('source_dest_check') // old: false, new: false ``` I couldn't figure out a way of coherently dealing with that result, so I ended up throwing up my hands and giving up on the guard altogether. We'll call `ModifyInstance` more than we have to, but this at least yields expected behavior for both Creates and Updates. Fixes hashicorp#1020
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the
GetOksemantics to mean that a value is non-zero. The prior semantics were: "it is set in the configuration." But that used the assumption that a diff would only contain values that are non-zero IF they were in the configuration. With some bug fixes today, this assumption is no longer true: the diff can contain changes that aren't necessarily from the configuration, but could be.Before this commit,
GetOkwas then totally broken: it always returnedtrue, basically, because the diff contains a lot more information about zero values being set. This was required for #952 and friends.This makes the requirement slightly more strict which is "it must exist AND it must be non-zero". I took a look around the builtins and this looks to be what everyone is assuming.
If the provider author wants to check that its set and is zero, then they can just use
Get, which returns a zero value (never nil) for the proper type. And if they want to set a default that is non-zero, then theDefaultfield is still available.