Add filter value properties with explicit types. #29
Conversation
| Type: schema.TypeString, | ||
| Description: "The value used for the filter. Use of the explicitly typed `value_*` variants is recommended until the honeycomb API is able to support type inference as initially described in https://github.com/kvrhdn/terraform-provider-honeycombio/issues/27.", | ||
| Optional: true, | ||
| }, |
There was a problem hiding this comment.
| }, | |
| Deprecated: "Use of attribute `value` is discouraged, prefer using the explicitly typed `value_*` variants instead", | |
| }, |
We could set the Deprecated attribute to warn users when using value instead of value_*. But it feels a bit weird since we are not necessarily deprecating it...
This results in:
➜ terraform apply
data.honeycombio_query.query[2]: Refreshing state... [id=2435509962]
data.honeycombio_query.query[3]: Refreshing state... [id=1242873032]
data.honeycombio_query.query[1]: Refreshing state... [id=1329679303]
data.honeycombio_query.query[0]: Refreshing state... [id=2511879708]
honeycombio_board.board: Refreshing state... [id=rpAjr7deUcb]
Warning: Deprecated Attribute
on main.tf line 17, in data "honeycombio_query" "query":
17: data "honeycombio_query" "query" {
Use of attribute `value` is discouraged, prefer using the explicitly typed
`value_*` variants
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.
Thoughts?
There was a problem hiding this comment.
We might want to add that value won't work for non-string values
There was a problem hiding this comment.
I don't know, I'm starting to think it might make sense to just remove it for now since it's fundamentally broken for a common use case
There was a problem hiding this comment.
Yeah, I agree that if you are using value instead of value_string you're likely going to be making mistakes and there is no way for the provider to warn about this.
Since we are adding explicit types already I think it's best to go all the way and deprecate use of value. This will clearly nudge users to consider the type of value and hopefully use the correct value_* variant.
|
Hi @fitzoh, I think this is ready to merge (and release). Do you want to give the last commits I added a final review before merging? |
Added a couple explicit warnings around Other than that I'd say Also +1 for the list changes |
Co-authored-by: Andrew Fitzgerald <andrew.fitzgerald@cardinalhealth.com>
Co-authored-by: Andrew Fitzgerald <andrew.fitzgerald@cardinalhealth.com>
Co-authored-by: Andrew Fitzgerald <andrew.fitzgerald@cardinalhealth.com>
|
Thanks again @cah-andrew-fitzgerald. I'll release this with v0.0.9 🚀 |
|
And thank you for the cleanup/docs @kvrhdn |
As described in #27, the Honeycomb API currently fails to parse queries if a filter is sent with the wrong type in the JSON payload.
This PR adds new
value_*options to the filter schema for each type in the Honeycomb API.These properties are mutually exclusive with themselves and the original
valuefield.Feel free to either comment or edit @kvrhdn.
If you'd prefer to go with one of the other options enumerated in #27 that's fine too, just putting an option out there.
Fixes #27