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

docs for GetOkExists are misleading #283

Closed
sophaskins opened this issue Dec 18, 2019 · 2 comments · Fixed by #350
Closed

docs for GetOkExists are misleading #283

sophaskins opened this issue Dec 18, 2019 · 2 comments · Fixed by #350
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@sophaskins
Copy link

Terraform Version

$ terraform -v
Terraform v0.12.18

Terraform Configuration Files

n/a

Debug Output

n/a

Crash Output

n/a

Expected Behavior

Documentation matches code behavior

Actual Behavior

The documentation for GetOkExists in the github.com/hashicorp/terraform/helper/schema package starts the same as the documentation for GetOk:

https://github.com/hashicorp/terraform/blob/f1237f816c5d00c59648d3170c7b8f4afe6d982d/helper/schema/resource_data.go#L111-L125

Notably:

GetOkExists returns the data for a given key and whether or not the key has been set to a non-zero value.

However, the implementation for GetOkExists (and the rest of its line comments!) indicate that it does not check if the key has been set to a non-zero value, and that indeed that's why it exists as a function in the first place!

Steps to Reproduce

n/a

Additional Context

I ran in to this debugging an issue in a plugin where I was unable to set a value of 0 kevholditch/terraform-provider-kong#93. It turned out that the field was being read with GetOk and thus, when it was 0, it was being turned by the plugin in to nil as if it hadn't been set.

When digging in the code, I was somewhat confused until I realized the line comments were not in line with the code.

I didn't just open this as a PR because my research also indicated that GetOk and GetOkExists have a somewhat complicated history, and it's possible there's some subtlety going on here that I do not understand.

References

hashicorp/terraform#15723
hashicorp/terraform#4936 (comment)

@jbardin jbardin transferred this issue from hashicorp/terraform Dec 18, 2019
@radeksimko radeksimko added the documentation Improvements or additions to documentation label Jan 2, 2020
@paultyng
Copy link
Contributor

@sophaskins thanks for reporting this, like many things in this SDK, there for sure is a complicated history. Most of us are also not original authors so its really hard to say original intent, but I think your observation seems correct to me, so can update the godoc for now as the existence and naming of this function of course implies different behavior!

@paultyng paultyng self-assigned this Jan 29, 2020
@paultyng paultyng assigned appilon and unassigned paultyng Mar 5, 2020
@ghost
Copy link

ghost commented Apr 11, 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 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants