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.

Sign up for GitHub

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

Add filter value properties with explicit types. #29

Merged
merged 10 commits into from
Aug 23, 2020
Merged

Add filter value properties with explicit types. #29

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0f71817
Add filter value properties with explicit types.
fitzoh Aug 20, 2020
6082fe6
terraform fmt the examples
Aug 22, 2020
7bfa3ba
Set global schema.DescriptionKind to markdown
Aug 22, 2020
bac3411
Document query.filter.value_*; deprecate untyped value
Aug 22, 2020
8fda550
Validate filter ops have a value when required
Aug 22, 2020
b1cb029
Switch query.filters to (ordered) TypeList instead of TypeSet
Aug 22, 2020
b13f086
Add explicit warning about value breaking filters on non-string columns
yvrhdn Aug 23, 2020
f093b16
Add explicit warning about value breaking filters on non-string columns
yvrhdn Aug 23, 2020
2066baf
Add explicit warning about value breaking filters on non-string columns
yvrhdn Aug 23, 2020
1a97b17
Tweak descriptions a bit
Aug 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions docs/data-sources/query.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,14 @@ Each query configuration may have zero or more `calculation` blocks, which each
Each query configuration may have zero or more `filter` blocks, which each accept the following arguments:

* `column` - (Required) The column to apply the filter to.
* `op` - (Required) The operator to apply, see the supported list of filter operators at [Filter Operators](https://docs.honeycomb.io/api/query-specification/#filter-operators).
* `value` - (Optional) The value to be used with the operator, not all operators require a value.
* `op` - (Required) The operator to apply, see the supported list of filter operators at [Filter Operators](https://docs.honeycomb.io/api/query-specification/#filter-operators). Not all operators require a value.
* `value_string` - (Optional) The value used for the filter when the column is a string. Mutually exclusive with `value` and the other `value_*` options.
* `value_integer` - (Optional) The value used for the filter when the column is an integer. Mutually exclusive with `value` and the other `value_*` options.
* `value_float` - (Optional) The value used for the filter when the column is a float. Mutually exclusive with `value` and the other `value_*` options.
* `value_boolean` - (Optional) The value used for the filter when the column is a boolean. Mutually exclusive with `value` and the other `value_*` options.
* `value` - (Optional) Deprecated: use the explicitly typed `value_string` instead. Mutually exclusive with the other `value_*` options.
yvrhdn marked this conversation as resolved.
Show resolved Hide resolved

-> **NOTE** The type of the filter value should match with the type of the column. To determine the type of a column visit the dataset settings page, all the columns and their type are listed under _Schema_. This provider will not be able to detect invalid combinations.

Each query configuration may have zero or more `order` blocks, which each accept the following arguments. An order term can refer to a `calculation` or a column set in `breakdowns`. When referring to a `calculation`, `op` and `column` must be the same as the calculation.

Expand Down
3 changes: 2 additions & 1 deletion honeycombio/data_source_query.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,9 @@ func dataSourceHoneycombioQuery() *schema.Resource {
},
"value": {
Type: schema.TypeString,
yvrhdn marked this conversation as resolved.
Show resolved Hide resolved
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.",
Description: "Deprecated: use the explicitly typed `value_string` instead. Mutually exclusive with the other `value_*` options.",
yvrhdn marked this conversation as resolved.
Show resolved Hide resolved
Optional: true,
Deprecated: "Use of attribute `value` is discouraged, prefer using the explicitly typed `value_*` variants instead",
yvrhdn marked this conversation as resolved.
Show resolved Hide resolved
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
},
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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to add that value won't work for non-string values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"value_string": {
Type: schema.TypeString,
Expand Down