Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need a "GetExists" or similar for determining if a key is set in a configuration #5694

Closed
catsby opened this issue Mar 17, 2016 · 8 comments
Closed

Comments

@catsby
Copy link
Contributor

catsby commented Mar 17, 2016

For AWS AutoScaling groups, max/min/desired capacity can be 0, so we added the code in #4693 to no longer use GetOk in checking these values, and instead just use Get.

#5681 reminds us that simply omitting desired_capacity should be allowed, too, since it's not actually required in the API.

We need to be know if a key was set at all, not just if it was set and not a non-default value.

d.Get:

// If you want to test if something is set at all in the configuration,
// use GetOk.

d.GetOk:

// GetOk returns the data for the given key and whether or not the key
// has been set to a non-zero value at some point.

We need to check if it's been set, regardless of the non-default status or not

@stack72
Copy link
Contributor

stack72 commented Mar 17, 2016

This ^^ :) Will be an awesome addition!

@phinze
Copy link
Contributor

phinze commented Mar 17, 2016

Definitely agreed that func (d *ResourceData) IsSetInConfig(key string) bool or something would be useful!

So the history here is that GetOk at once point claimed to be able to tell whether a value existed in the state, but it turned out to be broken. So in #993 we "fixed the glitch" in part by changing up the semantics of GetOk such that ok meant "non-zero" instead of the claimed-but-broken "present in config".

As @mitchellh mentioned there in a comment:

...you can't tell if something is set or not. 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.

So this issue can be the home for the fact that we do indeed want to support that in the future. 😁 As Mitchell says, it'll be a decent amount of work to thread through metadata in the diff such that we can properly detect absence from config vs default values. But I think it's probably worth it for the benefit it gives to provider authors.

@stack72
Copy link
Contributor

stack72 commented Mar 27, 2016

This is going to solve quite a few issues regarding setting of 0. I have linked a few that I have found so far

@ab
Copy link
Contributor

ab commented Sep 12, 2016

Anything that would be useful to help with / test out here? Just ran into #5681.

glasser added a commit to meteor/terraform that referenced this issue Nov 8, 2016
Specifically, this allows you to create a schedule which only changes
some of the group parameters without changing others, by setting the
parameters you wish to remain empty to -1.  This means you can adjust
min or max size without affecting the desired capacity, for example.

The ad hoc support for -1 isn't as nice as having real support for
leaving values out (see hashicorp#5694) but it solves a real use case.

Fixes hashicorp#5681.
glasser added a commit to meteor/terraform that referenced this issue Nov 8, 2016
Specifically, this allows you to create a schedule which only changes
some of the group parameters without changing others, by setting the
parameters you wish to remain empty to -1.  This means you can adjust
min or max size without affecting the desired capacity, for example.

The ad hoc support for -1 isn't as nice as having real support for
leaving values out (see hashicorp#5694) but it solves a real use case.

Fixes hashicorp#5681.
@pashap pashap mentioned this issue Nov 11, 2016
6 tasks
glasser added a commit to meteor/terraform that referenced this issue Dec 28, 2016
Specifically, this allows you to create a schedule which only changes
some of the group parameters without changing others, by setting the
parameters you wish to remain empty to -1.  This means you can adjust
min or max size without affecting the desired capacity, for example.

The ad hoc support for -1 isn't as nice as having real support for
leaving values out (see hashicorp#5694) but it solves a real use case.

Fixes hashicorp#5681.
@brycefisher
Copy link

I just ran into #5069 when trying to provision DNS MX records on DigitalOcean. Is this issue resolved in master?

glasser added a commit to meteor/terraform that referenced this issue Jun 29, 2017
Specifically, this allows you to create a schedule which only changes
some of the group parameters without changing others, by setting the
parameters you wish to remain empty to -1.  This means you can adjust
min or max size without affecting the desired capacity, for example.

The ad hoc support for -1 isn't as nice as having real support for
leaving values out (see hashicorp#5694) but it solves a real use case.

Fixes hashicorp#5681.
@jbmchuck
Copy link

jbmchuck commented Sep 8, 2017

+1, ran into this in another context.

@apparentlymart
Copy link
Contributor

In #15723 we added GetOkExists, which does approximately what was requested here.

We suspect it has some caveats on edge-cases where something is removed when already set to the default in the state, due to some details of how diffs and state are represented internally. In practice, it seems like it's been working well enough to be useful in the main case, and the remaining caveats should be dealt with via some long-term work to improve how we represent diffs and state so that "unset" is a separate idea from the zero value.

Since the broader work on the internal representations of things is on our radar (though not actually a GitHub issue, since it's a bit too broad/undefined at this time) and we've seen some success using GetOkExists in spite of the theoretical quirks, I'm going to close this issue. If people find issues related to the edge-cases I alluded to above then I'd encourage reporting them as separate, new issues where we can more easily get into the details.

@ghost
Copy link

ghost commented Apr 7, 2020

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 ghost locked and limited conversation to collaborators Apr 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants